Skip to content
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

sapi/cli: Extend --ini to print INI settings changed from the builtin default #17459

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TimWolla
Copy link
Member

This is intended to make it easier to check whether or not a given INI setting is changed from the default when building reproducers for a bugreport, without forgetting any that might be relevant to the report.

As an example, running sapi/cli/php -c /etc/php/8.3/cli/ --ini on my Ubuntu will now output:

Configuration File (php.ini) Path: /usr/local/lib
Loaded Configuration File:         /etc/php/8.3/cli/php.ini
Scan for additional .ini files in: (none)
Additional .ini files parsed:      (none)

Non-standard INI settings:
allow_url_include: "0" -> ""
auto_append_file: (none) -> ""
auto_prepend_file: (none) -> ""
display_errors: "1" -> ""
display_startup_errors: "1" -> ""
enable_dl: "1" -> ""
error_reporting: (none) -> "22527"
html_errors: "1" -> "0"
ignore_repeated_errors: "0" -> ""
ignore_repeated_source: "0" -> ""
implicit_flush: "0" -> "1"
log_errors: "0" -> "1"
mail.add_x_header: "0" -> ""
mail.mixed_lf_and_crlf: "0" -> ""
max_execution_time: "30" -> "0"
memory_limit: "128M" -> "-1"
request_order: (none) -> "GP"
session.cookie_httponly: "0" -> ""
session.gc_divisor: "100" -> "1000"
session.gc_probability: "1" -> "0"
session.sid_bits_per_character: "4" -> "5"
session.sid_length: "32" -> "26"
short_open_tag: "1" -> ""
unserialize_callback_func: (none) -> ""
user_dir: (none) -> ""
variables_order: "EGPCS" -> "GPCS"
zend.assertions: "1" -> "-1"
zend.exception_ignore_args: "0" -> "1"
zend.exception_string_param_max_len: "15" -> "0"

…in default

This is intended to make it easier to check whether or not a given INI setting
is changed from the default when building reproducers for a bugreport, without
forgetting any that might be relevant to the report.

As an example, running `sapi/cli/php -c /etc/php/8.3/cli/ --ini` on my Ubuntu
will now output:

    Configuration File (php.ini) Path: /usr/local/lib
    Loaded Configuration File:         /etc/php/8.3/cli/php.ini
    Scan for additional .ini files in: (none)
    Additional .ini files parsed:      (none)

    Non-standard INI settings:
    allow_url_include: "0" -> ""
    auto_append_file: (none) -> ""
    auto_prepend_file: (none) -> ""
    display_errors: "1" -> ""
    display_startup_errors: "1" -> ""
    enable_dl: "1" -> ""
    error_reporting: (none) -> "22527"
    html_errors: "1" -> "0"
    ignore_repeated_errors: "0" -> ""
    ignore_repeated_source: "0" -> ""
    implicit_flush: "0" -> "1"
    log_errors: "0" -> "1"
    mail.add_x_header: "0" -> ""
    mail.mixed_lf_and_crlf: "0" -> ""
    max_execution_time: "30" -> "0"
    memory_limit: "128M" -> "-1"
    request_order: (none) -> "GP"
    session.cookie_httponly: "0" -> ""
    session.gc_divisor: "100" -> "1000"
    session.gc_probability: "1" -> "0"
    session.sid_bits_per_character: "4" -> "5"
    session.sid_length: "32" -> "26"
    short_open_tag: "1" -> ""
    unserialize_callback_func: (none) -> ""
    user_dir: (none) -> ""
    variables_order: "EGPCS" -> "GPCS"
    zend.assertions: "1" -> "-1"
    zend.exception_ignore_args: "0" -> "1"
    zend.exception_string_param_max_len: "15" -> "0"
@cmb69
Copy link
Member

cmb69 commented Jan 13, 2025

Looks like a good idea, although it might be confusing, since distros may ship non-default settings as default, so users might wonder why they have some non-default settings. Maybe only show the non-defaults when explicitly requested (maybe --ini=diff or something)?

@TimWolla
Copy link
Member Author

Maybe only show the non-defaults when explicitly requested (maybe --ini=diff or something)?

That would also work for me, no strong opinion (other than that this should be a thing one way or another). My intention with this PR was to spark this kind of discussion and feedback 😄

@@ -1096,6 +1104,31 @@ static int do_cli(int argc, char **argv) /* {{{ */
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)");
zend_printf("Additional .ini files parsed: %s\n", php_ini_scanned_files ? php_ini_scanned_files : "(none)");
zend_printf("\n");
zend_printf("Non-standard INI settings:\n");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
zend_printf("Non-standard INI settings:\n");
zend_printf("Non-default INI settings:\n");

@NattyNarwhal
Copy link
Member

Looks like a good idea, although it might be confusing, since distros may ship non-default settings as default, so users might wonder why they have some non-default settings. Maybe only show the non-defaults when explicitly requested (maybe --ini=diff or something)?

As a downstream distributor, this is something to keep in mind. This feature looks pretty useful at first glance for triaging issues users have, although we'd have to keep in mind our own defaults when they show up in the output (which themselves are derived from php.ini-production).

@TimWolla
Copy link
Member Author

although we'd have to keep in mind our own defaults when they show up in the output (which themselves are derived from php.ini-production).

Indeed. I do not expect this to be a big issue in practice, because most settings won't meaningfully affect the behavior of a PHP script. It would perhaps be a good opportunity to check whether the php.ini-production settings could be aligned with the builtin defaults one way or another.

I expect this to be most useful for the OPcache INIs and possibly extensions with a large number of settings where the user is expected to modify them (e.g. Xdebug).

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