[Process] Introducing a new `PhpSubprocess` handler #48485

Toflar · 2022-12-05T15:03:22Z

Q	A
Branch?	6.4
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets
License	MIT
Doc PR	will provide as soon as the API is clear

This PR provides a new PhpSubprocess handler which can be used to run PHP commands and taking over their parent PHP configuration. This is not super easy in PHP because the only way to do that requires a temporary ini file.
That's why I see perfect fit for it in the symfony/process component as it solves a common issue.

The code is heavily inspired/copied from composer/xdebug-handler where the requirement is similar - being able to have a child process/subprocess that takes over the config of the parent process. Hence, mentioning @Seldaek here.

Basically, this can be very useful in Symfony when you want to run bin/console other-command from within Symfony.
Let's imagine a general cronjob command that is supposed to delegate to other commands like this (simplified a lot):

#[AsCommand(name: 'run-cronjobs')]
class RunCronJobsCommand extends Command
{
    protected function execute(InputInterface $input, OutputInterface $output): int
    {
        $cron1 = new Process(['bin/console', 'cache:pool:prune']);
        $cron2 = new Process(['bin/console', 'something:else']);
        // etc.
    }
}

Now if you run the cronjob command itself with dynamic configuration like so: php -d memory_limit=256M bin/console run-cronjobs, the subprocesses (cache:pool:prune etc.) will not be limited to 256M because they themselves will start a completely new PHP process which will take the default PHP settings again.
This affects everything, not just memory_limit but also runtime, extensions etc. You name it.

Oftentimes, however, that's not what you want. You'd like to have the children have the same restrictions as your parent process.

With this new handler, all you have to do is to use PhpSubprocess instead of Process and it will create a temporary ini file taking over all the settings of the parent process. Done 😊

The current design is pretty straightforward without any extension points just yet (no way to fetch the ini files separately etc.). Also, it could maybe do with some more tests for the ini parsing but at this stage I would like to ask if this would even be a welcomed addition to Symfony or not 😊

src/Symfony/Component/Process/Tests/OutputMemoryLimitProcess.php

src/Symfony/Component/Process/CHANGELOG.md

Seldaek · 2022-12-05T20:20:43Z

Pinging @johnstevenson as he authored most of the xdebug-handler code. Feel free to ignore if not interested though :)

src/Symfony/Component/Process/PhpSubprocess.php

src/Symfony/Component/Process/CHANGELOG.md

OskarStark · 2022-12-06T13:27:06Z

For the record:

Fabien documented how to write CHANGELOG.md files here: symfony/symfony-docs#14830

johnstevenson · 2022-12-06T14:23:35Z

I guess this will work as well as composer/xdebug-handler does (which seems to be pretty well) with its limitation of not being able to detect extensions loaded on the command line.

Proper attribution for the writeTmpIni and mergeLoadedConfig methods would be useful, in my opinion.

Toflar · 2022-12-06T14:34:00Z

Proper attribution for the writeTmpIni and mergeLoadedConfig methods would be useful, in my opinion.

Sure, would you mind adding a code review/suggestion as to how you'd like to have this being done? Might also just add yourself as an @author?

nicolas-grekas

I think this can be useful. Here are some comments.

src/Symfony/Component/Process/PhpSubprocess.php

src/Symfony/Component/Process/CHANGELOG.md

src/Symfony/Component/Process/PhpSubprocess.php

src/Symfony/Component/Process/Tests/OutputMemoryLimitProcess.php

src/Symfony/Component/Process/Tests/PhpSubprocessTest.php

Toflar

@nicolas-grekas everything addressed :)

src/Symfony/Component/Process/CHANGELOG.md

src/Symfony/Component/Process/PhpSubprocess.php

nicolas-grekas

some minor things and good to me

src/Symfony/Component/Process/PhpSubprocess.php

src/Symfony/Component/Process/CHANGELOG.md

src/Symfony/Component/Process/PhpSubprocess.php

fabpot

LGTM but @Toflar, you need to rebase to remove the merge commit.

Toflar · 2023-08-03T07:38:57Z

Rebased and addressed the remaining reviews.

nicolas-grekas · 2023-08-03T09:14:54Z

Thank you @Toflar.

xabbuh · 2023-08-06T16:55:11Z

Do we really need this new class? The failing tests suggest otherwise.

nicolas-grekas · 2023-08-06T20:11:35Z

We want this feature yes. The tests are with sigchild enabled. Something to investigate. Same for the open_basedir-related failures.

…cutable (GromNaN) This PR was merged into the 6.4 branch. Discussion ---------- [Process] Fix OutputMemoryLimitProcess.php cannot be executable | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | - | License | MIT PHP files cannot be executable without shebang. File introduced by #48485 Note that there is no other executable .php file. Commits ------- 09992b7 PHP files cannot be executable without shebang

Description ----------- Unfortunately, our `ProcessUtil` service currently cannot be used with custom flags such as `-d memory_limit=-1`. You currently have to do something like this: ```php $process = $this->processUtil->createSymfonyConsoleProcess('...'); $reflection = new \ReflectionClass($process); $commandline = $reflection->getProperty('commandline'); $commandline = $commandline->getValue($process); array_splice($commandline, 1, 0, ['-d', 'memory_limit=-1']); $process = new Process($commandline); return $this->processUtil->createPromise($process); ``` First, I thought we could add a new `createSymfonyConsoleProcessWithAdditonalDirectives()` but then, as soon as you want to use something else than `new Process()` (e.g. my new [`new PhpSubprocess()`](symfony/symfony#48485)) then this wouldn't work either. So I thought it would be best to expose the console path and binary path as getters. Commits ------- 7e3ba37 Allow to re-use the ProcessUtil data

Description ----------- Unfortunately, our `ProcessUtil` service currently cannot be used with custom flags such as `-d memory_limit=-1`. You currently have to do something like this: ```php $process = $this->processUtil->createSymfonyConsoleProcess('...'); $reflection = new \ReflectionClass($process); $commandline = $reflection->getProperty('commandline'); $commandline = $commandline->getValue($process); array_splice($commandline, 1, 0, ['-d', 'memory_limit=-1']); $process = new Process($commandline); return $this->processUtil->createPromise($process); ``` First, I thought we could add a new `createSymfonyConsoleProcessWithAdditonalDirectives()` but then, as soon as you want to use something else than `new Process()` (e.g. my new [`new PhpSubprocess()`](symfony/symfony#48485)) then this wouldn't work either. So I thought it would be best to expose the console path and binary path as getters. Commits ------- 7e3ba373 Allow to re-use the ProcessUtil data

carsonbot added Status: Needs Review Feature labels Dec 5, 2022

carsonbot added this to the 6.3 milestone Dec 5, 2022

Toflar commented Dec 5, 2022

View reviewed changes

src/Symfony/Component/Process/Tests/OutputMemoryLimitProcess.php Outdated Show resolved Hide resolved

OskarStark reviewed Dec 5, 2022

View reviewed changes

src/Symfony/Component/Process/CHANGELOG.md Outdated Show resolved Hide resolved

OskarStark added the Process label Dec 5, 2022

carsonbot changed the title ~~Introducing a new PhpSubprocess handler~~ [Process] Introducing a new PhpSubprocess handler Dec 5, 2022

OskarStark changed the title ~~[Process] Introducing a new PhpSubprocess handler~~ [Process] Introducing a new PhpSubprocess handler Dec 5, 2022

OskarStark reviewed Dec 6, 2022

View reviewed changes

src/Symfony/Component/Process/PhpSubprocess.php Outdated Show resolved Hide resolved

src/Symfony/Component/Process/CHANGELOG.md Outdated Show resolved Hide resolved

nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023

nicolas-grekas reviewed Aug 1, 2023

View reviewed changes

Toflar commented Aug 2, 2023

View reviewed changes

stof reviewed Aug 2, 2023

View reviewed changes

src/Symfony/Component/Process/CHANGELOG.md Show resolved Hide resolved

src/Symfony/Component/Process/PhpSubprocess.php Outdated Show resolved Hide resolved

nicolas-grekas reviewed Aug 2, 2023

View reviewed changes

nicolas-grekas approved these changes Aug 2, 2023

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Aug 2, 2023

fabpot requested changes Aug 2, 2023

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Aug 2, 2023

Toflar force-pushed the feature/php-subprocess branch 3 times, most recently from b89710f to 9364391 Compare August 3, 2023 07:36

Introducing a new PhpSubprocess handler

f682a29

Toflar force-pushed the feature/php-subprocess branch from 9364391 to f682a29 Compare August 3, 2023 07:38

nicolas-grekas mentioned this pull request Aug 3, 2023

[Process] Introducing a new PhpSubprocess handler symfony/symfony-docs#18680

Closed

nicolas-grekas merged commit 0ba946a into symfony:6.4 Aug 3, 2023

Toflar deleted the feature/php-subprocess branch August 3, 2023 09:16

This was referenced Oct 21, 2023

Release v6.4.0-BETA1 #52220

Merged

Release v7.0.0-BETA1 #52224

Merged

Toflar mentioned this pull request Oct 25, 2023

Allow to re-use the ProcessUtil data contao/contao#6465

Merged

GromNaN mentioned this pull request Nov 4, 2023

[Process] Fix OutputMemoryLimitProcess.php cannot be executable #52455

Merged

Uh oh!

[Process] Introducing a new PhpSubprocess handler #48485

[Process] Introducing a new PhpSubprocess handler #48485

Uh oh!

Conversation

Toflar commented Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Seldaek commented Dec 5, 2022

Uh oh!

Uh oh!

Uh oh!

OskarStark commented Dec 6, 2022

Uh oh!

johnstevenson commented Dec 6, 2022

Uh oh!

Toflar commented Dec 6, 2022

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Toflar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fabpot left a comment

Choose a reason for hiding this comment

Uh oh!

Toflar commented Aug 3, 2023

Uh oh!

nicolas-grekas commented Aug 3, 2023

Uh oh!

xabbuh commented Aug 6, 2023

Uh oh!

nicolas-grekas commented Aug 6, 2023

Uh oh!

Uh oh!

[Process] Introducing a new `PhpSubprocess` handler #48485

[Process] Introducing a new `PhpSubprocess` handler #48485

Toflar commented Dec 5, 2022 •

edited

Loading