Add ini_set() error reporting

[yohgaki]

Have you ever been frustrated by typo in ini_set() / ini_get() INI name?
This PR add ini_set() / ini_get() error reporting.
e.g. ini_set('does_not_exist', 1234); // Raise E_WARNING

To report INI errors, enable report_ini_error.
i.e. ini_set('report_ini_error', 1);

Travis spotted obsolete INI usage errors of our test codes
https://travis-ci.org/php/php-src/jobs/156940393#L1359
https://travis-ci.org/php/php-src/jobs/156940393#L1527
https://travis-ci.org/php/php-src/jobs/156940393#L1554
https://travis-ci.org/php/php-src/jobs/156940393#L1562

[cmb69]

Raising E_WARNING for unrecognized ini options appears to make sense, but I would not add another ini option to configure this. :-)

[yohgaki]

@cmb69 It could be
bool ini_report_error([bool $enable])
I think of this implementation. This approach has drawbacks

• Cannot apply check easily. (For our unit test, we need to modify run-tests.php)
• Cannot enable error reporting by default, BC. (We can disable this in php.ini-production)

However, I don't mind much as user should be able to enable error reporting during development. It could be RFC vote option.

[nicolas-grekas]

This is a significant bc break. It's not uncommon at all to read an ini value that is defined only when some extension is enabled, without checking if the extension is loaded first. 👎 for that reason to me...

[yohgaki]

@nicolas-grekas
It's for development, not for production. Trying to set/get non-existing module's INI value is bug most likely, isn't it? (Finding such errors during development is the purpose of this patch)

Having an option for each ini_set() and ini_get() could be good option.

[nicolas-grekas]

Looks at e.g. https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Debug/ExceptionHandler.php#L42
There is no bug here when xdebug is not installed...

[yohgaki]

@nicolas-grekas
Good example! This kind of ini_get() might be used, especially for modules like xdebug. I hope those users use @ for it.

Finding typo could be irritating. It should be easier than now. IMO.
If typo is happened on security related INI, it could be fatal.

[nikic]

I'm -1 here for the same reason as @nicolas-grekas. It's super common to indiscriminately set ini settings for extensions that may or may not be loaded (or may or may not have that setting in that version). I don't think we currently even have a way to check if an ini setting exists upfront.

[yohgaki]

@nikic
extension_loaded() could be used, but the code wouldn't be nice. Simply using @ would be the best resolution.

Setting INI values could be security problem, so only raise warning for ini_set()?

I'll make this a voting option.

[dshafik]

I think this message should be "INI Setting "%s" does not exist"

[dshafik]

@yohgaki using @ to silence errors is very much frowned upon, and is slow.

Adding a ini_parameter_exists() function might be a worthwhile addition, OR make the behavior an addition parameter for ini_(set|get)(), rather than an INI setting.

Either way, it should be a E_NOTICE, not a E_WARNING — given that a nonexistent variable or constant is an E_NOTICE, which are potentially way more likely to be bugs than this.

[yohgaki]

@dshafik

Adding a ini_parameter_exists() function might be a worthwhile addition, OR make the behavior an addition parameter for ini_(set|get)(), rather than an INI setting.

I like latter. ini_set($name, $value [, $ignore_error=FALSE]), ini_get($name [, $ignore_error=FALSE]). However, I expect some people will have complaint because internal functions do not allow excessive parameters. Therefore, ini_parameter_exists() may be better choice because users can emulate behavior with script.

I'll make these RFC vote option.

[mwillbanks]

Going back to @dshafik's argument, I would far prefer to see an addition of a parameter but rather than it error to throw an exception. This way I can handle it accordingly whereas having an error itself would prove to be more work by having to handle the error condition and swap error handlers.

string ini_set ( string $varname , string $newvalue [, boolean $throw = false])
string ini_get ( string $varname [, boolean $throw = false])

Now in this case, I can define how I feel about the error condition and if I want to catch for instance on a non-existence I can. I really like that this will provide some error mechanism but having the ability to throw and catch (even in real-world production code) is very useful.

[KalleZ]

Closing this due to inactivity. Please open a new PR and link to this if active work is being put back into it.