-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix visibility of whitespace in config output #18527
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
When a config var has whitespace (especially trailing whitespace) it is hard to see. This commit wraps the values (if they exist) in backticks so the difference is visually observable: Before: ``` $ export PHP_INI_SCAN_DIR="/opt/homebrew/etc/php/8.4/conf.d " $ ./sapi/cli/php --ini Configuration File (php.ini) Path: /usr/local/lib Loaded Configuration File: /opt/homebrew/etc/php/8.4/conf.d Scan for additional .ini files in: (none) Additional .ini files parsed: (none) ``` > Note > The above output has trailing whitespace that is not visible, you can see it if you copy it into an editor: After: ``` $ ./sapi/cli/php --ini Configuration File (php.ini) Path: `/usr/local/lib` Loaded Configuration File: `/opt/homebrew/etc/php/8.4/conf.d ` Scan for additional .ini files in: (none) Additional .ini files parsed: (none) ``` Above the whitespace is visible `/opt/homebrew/etc/php/8.4/conf.d `. Close php#18390
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR. I think I would prefer double quotes as the indicator, as this would make the entire value copy and pasteable into a terminal without causing issues, i.e. I can just use the value and run vim "whatever I copied" to view a config file.
From a suggested comment php#18527 (review). Before: ``` $ ./sapi/cli/php --ini Configuration File (php.ini) Path: `/usr/local/lib` Loaded Configuration File: `/opt/homebrew/etc/php/8.4/conf.d ` Scan for additional .ini files in: (none) Additional .ini files parsed: (none) ``` After: ``` $ ./sapi/cli/php --ini Configuration File (php.ini) Path: "/usr/local/lib" Loaded Configuration File: "/opt/homebrew/etc/php/8.4/conf.d " Scan for additional .ini files in: (none) Additional .ini files parsed: (none) ```
@TimWolla seems good. I initially picked a backtick due to code literal format for markdown like this but I also agree that it's helpful to have a quote right there, especially for cases like "/dirs/like this that have spaces/in them" where copying and pasting the dir into a shell (a likely use case) would fail and the different segments might be treated as multiple arguments.
I'll generally defer character choice as a style decision for primary project maintainers/contributors. Updated with a new commit and description to match.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually use double quotes so this makes more sense than backticks.
Thank you!
I wonder if the string inside the quotes should've gotten escaped
I wonder if the string inside the quotes should've gotten escaped
Might be sensible, but isn't that a bit of a pre-existing issue?
I wonder if the string inside the quotes should've gotten escaped
Might be sensible, but isn't that a bit of a pre-existing issue?
I agree it's pre-existing. I realized it just now because we now explicitly add quotes around it 🙂 I can look for making a patch if this sounds good.
I wonder if the string inside the quotes should've gotten escaped
Might be sensible, but isn't that a bit of a pre-existing issue?
I agree it's pre-existing. I realized it just now because we now explicitly add quotes around it 🙂 I can look for making a patch if this sounds good.
Yeah it makes sense to me to escape it :)
Thanks, all, for jumping on this quickly. Appreciate everything y'all do!
In php#18527, I accidentally swapped the values. This is before my modification: ``` zend_printf("Configuration File (php.ini) Path: %s\n", PHP_CONFIG_FILE_PATH); zend_printf("Loaded Configuration File: %s\n", php_ini_opened_path ? php_ini_opened_path : "(none)"); zend_printf("Scan for additional .ini files in: %s\n", php_ini_scanned_path ? php_ini_scanned_path : "(none)"); ``` - "Loaded Configuration File" should be `php_ini_opened_path` - "Scan for additional .ini files in" shoudl be `php_ini_scanned_path`
This PR introduces a bug: php_ini_scanned_path and php_ini_opened_path are now swapped in the code.
It also doesn't add the same quotation marks to the output of php -i, so the behavior between that and php --ini is now inconsistent in this regard.
But most importantly, it causes a significant regression: there are a lot of projects out there that parse the output of php --ini, and suddenly having quotation marks will break them (a GitHub code search for "php --ini" grep or "php --ini" sed or "php --ini" awk gives slightly more insightful results than ORing the three in a single query, as the search then shows code first that has several different hits).
For example: https://github.com/shivammathur/setup-php/blob/cf4cade2721270509d5b1c766ab3549210a39a2a/src/scripts/unix.sh#L246
IMO this should be reverted, @nielsdos / @Girgias, until the BC impact has been thoroughly assessed.
In #18527, I accidentally swapped the values. This is before my modification: ``` zend_printf("Configuration File (php.ini) Path: %s\n", PHP_CONFIG_FILE_PATH); zend_printf("Loaded Configuration File: %s\n", php_ini_opened_path ? php_ini_opened_path : "(none)"); zend_printf("Scan for additional .ini files in: %s\n", php_ini_scanned_path ? php_ini_scanned_path : "(none)"); ``` - "Loaded Configuration File" should be `php_ini_opened_path` - "Scan for additional .ini files in" shoudl be `php_ini_scanned_path`
IMO this should be reverted, @nielsdos / @Girgias, until the BC impact has been thoroughly assessed.
Thankfully, this example works out of the box. It's a good thing @TimWolla advocated for quotes!
$ ./sapi/cli/php --ini
Configuration File (php.ini) Path: "/usr/local/lib"
Loaded Configuration File: (none)
Scan for additional .ini files in: (none)
Additional .ini files parsed: (none)
$ cut -d '"' -f 2 < <(./sapi/cli/php --ini | grep '(php.ini)' | sed -e "s|.*: s*||")
/usr/local/lib
I agree with @dzuelke that perhaps the impact area is larger than previously realized. I think, at minimum, adding a NEWS entry and mentioning that it is changing is worth doing. I'm happy to send a PR, but feel like I'm accidentally racking up commits for my mistake already. @dzuelke did a good job of finding that other bug, I would like him to have the commit. Here's a (perhaps overly verbose) suggestion:
$ git diff diff --git a/NEWS b/NEWS index 706533b9c8..c63b8d32c3 100644 --- a/NEWS +++ b/NEWS @@ -14,6 +14,10 @@ PHP NEWS . Drop support for -z CLI/CGI flag. (nielsdos) . Fixed GH-17956 - development server 404 page does not adapt to mobiles. (pascalchevrel) + . Change `php --ini` output to now wrap values in double quotes to better + show hidden whitespace. Scripts that rely on parsing this information + can use the `cut` to remove the quotes. For example https://github.com/shivammathur/setup-php/blob/cf4cade2721270509d5b1c766ab3549210a39a2a/src/scripts/unix.sh#L246 + (schneems) - CURL: . Added CURLFOLLOW_ALL, CURLFOLLOW_OBEYCODE and CURLFOLLOW_FIRSTONLY
Moving forward, this suggests that perhaps a more stable, machine-readable CLI flag (or command) for such script authors would be good for stability. For example, something like php --ini=json that authors could then parse via jq (which is a pretty common system dependency at this point). This could be a compliment to the --ini=diff just added in 8.5 (thanks @TimWolla, yet again).
@schneems there is another option that I think solves both your original desire to highlight such a hard to spot error, and preserves backward compatibility: only quote the value if the TTY is a terminal.
That way, no regression is caused for any piping use case, but a human inspecting the output gets the quoting. This is similar to how many programs that output ANSI colors disable them once stdout is piped, so a perfectly common approach.
You can check for it like so (stdout is always FD number 1):
#ifdef PHP_WIN32 /* Check if the Windows standard handle is redirected to file */ stdout_is_tty = php_win32_console_fileno_is_console(1); #elif HAVE_UNISTD_H /* Check if the file descriptor identifier is a terminal */ stdout_is_tty = isatty(1); #endif
And then use php_escape_shell_arg() on the string, it handles multibyte characters etc correctly and wraps everything in single quotes with correct escaping.
As I mentioned before, this quoting should maybe also be done for php -i (that code lives in ext/standard/info.c's php_print_info()) since a lot of people use that as well, and there might be a few other "locations of interest" in there - extension_dir, open_basedir, sys_temp_dir, upload_tmp_dir, user_dir come to mind.
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: ``` $ ./sapi/cli/php --ini Configuration File (php.ini) Path: "/usr/local/lib" Loaded Configuration File: (none) Scan for additional .ini files in: (none) Additional .ini files parsed: (none) ``` Output is not a tty: ``` $ ./sapi/cli/php --ini Configuration File (php.ini) Path: /usr/local/lib Loaded Configuration File: (none) Scan for additional .ini files in: (none) Additional .ini files parsed: (none) ```
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: ``` $ ./sapi/cli/php --ini Configuration File (php.ini) Path: "/usr/local/lib" Loaded Configuration File: (none) Scan for additional .ini files in: (none) Additional .ini files parsed: (none) ``` Output is not a tty: ``` $ ./sapi/cli/php --ini Configuration File (php.ini) Path: /usr/local/lib Loaded Configuration File: (none) Scan for additional .ini files in: (none) Additional .ini files parsed: (none) ```
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) ```
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) ```
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) ```
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) ```
Uh oh!
There was an error while loading. Please reload this page.
When a config var has whitespace (especially trailing whitespace) it is hard to see. This commit wraps the values (if they exist) in a character so the difference is visually observable:
Before:
Note
The above output has trailing whitespace that is not visible, you can see it if you copy it into an editor.
After:
Above the whitespace is now visible
/opt/homebrew/etc/php/8.4/conf.d.Close #18390