@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.

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!

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.

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.