Skip to content

Conversation

TimWolla
Copy link
Member

@TimWolla TimWolla commented Aug 27, 2025

@TimWolla TimWolla force-pushed the argc-argv-from-query branch from 6900d7e to c1f22ff Compare August 27, 2025 12:59
@@ -886,6 +891,7 @@ static bool php_auto_globals_create_server(zend_string *name)
zend_hash_update(Z_ARRVAL(PG(http_globals)[TRACK_VARS_SERVER]), ZSTR_KNOWN(ZEND_STR_ARGC), argc);
}
} else {
zend_error(E_DEPRECATED, "Deriving $_SERVER['argc'] and $_SERVER['argv'] from $_SERVER['QUERY_STRING'] is deprecated, configure register_argc_argv=0 to suppress this message and access the query parameters via $_SERVER['QUERY_STRING'] or $_GET");
Copy link
Member

Choose a reason for hiding this comment

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

This is a nit, but this should be two sentences:

Deriving $_SERVER['argc'] and $_SERVER['argv'] from $_SERVER['QUERY_STRING'] is deprecated. Configure register_argc_argv=0 to suppress this message and access the query parameters via $_SERVER['QUERY_STRING'] or $_GET

(ie, a deprecated. Configure instead of a deprecated, configure)

}
} else if (PG(register_argc_argv)) {
zend_error(E_DEPRECATED, "Deriving $_SERVER['argc'] and $_SERVER['argv'] from $_SERVER['QUERY_STRING'] is deprecated, configure register_argc_argv=0 to suppress this message and access the query parameters via $_SERVER['QUERY_STRING'] or $_GET");
Copy link
Contributor

Choose a reason for hiding this comment

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

Trying to make the message shorter while still accurate.
Accessing the query parameters might be a bad suggestion (as that might reintroduce the security issue)

Suggested change
zend_error(E_DEPRECATED, "Deriving $_SERVER['argc'] and $_SERVER['argv'] from $_SERVER['QUERY_STRING'] is deprecated, configure register_argc_argv=0 to suppress this message and access the query parameters via $_SERVER['QUERY_STRING'] or $_GET");
zend_error(E_DEPRECATED, "Deriving $_SERVER['argv'] from the query string is deprecated, configure register_argc_argv=0 to turn this off.");

Copy link
Member

Choose a reason for hiding this comment

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

I would go with thisshortened variant, but also split it up in two sentences as I originally suggested, and maybe "this" -> "this message" ?

zend_error(E_DEPRECATED, "Deriving $_SERVER['argv'] from the query string is deprecated. Configure register_argc_argv=0 to turn this message off");

(make sure not to include the . at the end!)

Copy link
Contributor

Choose a reason for hiding this comment

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

"this message", but also this deriving from behavior; that's why I didn't add message in my suggestion (but I understand it makes things a bit more implicit, while accurate :)

@TimWolla TimWolla marked this pull request as ready for review September 2, 2025 08:46
@TimWolla TimWolla requested a review from bukka as a code owner September 2, 2025 08:46
@TimWolla TimWolla requested a review from Girgias September 2, 2025 08:46
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

The implementation looks OK. I will let others judge the deprecation message content.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants