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

Pass the path to a php.ini file to a child process #412

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

werdender
Copy link

Q A
Is bugfix? yes
New feature? no
Breaks BC? no
Tests pass? yes
Fixed issues

Description:
The problem is the child process ignores path to a php.ini file which was defined in supervisor tools like systemd. For example, if you have configured the pdo_mysql.default_socket parameter (which must be different for different sites) in an ini file, and want to run the queue in systemd unit file like this:
ExecStart=/usr/bin/php -c /var/www/somesite/php.ini /var/www/somesite/current/yii queue/listen
the passed -c parameter will be ignored by a child process and it will b still try using xx/cli/php.ini file and the wrong socket.

So, I've added the -c parameter to the child process.

Copy link
Member

@samdark samdark left a comment

Choose a reason for hiding this comment

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

Good change 👍 but adjustment is needed.

Also need a line for CHANGELOG.md.

src/cli/Command.php Outdated Show resolved Hide resolved
@samdark samdark added the status:under development Someone is working on a pull request. label Mar 26, 2021
@rob006
Copy link

rob006 commented Mar 26, 2021

IMO this should be optional (it is not needed in 99.999% cases and produces noise in processes list) and configurable in more flexible way, like phpArguments property that allows you to configure any arguments passed to phpBinary.

@werdender werdender requested a review from samdark March 28, 2021 13:34
@samdark samdark added status:code review The pull request needs review. and removed status:under development Someone is working on a pull request. labels Apr 6, 2021
@samdark
Copy link
Member

samdark commented Apr 6, 2021

What @rob006 suggests makes sense. Let's make it optional. Otherwise, it's good.

@samdark samdark added status:under development Someone is working on a pull request. and removed status:code review The pull request needs review. labels Apr 6, 2021
@werdender
Copy link
Author

To be honest I don't think we need take care about process list, just look to the another processes - nobody is shy to pass arguments.

Also I believe it's a little weird to ask "should I use the .ini file you provided?". Of course it should.

One more thing: the goal is run application to an another server (reserve) without any changes in its configuration files. I can do it by changes in the .ini file, but it falls apart if we make it configurable. Looks like the passing arguments to the phpBinary even is not documented?

@rob006
Copy link

rob006 commented Apr 11, 2021

Also I believe it's a little weird to ask "should I use the .ini file you provided?". Of course it should.

This is not how your PR works. You could say that if you would set -c option for exec command only if it was explicitly passed to worker command, but you're setting it always, even if it is not needed nor requested.

Also you're so fixated about your own edge, that you're forgetting that -c is not he only option that may need similar behavior. With phpArguments you can handle all possible edge cases, without risk of BC break (you've made some assumptions, that may not always be true: PHP binary used by exec command is configurable, so user can use different binary for exec command and silently overwriting php.ini file in this way is just wrong).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:under development Someone is working on a pull request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants