bug #51478 [Process] Fix bug where $this->callback is never null, resulting in bad argument (weaverryan)

This PR was squashed before being merged into the 6.4 branch.

Discussion
----------

[Process] Fix bug where $this->callback is never null, resulting in bad argument

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | None
| License       | MIT
| Doc PR        | Not needed

Hi!

Caused by #51068 - by the time `$this->getDescriptors()` is called, `$this->callback` is ALWAYS a callable thanks to `$this->callback = $this->buildCallback($callback)`.

Previously, there was a separate `hasCallback` property that handled this logic. I chose to NOT re-add that because it was only needed in `getDescriptors()` and that is only called from this one place: easier to pass `$hasCallback` as an argument.

Caught by the symfony/ux test suite ❤️

Cheers!

Commits
-------

95aa23699f2 [Process] Fix bug where $this->callback is never null, resulting in bad argument

Author: fabpot

Process.php

@@ -304,7 +304,7 @@ public function start(callable $callback = null, array $env = [])
         $this->resetProcessData();
         $this->starttime = $this->lastOutputTime = microtime(true);
         $this->callback = $this->buildCallback($callback);
-        $descriptors = $this->getDescriptors();
+        $descriptors = $this->getDescriptors(null !== $callback);
 
         if ($this->env) {
             $env += '\\' === \DIRECTORY_SEPARATOR ? array_diff_ukey($this->env, $env, 'strcasecmp') : $this->env;
@@ -1240,15 +1240,15 @@ public static function isPtySupported(): bool
     /**
      * Creates the descriptors needed by the proc_open.
      */
-    private function getDescriptors(): array
+    private function getDescriptors(bool $hasCallback): array
     {
         if ($this->input instanceof \Iterator) {
             $this->input->rewind();
         }
         if ('\\' === \DIRECTORY_SEPARATOR) {
-            $this->processPipes = new WindowsPipes($this->input, !$this->outputDisabled || $this->callback);
+            $this->processPipes = new WindowsPipes($this->input, !$this->outputDisabled || $hasCallback);
         } else {
-            $this->processPipes = new UnixPipes($this->isTty(), $this->isPty(), $this->input, !$this->outputDisabled || $this->callback);
+            $this->processPipes = new UnixPipes($this->isTty(), $this->isPty(), $this->input, !$this->outputDisabled || $hasCallback);
         }
 
         return $this->processPipes->getDescriptors();

Tests/ProcessTest.php

@@ -200,6 +200,20 @@ public function testCallbacksAreExecutedWithStart()
         $this->assertSame('foo'.\PHP_EOL, $data);
     }
 
+    public function testReadSupportIsDisabledWithoutCallback()
+    {
+        $this->expectException(LogicException::class);
+        $this->expectExceptionMessage('Pass the callback to the "Process::start" method or call enableOutput to use a callback with "Process::wait".');
+
+        $process = $this->getProcess('echo foo');
+        // disabling output + not passing a callback to start() => read support disabled
+        $process->disableOutput();
+        $process->start();
+        $process->wait(function ($type, $buffer) use (&$data) {
+            $data .= $buffer;
+        });
+    }
+
     /**
      * tests results from sub processes.
      *