Fix GH-20279: register_argc_argv set at 0 on sapi cli after deprecation. #20280
Conversation
| } | ||
| } else if (PG(register_argc_argv)) { | ||
| zend_error(E_DEPRECATED, "Deriving $_SERVER['argv'] from the query string is deprecated. Configure register_argc_argv=0 to turn this message off"); | ||
| if (strcmp(sapi_module.name, "cli") && strcmp(sapi_module.name, "cli-server")) { |
There was a problem hiding this comment.
This is an impossible branch. It's also dead code for the CLI.
| "register_argc_argv=1\n"; | ||
|
|
||
| static const char SERVER_HARDCODED_INI[] = | ||
| "register_argc_argv=1\n"; |
There was a problem hiding this comment.
The CLI server is not the CLI.
There was a problem hiding this comment.
I was confused by this while keeping it hardcoded to On for CLI-related ones but the commit conveys a different message sapi: Remove hardcoded `register_argc_argv` for CLI SAPIs
There was a problem hiding this comment.
The CLI server should not be treated as a CLI SAPI, since its purpose is HTTP requests. The CLI SAPIs are cli and phpdbg.
I was confused by this while keeping it hardcoded to On for CLI-related ones but the commit conveys a different message sapi: Remove hardcoded
register_argc_argvfor CLI SAPIs
Yes, the implementation does not actually match the RFC to the letter, but it matches it in intent. What the implementation does, is ignoring the INI setting when SG(request_info).arg is given, since that will match the actual argv given to the process and thus is trustworthy. This is only set for the CLI SAPIs.
So the RFC said "keeping it hardcoded to 1" and the implementation does "ignore the INI", which results in the same outcome, but a simpler and more predictable implementation. This is the relevant commit: 6ea0eb9
/cc @nicolas-grekas
| "max_execution_time=0\n" | ||
| "max_input_time=-1\n"; | ||
| "max_input_time=-1\n" | ||
| "register_argc_argv=1\n"; |
There was a problem hiding this comment.
This was intentionally removed, since the implementation just ignores the INI for the CLI entirely. In practice this will not hardcode the INI anyways, since the -d flag still takes precedence.
|
so in other words, there is no bug here ? |
I'd say there is no bug here. The implementation was approved by the RFC author and 4 different core developers and was also extensively discussed in Slack. While we could revert the changes to the C files from 026d470, I don't see much value there, since the value has no effect either and and since |
|
oky doky thanks for the clarifications ! |
2 participants
No description provided.