Fixes #1447, adds new sniff for blacklisted ini_set directives.

[NielsdeBlaauw]

I've decided to create a new sniff for checking proper ini_set directives. With this feature the original sniff from DiscouragedPHPFunctionsSniff.php is removed.

• It adds a whitelist of options that are valid to set by a plugin (in the case of short_open_tag it is only valid with some limited values).
• It adds a blacklist of options that have better alternatives, or might influence other plugins. (short_open_tag is checked for values turning it off).
• All ini_set directives that are not either in the whitelist or blacklist are marked as risky with a warning. This mimics original behaviour.

For now I've used the following insights from the issue:

• max_execution_time is an error, as a better alternative exists that should be preferred.
• I've left error_log as a warning for now, as I personally would prefer that plugins do not move the error_log around. I could only see some security plugins doing this. They can use an ignore statement if they really need it.

phpcbf did not work for me locally, so this probably needs some formatting initially.

Fixes #1447

[jrfnl]

Hi @NielsdeBlaauw, I'm very happy to see your first PR and it's looking very good indeed.

One thing I'm missing is for the sniff to be added to one of the rulesets, otherwise it will only be active when the overall WordPress ruleset is used.
In this case, adding the sniff to the WordPress-Extra ruleset seems appropriate.

Other than that, I see no major issues, just some minor code and documentation consistency issues.

So, please don't get discouraged by the amount of comments I've left, safe for my remark about ini_alter, everything else is just a case of dotting the i's and crossing the t's.

[jrfnl]

(The principle outlined here goes for this line and the next)

While for the "whitelist", using the raw content of the parameter is fine, using it for the "blacklist" could prevent errors from being thrown.

To explain this a bit further:

• If something is not on the whitelist, a warning will be thrown. In other words, if people pass the $option_name in in an obfuscated manner, they might get a false positive.
• If something is on the blacklist, however, and it is not recognized correctly, an error would be downgraded to a warning and warnings are often disregarded or silenced (using -n on the command line).

As in the case of this sniff, in both situations, a warning would still be thrown, IMO this does not need to be changed, but I did want to make sure you are aware of this.

Background information/example of when this comes into play

The WPCS sniffs are used in various contexts and for some of these, there will be people actively trying to circumvent the sniffs - think VIPCS or Theme review -.

Example code to circumvent the sniff throwing an error (though it will still throw a warning):

ini_set( 'error' . '_' . 'reporting', 0);
ini_set( 'short_open_tag', 00); // 00 = hex integer 0

[jrfnl]

Just marking this as "unresolved" to make it easier to find for other people as this comment documents why using the raw parameter value for this sniff is ok.

[jrfnl]

Darn... one more thing and this doesn't necessarily have to be solved now:

It might be an idea to make the error codes modular, at least for the Blacklist code, like so $this->string_to_errorcode( $option_name . '_Blacklisted' ),

That way, if a plugin has a very good reason to change one of these ini directives, they could <exclude ...> just the errorcode for that particular directive, without disabling the whole blacklist.

[NielsdeBlaauw]

Thanks for the feedback. All should be fixed now. Let me know.

[jrfnl]

Hi @NielsdeBlaauw Nearly there. I've marked a few of my earlier comments as unresolved as they haven't been completely addressed, but this is nearly ready for merge.

Would you like to squash the commits before that time or shall I do that when I merge it ?

P.S.: Thank you for your quick response to the review!

[NielsdeBlaauw]

@jrfnl Thanks for taking the time for such extensive feedback.

Travis seems to be having some issues on the php5.4 test at the moment (see https://travis-ci.org/WordPress-Coding-Standards/WordPress-Coding-Standards/jobs/497198381)

Would you like to squash the commits before that time or shall I do that when I merge it ?

If you can squash it in the merge that'd be great.

[jrfnl]

🎉

[jrfnl]

Thanks for taking the time for such extensive feedback.

You're welcome & thanks for the quick turn-around, makes things so much easier 😄

Travis seems to be having some issues on the php5.4 test at the moment (see https://travis-ci.org/WordPress-Coding-Standards/WordPress-Coding-Standards/jobs/497198381)

I restarted the build and it's fine now.

If you can squash it in the merge that'd be great.

Will do.

---

@GaryJones Did you want to have another look or shall I merge it ?

[NielsdeBlaauw]

Hi @GaryJones

Added the additional tests and changed the value verification to be case insensitive.