-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Only quote php --ini
values when out is a tty
#18583
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
Conversation
A comment in php#18527 mentioned that many scripts use `php—-ini` to bootstrap information about the system. Searching on GitHub confirms that many places will not work with quotes out of the box: https://github.com/search?q=content%3A%22php%20--ini%22&type=code. Many seem to be variants on this pattern ``` $ echo "extension=mongodb.so" >> `php --ini | grep "Loaded Configuration" | sed -e "s|.*:\s*||"` ``` To preserve these existing scripts, we can detect when the output is a TTY to optionally quote them. This is the new behavior: Output is a tty: ``` $ make -j$(nproc) &> /dev/null && env PHP_INI_SCAN_DIR="/opt/homebrew/etc/php/8.4/conf.d " PHPRC="/opt/homebrew/etc/php/8.4" ./sapi/cli/php --ini Configuration File (php.ini) Path: "/usr/local/lib" Loaded Configuration File: "/opt/homebrew/etc/php/8.4/php.ini" Scan for additional .ini files in: "/opt/homebrew/etc/php/8.4/conf.d " Additional .ini files parsed: (none) ``` Output is not a tty: ``` $ make -j$(nproc) &> /dev/null && env PHP_INI_SCAN_DIR="/opt/homebrew/etc/php/8.4/conf.d " PHPRC="/opt/homebrew/etc/php/8.4" ./sapi/cli/php --ini | tee Configuration File (php.ini) Path: /usr/local/lib Loaded Configuration File: /opt/homebrew/etc/php/8.4/php.ini Scan for additional .ini files in: /opt/homebrew/etc/php/8.4/conf.d Additional .ini files parsed: (none) ```
26deb0c
to
58d8d94
Compare
Context: When reviewing BC issues as part of my 8.5. RM duties I came across #18527 and the follow ups.
As this change breaks existing scripts we should document it and give people alternatives, or revert it. I will sync with @DanielEScherzer on that and gather some feedback.
The introduced inconsistentcy between php --ini
, php -i
, php-config --ini
and php -r 'echo php_ini_loaded_file();'
isn't great either.
And while this PR would "fix" the BC issue, I'm against changing the output depending on tty/no-tty as that's another source of non-obvious behavior and confusion.
Yeah, I'm also not a big fan of having different output for TTY and not - when I build up a set of piped commands I inspect the output from one and then write the next, so having a different output when I run the command directly and when the output is piped somewhere would be confusing
We can tighten the search a bit to skip php --ini=...
, forks, archived repos, and documentation pages:
content:/php --ini(\s|$|['"`])/ -is:fork -is:archived -path:.md -path:.rst -path:.html -path:.txt
only 1.5k hits now
Would it be a break to add something like php --ini --no-quotes
where another option controls the quoting?
I'm against changing the output depending on tty/no-tty as that's another source of non-obvious behavior and confusion.
That's fair. I agree it's a bit mind-bendy to introduce a bug in a script that might only exist while you're not looking at it.
Would it be a break to add something like php --ini --no-quotes where another option controls the quoting?
I think the best long-term option is something like this rubygems/rubygems#8728 where the output is in an explicitly machine readable format (--format=json
in that case). So then anyone doing scripting has a stable interface and the maintainers of the CLI code don't have to guess based on other ancillary signals (like TTY or not). It depends on the existence of a tool like jq
to parse but that's fairly common at this point (my opinion).
But that's a much bigger lift to introduce a new JSON output and associated formatting flag. I'm not confident I could make an acceptable patch to get a feature like that over the line (C is not my day-job lang).
(brainstorming) If there's interest there we could put out a "help wanted" call. In the mean time if there's some desire to allow for a switch of this behavior we could introduce --format=noquote
(or some other, better value) and while we wait for an eventual --format=json
Thank you for the quick reply, especially considering the long waiting time. Much appreciated.
Regarding the whole feature: After reaching out to a couple more people nobody else seemed all that concerned with the BC implications of this change. Given that, I'm not going to argue for removing this from 8.5. If anything we'll document it in UPGRADING and suggest a better way of getting that data.
Adding --format=json
or maybe --ini=json
is something others also suggested as a stable computational API outside of php -r 'echo php_ini_loaded_file();'
and `php -r 'echo ini_get("extension_dir");'. This addition likely doesn't even require an RFC. I'm happy to help with that as well, should be rather simple as json_encode is always available since PHP 8.0.
So if nobody objects I'd close this one.
Closing in favor of #19655
A comment in #18527 mentioned that many scripts use
php—-ini
to bootstrap information about the system. Searching on GitHub confirms that many places will not work with quotes out of the box: https://github.com/search?q=content%3A%22php%20--ini%22&type=code.Many seem to be variants on this pattern
To preserve these existing scripts, we can detect when the output is a TTY to optionally quote them. This is the new behavior:
Output is a tty:
Output is not a tty: