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

Allow to re-use the ProcessUtil data #6465

Merged
merged 1 commit into from Nov 12, 2023

Conversation

Toflar
Copy link
Member

@Toflar Toflar commented Oct 25, 2023

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:

$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()) then this wouldn't work either.

So I thought it would be best to expose the console path and binary path as getters.

@aschempp
Copy link
Member

how would the console path getter help in setting console arguments?

@Toflar
Copy link
Member Author

Toflar commented Oct 25, 2023

The example above would now be

$process = new Process([
    $this->processUtil->getPhpBinary(),
    '-d',
    'memory_limit=-1',
    $this->processUtil->getConsolePath(),
    ...
);

return $this->processUtil->createPromise($process);

@aschempp
Copy link
Member

so you would get the console path from the class instead of injecting the container configuration? 🤔

@Toflar
Copy link
Member Author

Toflar commented Oct 25, 2023

Yes, because that's not the only thing I need.

@leofeyer leofeyer added this to the 5.3 milestone Oct 26, 2023
@Toflar Toflar requested a review from a team November 9, 2023 12:19
@leofeyer leofeyer merged commit 0ed9915 into contao:5.x Nov 12, 2023
17 checks passed
@leofeyer
Copy link
Member

Thank you @Toflar.

@Toflar Toflar deleted the feature/more-flexible-process-util branch November 13, 2023 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants