Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Workaround for constructor signature change in Symfony/Process #2766

Merged
merged 2 commits into from
Aug 13, 2021

Conversation

bobdenotter
Copy link
Member

@bobdenotter bobdenotter commented Aug 12, 2021

The important part of this PR is:

        // Depending on the context, we're using either Symfony/Process 2.8.52 (bundled with composer 2.1.x)
        // or Symfony/Process 5.3.x (if we're using our own). The signature of the Constructor changed
        // from: `public function __construct(string $commandline, …)`
        // to:   `public function __construct(array $command, …)`
        // We'll have to attempt one, and otherwise fall back to the other.

        try {
            $process = new Process($command);
        } catch (\TypeError $e) {
            $process = new Process([$command]);
        }

This was fun to figure out! (not)

Comment on lines +46 to +50
try {
$process = new Process($command);
} catch (\TypeError $e) {
$process = new Process([$command]);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Safer would be:

Suggested change
try {
$process = new Process($command);
} catch (\TypeError $e) {
$process = new Process([$command]);
}
$process = Process::fromShellCommandline($command);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will try that out.. If that works, that'll be a lot less fugly!

Thanks for the suggestion, @stloyd!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alas, it doesn't seem to work in the context of composer create-project, because it seems that method isn't present yet in symfony/process 2.8.x, as used by Composer.

It breaks with this error:

…
…
120 packages you are using are looking for funding.
Use the `composer fund` command to find out more!

Run composer recipes at any time to see the status of your Symfony recipes.

> Bolt\ComposerScripts\ProjectEventHandler::postUpdate

 ! [NOTE] Running composer "post-update-cmd" scripts

PHP Fatal error:  Uncaught Error: Call to undefined method Symfony\Component\Process\Process::fromShellCommandline() in /Users/bob/Sites/Bolt/playground/vendor/bolt/core/bin/composer-script/Script.php:39
Stack trace:
#0 /Users/bob/Sites/Bolt/playground/vendor/bolt/core/bin/composer-script/PostUpdateScript.php(13): Bolt\ComposerScripts\Script::run('php vendor/bobd...')
#1 /Users/bob/Sites/Bolt/playground/vendor/bolt/core/bin/composer-script/ProjectEventHandler.php(44): Bolt\ComposerScripts\PostUpdateScript::execute()
#2 phar:///usr/local/bin/composer/src/Composer/EventDispatcher/EventDispatcher.php(377): Bolt\ComposerScripts\ProjectEventHandler::postUpdate(Object(Composer\Script\Event))
#3 phar:///usr/local/bin/composer/src/Composer/EventDispatcher/EventDispatcher.php(236): Composer\EventDispatcher\EventDispatcher->executeEventPhpScript('Bolt\\ComposerSc...', 'postUpdate', Object(Composer\Script\Event))
#4 phar:///usr/local/bin/composer/src/Composer/EventDispatcher/EventDispatcher.php(117): Composer\EventDispatcher\EventDispatc in /Users/bob/Sites/Bolt/playground/vendor/bolt/core/bin/composer-script/Script.php on line 39

Copy link
Member

@mcdennem mcdennem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@bobdenotter bobdenotter merged commit f057eac into 4.2 Aug 13, 2021
@bobdenotter bobdenotter deleted the fix/symfony-process-constructor branch August 13, 2021 07:32
@bobdenotter bobdenotter mentioned this pull request Aug 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants