fix: Correctly inherit the Composer restarted process settings #995
Conversation
> [!WARNING] > Note: this PR is more of draft. I am not confident I understand fully the issue neither that this is the right fix (and I could not test it). Do NOT assume this code just works. After a lot of digging in box-project/box#988, @maartendekeizer could identify the root of the issue. My understanding is that Flex tries to detect if the current PHP process was restarted by Composer and forwards its restarted settings to the sub-processes it is going to launch. There is currently two things done: - symfony#91 which if took code from SensioDistributionBundle. - symfony#899 which kind of followed the suite. I suspect the mentioned code predates `composer/xdebug-handler`. Now with this package, there is two things to take into account: - The `composer/xdebug-handler` API is likely much safer, more robust and a lot less hacky to use. - There is other applications that can restart a PHP process. As an example Box restarts the PHP process to be able to correct the `phar.readonly` setting that cannot be changed at runtime. It matters as the restarted process by be executing a Composer command. As mentioned in the warning, I could not test this, not even run the tests locally, so I would be careful about this PR. I just wanted to give a base about a potential fix with context about the original issue encountered. On my side to not have to wait on Flex and avoid the users to have to update it to have the fix, Box launches its Composer commands with `COMPOSER_ORIGINAL_INIS=''` to avoid Flex to trigger this bad piece of code.
| $arguments[] = '--php-ini='.$ini; | ||
| } | ||
|
|
||
| if ($memoryLimit = (string) getenv('COMPOSER_MEMORY_LIMIT')) { |
There was a problem hiding this comment.
I think we still needs this if block, the env value is added as argument to the processes which is not done via PhpConfig
There was a problem hiding this comment.
but the memory limit has been configured and dumped into the tmp ini file, so with persistent settings those changes done to the restarted PHP process should be preserved (I think)
There was a problem hiding this comment.
Nope, XdebugHandler dumped the ini value not the runtime value.
It is not possible to set the PHP memory limit via envs (beside when configured in php.ini). So custom code like this is required.
This code makes it also possible to have another memory limit for a subprocess like Composer then the main processes.
But you can debate if it is correct that this env will be passed to a subprocesses. If you give Composer a limit of 1Gb, you will give the main processes and each subprocess 1Gb, so it is possible that the whole run takes 2Gb.
There was a problem hiding this comment.
I do not understand what you mean. If you executed composer with a specific memory limit, Composer will have restarted with persistent settings where the current memory limit (and the one that XdebugHandler will use) is that one.
But I admit I think it deserves to be properly checked rather than merged blindly. It needs a fix, but we should make sure this change is the right fix.
|
A side note about the original code. The By exploding this string and take the first item (in this example |
- Use the persistent setting: any sub-process should use the same settings. - Reset `COMPOSER_ORIGINAL_INIS` to avoid the buggy Flex code (see symfony/flex#995). Closes box-project#1089, box-project#988.
- Use the persistent setting: any sub-process should use the same settings. - Reset `COMPOSER_ORIGINAL_INIS` to avoid the buggy Flex code (see symfony/flex#995). Closes #1089, #988.
|
Please let us know when you're more confident with the proposed changes. |
|
Pew I have no idea. Part of the issue is, I'm not sure how you test this. How have you tested it so far? |
|
Closing as actually this is working as intended. Flex wants to execute a PHP Process as if it was the original command, not the restarted process (if restarted) with modified settings. So if Composer disables Xdebug, the process that flex starts should have Xdebug on. The current code could be refactored to use |
Warning
Note: this PR is more of draft. I am not confident I understand fully
the issue neither that this is the right fix (and I could not test it). Do NOT assume this code just works.
After a lot of digging in box-project/box#988, @maartendekeizer could identify the root of the issue.
My understanding is that Flex tries to detect if the current PHP process was restarted by Composer and forwards its restarted settings to the sub-processes it is going to launch. There is currently two things done:
Check for COMPOSER_ORIGINAL_INIS env otherwise use regular php ini #91 which if took code from SensioDistributionBundle.
Add COMPOSER_MEMORY_LIMIT support to Symfony\Flex\ScriptExecutor #899 which kind of followed the suite.
I suspect the mentioned code predates
composer/xdebug-handler. Nowwith this package, there is two things to take into account:
The
composer/xdebug-handlerAPI is likely much safer, more robust and a lot less hacky to use.There is other applications that can restart a PHP process. As an example Box restarts the PHP process to be able to correct the
phar.readonlysetting that cannot be changed at runtime. It matters as the restarted process by be executing a Composer command.As mentioned in the warning, I could not test this, not even run the tests locally, so I would be careful about this PR. I just wanted to give a base about a potential fix with context about the original issue encountered.
On my side to not have to wait on Flex and avoid the users to have to update it to have the fix, Box launches its Composer commands with
COMPOSER_ORIGINAL_INIS=''to avoid Flex to trigger the problematic piece of code.