Suppress calls to ini_set in core

[mdwheele]

On many shared hosts, the PHP function ini_set is disabled for security reasons. Calls to the function when it is disabled generate a warning that is usually not shown to the user because display_errors is configured as such. However, this event is still logged and for high traffic sites, this can create a ton of events.

You can suppress warnings from this function by prepending an @ symbol to the beginning of each call. We are already doing this in many places in core.

By running grep -r "[^@]ini_set" . in the root of a WordPress installation, all occurrences will be located.

In addition, the following command can be run in the root to patch all occurrences by prepending with an @ symbol.

find . -type f -exec sed -i 's/@*ini_set/@ini_set/g' {} \;

This searches the current directory for all occurrences of ini_set with zero-or-more occurrences of @ prepended. This is an idempotent command in that it should produce the same results if run many times. It could be made part of a build preparation process to clean occurrences of ini_set without suppression.

I will be attaching a patch that makes these changes!

Thanks!

[jdgrimes]

What about checking if ini_set() is callable before attempting to call it instead? I think that would be preferable to using the @ operator, which should (almost) always be avoided.

[mdwheele]

That's definitely reasonable as well. It depends on what core suggests. Since they have been using suppression operator for this exact use-case, 33112.1.patch follows this convention.

However, for completeness, do note that any function that is made a part of /etc/php.ini#disable_functions is still "callable", it will just result in a warning. Therefore, the following type of check would need to be put in place for all affected locations:

if(false === strpos(ini_get("disable_functions"), "ini_set")) {  
    // Meaning that "ini_set" was NOT found in the disable functions list; so we can use it safely.
}

The biggest wrench in this is that just as many shared hosts disable ini_set, they also disable ini_get.

Unless there is some other way of doing this, I think suppression operator (while definitely a bad practice for other use-cases) may be an acceptable solution here.

Thanks for the comment!

[jdgrimes]

Replying to mdwheele:

Right, it would require ini_get() to be available, and if that is commonly disabled, I guess we have little choice. I just thought that I would throw it out for discussion.

[SergeyBiryukov]

For reference, we have a precedent for checking disable_functions in [29330].

[mdwheele]

I'm definitely willing to follow 29330's precedent! Is there somewhere in particular I can put a utility function for checking this? If so, please advise on the best place to lay this and I'll:

• Attach a patch that adds the two functions with unit tests
• Alter my current patch to use these instead

As far as the ini_get dependency is concerned, perhaps that's just where we draw the line to avoid using the suppression operator? If we go this route, I'm amenable to submit patches against other occurrences of suppression operator for the same type of use-case (suppressing ini_set calls).

Just let me know!

Utility functions are something to the tune of:

function is_enabled($function) {
    $disabled_functions=explode(',',ini_get('disable_functions'));
    return ! in_array($function, $disabled_functions);
}

function is_disabled($function) {
    return ! is_enabled($function);
}

[jdgrimes]

Replying to SergeyBiryukov:

For reference, we have a precedent for checking disable_functions in [29330].

I've just checked, and ini_get() is also used 45 other places in core. In only 2 places is it prepended with @.

[mdwheele]

Replying to jdgrimes:

I've just checked, and ini_get() is also used 45 other places in core. In only 2 places is it prepended with @.

Sweet. Some of those also look like serious dependencies on the return value of that function as well. If it's disabled, it returns NULL which would still operate as long as code dependent on it understood that.

Either way, we probably need to scope what we want to do here in this ticket. I think that since Sergey has linked to us precedent for using ini_get to detect whether or not a function is enabled, that we should press forward with @jdgrimes original suggestion, which would include:

• Implementing some utility functions: is_enabled, is_disabled (technically optional, but would be a lot of duplicate code everywhere) (also, naming is up for debate; is_enabled might be unclear)
• Refreshing my patch to use these new utilities
• Looking for other instances of ini_set and conditionally running them based on whether it is enabled.

That'd probably wrap up *this* ticket with some opportunities for future work elsewhere (documenting the functions, sending patches out to popular plugins to use functions, etc.)

Thoughts?

P.S. @jdgrimes: hope I didn't come across as snarky. I'm bad at text communication and usually type things as they come in. Definitely didn't mean to sound short or anything!

Example of return value for ini_get when disabled:

[vagrant@local wordpress]$ php -ddisable_functions=ini_get -a
Interactive shell

php > var_dump(ini_get('display_errors'));
PHP Warning:  ini_get() has been disabled for security reasons in php shell code on line 1

Warning: ini_get() has been disabled for security reasons in php shell code on line 1
NULL

[jdgrimes]

Replying to mdwheele:

• Implementing some utility functions: is_enabled, is_disabled (technically optional, but would be a lot of duplicate code everywhere) (also, naming is up for debate; is_enabled might be unclear)

I think is_func_*abled() or is_function_*abled() would be more obvious.

P.S. @jdgrimes: hope I didn't come across as snarky. I'm bad at text communication and usually type things as they come in. Definitely didn't mean to sound short or anything!

No, no, I didn't think so. Sometimes it is hard to tell what tone is intended in text, but I usually try to just assume the best. :-)

[mdwheele]

Replying to obenland:

New to the contributing community so unaware of a lot of the etiquette though I did my best to read through the handbook. Should I not have attributed trunk as the version I was submitting against? If you can point me at related docs dictating intended behaviour here, please do!

I'll be sending along patches to wrap this up as soon as I get some free time to spare. Joined the Slack channel and saw that your comment was part of some "bug scrub".

Thanks!

[obenland]

Replying to mdwheele:

New to the contributing community so unaware of a lot of the etiquette though I did my best to read through the handbook. Should I not have attributed trunk as the version I was submitting against? If you can point me at related docs dictating intended behaviour here, please do!

First off: Welcome mdwheele and thanks for helping! :)

The version field indicates the earliest affected or applicable version for the reported issue. trunk would be the current development version of 4.3 which is probably not when the ini_set issue first arose. I just removed it as a part of triaging tickets that are awaiting review, so we have an accurate reflection of bugs reported against the new version.

There is ​some documentation around Trac in the Core Contributor Handbook.

[SergeyBiryukov]

Related: #37680, #48693.

[SergeyBiryukov]

#49101 was marked as a duplicate.