ComposerValidator should check if proc_open() is available

ComposerExecutableValidator no longer exists.

AFAICT this is still necessary in all places that interact with ProcessFactory, which in our case is everything using ComposerRunnerInterface.

Which at this point is only \Drupal\package_manager\ComposerInspector.

I am not sure where it will throw process error in \Drupal\package_manager\ComposerInspector

What happened to the nice & simple MR?!? 310 changes now! 🤪

Clearly the pre-existing MR is for 8.x-2.x. You should have created a new one for 3.0.x rather than futzing with the existing MR.

There is now literally more work than there was a few hours ago, because the correct example for 8.x-2.x has now been destroyed, GitLab does not allow me to recover it AFAICT, since it does not show the commit prior to your force push. 😢

If I remember correctly off the top of my head… then it's basically any invocation of \PhpTuf\ComposerStager\Domain\Service\ProcessRunner\ComposerRunnerInterface::run(), because that's what's interacts with Symfony Process.

It was something like:

try {
  runner->run('--version')
}
catch (LogicException $e) {
  $event->addError([
        $e->getMessage(),
      ]);
}

Do we want to surround first occurrences of \PhpTuf\ComposerStager\Domain\Service\ProcessRunner\ComposerRunnerInterface::run() in try catch or all of them, or the error can happen at any time?

#17: all.

Discussed this with @tedbow and @phenaproxima and decided that we don't want to do any logical changes here but just mention in out hook_help that the user need to enable proc_open.

I was following @TravisCarden's direction from 8 months ago in #3 a little bit too literally it seems 😅

I agree with @tedbow & @phenaproxima that we should not be catching this exception all over the place if it can only occur if proc_open() is disabled on a particular PHP install. But I don't see that in \PhpTuf\ComposerStager\Infrastructure\Factory\Process\ProcessFactory::create():

final class ProcessFactory implements ProcessFactoryInterface
{
    public function create(array $command): Process
    {
        try {
            return new Process($command);
        } catch (ExceptionInterface $e) { // @codeCoverageIgnore
            throw new LogicException($e->getMessage(), 0, $e); // @codeCoverageIgnore
        }
    }
}

Ah, but \Symfony\Component\Process\Process::__construct() has this:

    /**
     * @param array          $command The command to run and its arguments listed as separate entries
     * @param string|null    $cwd     The working directory or null to use the working dir of the current PHP process
     * @param array|null     $env     The environment variables or null to use the same environment as the current PHP process
     * @param mixed          $input   The input as stream resource, scalar or \Traversable, or null for no input
     * @param int|float|null $timeout The timeout in seconds or null to disable
     *
     * @throws LogicException When proc_open is not installed
     */
    public function __construct(array $command, string $cwd = null, array $env = null, mixed $input = null, ?float $timeout = 60)
    {
…
    }

So basically, \PhpTuf\ComposerStager\Infrastructure\Factory\Process\ProcessFactory::create() is catching far too broadly and then re-throwing specifically. I don't get why but … that's out of scope here.

Conclusion:

• Not catching LogicException everywhere ComposerRunnerInterface is used is fine 👍
• hook_help() entry is fine 👍
• But this is not enough. If proc_open() is disabled, there's literally nothing Package Manager can do. This should hence be a base requirement validator. Actually, arguably, this is an even more fundamental requirement than ComposerValidator, because without this we literally cannot call composer at all! 😳🫠

  That means that our existing base requirement \Drupal\package_manager\Validator\ComposerValidator will FAIL if proc_open() is disabled. But not in a way that we're catching today.

  So I think we should explicitly detect if proc_open() is available (if (!\function_exists('proc_open')) { …) as the first thing in \Drupal\package_manager\Validator\ComposerValidator::validate().

Wim Leers → closed merge request !548

The code looks fine but please refine both the formatting and the English. This is very sloppy: it looks hastily written… 🙈

This still has very obvious problems.

Just changed comments so not expecting to fail that's why.

I have one doubt on this.

Nice improvement!

Automatically closed - issue fixed for 2 weeks with no activity.

FYI: I went ahead and added a precondition for this to Composer Stager itself: https://github.com/php-tuf/composer-stager/pull/192