Output buffer - fix 5851

[kkmuffme]

Fix #5851

[codecov]

Codecov Report

Attention: Patch coverage is 74.76636% with 27 lines in your changes are missing coverage. Please review.

Project coverage is 92.22%. Comparing base (3e6c0dc) to head (6e8e56a).

Files	Patch %	Lines
src/Framework/TestCase.php	74.76%	27 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5852      +/-   ##
============================================
- Coverage     92.32%   92.22%   -0.10%     
- Complexity     6555     6597      +42     
============================================
  Files           699      699              
  Lines         19775    19869      +94     
============================================
+ Hits          18257    18324      +67     
- Misses         1518     1545      +27     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sebastianbergmann]

Let me start by thanking you for taking the time to investigate this issue!

@theseer, @localheinz, @staabm, @Schrank, @sebastianheuer, @Tesla91, and I discussed this issue during the PHPUnit Code Sprint in Munich today.

We are not convinced we need to distinguish all the different cases. Here is our attempt to solve the two main issues of "do not close PHPUnit's buffer" and "do not leave your buffer open":

diff --git a/src/Framework/TestCase.php b/src/Framework/TestCase.php
index 2d749ea24..cabbee413 100644
--- a/src/Framework/TestCase.php
+++ b/src/Framework/TestCase.php
@@ -10,6 +10,7 @@
 namespace PHPUnit\Framework;
 
 use const PHP_EOL;
+use const PHP_OUTPUT_HANDLER_REMOVABLE;
 use function array_keys;
 use function array_merge;
 use function array_reverse;
@@ -36,6 +37,7 @@
 use function ob_get_clean;
 use function ob_get_contents;
 use function ob_get_level;
+use function ob_get_status;
 use function ob_start;
 use function preg_match;
 use function restore_error_handler;
@@ -1434,9 +1436,18 @@ private function markSkippedForMissingDependency(ExecutionOrderDependency $depen
         $this->status = TestStatus::skipped($message);
     }
 
+    private function outputBufferCallback(): string
+    {
+        if (!$this->outputBufferingActive) {
+            return '';
+        }
+
+        throw new Exception('Test code or tested code flushed or cleaned global output buffers other than its own');
+    }
+
     private function startOutputBuffering(): void
     {
-        ob_start();
+        ob_start([$this, 'outputBufferCallback']);
 
         $this->outputBufferingActive = true;
         $this->outputBufferingLevel  = ob_get_level();
@@ -1449,27 +1460,28 @@ private function stopOutputBuffering(): bool
         if ($bufferingLevel !== $this->outputBufferingLevel) {
             if ($bufferingLevel > $this->outputBufferingLevel) {
                 $message = 'Test code or tested code did not close its own output buffers';
-            } else {
-                $message = 'Test code or tested code closed output buffers other than its own';
+
+                Event\Facade::emitter()->testConsideredRisky(
+                    $this->valueObjectForEvents(),
+                    $message,
+                );
+
+                $this->status = TestStatus::risky($message);
             }
 
             while (ob_get_level() >= $this->outputBufferingLevel) {
+                if (!(ob_get_status()['flags'] & PHP_OUTPUT_HANDLER_REMOVABLE)) {
+                    break;
+                }
+
                 ob_end_clean();
             }
 
-            Event\Facade::emitter()->testConsideredRisky(
-                $this->valueObjectForEvents(),
-                $message,
-            );
-
-            $this->status = TestStatus::risky($message);
-
             return false;
         }
 
-        $this->output = ob_get_clean();
-
         $this->outputBufferingActive = false;
+        $this->output                = ob_get_clean();
         $this->outputBufferingLevel  = ob_get_level();
 
         return true;

What do you think?

[kkmuffme]

Thanks!
It seems this addresses only issue 1) of #5851 while 2-5) remain unsolved (but are fixed by the changes in my PR)

Also there is a bug in the changed code still:

$this->output = ob_get_clean();

This will throw a notice if your buffer was closed and replaced by a non-removable one, since the ob level is the same. This issue is fixed in my PR though.

I discussed this issue during the PHPUnit Code Sprint in Munich today.
We are not convinced we need to distinguish all the different cases. Here is our attempt to solve the two main issues of "do not close PHPUnit's buffer" and "do not leave your buffer open"

If you could provide a very short summary of the discussion/why these cases don't need to be differentiated, it would greatly appreciate it. (if those are in German that's alright too)

[theseer]

Also there is a bug in the changed code still:

$this->output = ob_get_clean();

This will throw a notice if your buffer was closed and replaced by a non-removable one, since the ob level is the same.
This issue is fixed in my PR though.

I admit I didn't verify this by actually runnig a test for it. But I cannot see how this could be the case? In case our buffer is closed, the callback should have been called - which throws an exception. It should make the potentially problematic line unreachable. Which should cover your case 2 just fine.

But I'll create a testcase for that and see :)

I'll try to create separate cases for each of your other issues mentioned and see if our simplified oversimplified things ;)

I discussed this issue during the PHPUnit Code Sprint in Munich today.
We are not convinced we need to distinguish all the different cases. Here is our attempt to solve the two main issues of "do not close PHPUnit's buffer" and "do not leave your buffer open"

If you could provide a very short summary of the discussion/why these cases don't need to be differentiated, it would greatly appreciate it. (if those are in German that's alright too)

[kkmuffme]

which throws an exception

Thanks, I didn't see the forest for the trees! You are right, in that case we won't get a notice from there

because it seems this creates some other issues:

1. This will cause any output to be printed though - this behavior actually has a bug report (funnily enough by @mvorisek ) ob handler should not print the output in case of callback exception php/php-src#8866

2. any code that comes after it will never execute when I try to flush when !$this->outputBufferingActive:*
   https://3v4l.org/S7f87

*and if it wouldn't do that, it would cause it to be called twice due to a bug in php php/php-src#16133

3. assuming the !$this->outputBufferingActive - when code calls exit https://3v4l.org/ep7XJ (or implicit due to an error in the code/test), it would just report it as risky bc the buffer was flushed - but that wasn't really the issue the test had (but that it called exit or had a PHP error)

4. if the exception handler itself (e.g. if it was changed by the user or possibly by phpunit at some point) uses a buffered function (ob start, but I think even just calling print_r with 2nd arg true is sufficient) then this will result in a fatal error, since calling buffered functions inside the callback of a buffer isn't allowed

Since it's been a while since I reported this issue, I'm not 100% into it anymore right now (and also don't have time to get into it until Christmas), but buffers are extremely complex not just by their nature, but also bc PHP behaves incredibly confusing and the docs are often wrong or outdated too - so please feel free to point out any mistakes/wrong assumptions I made!