WPTT / WPThemeReview

PHP_CodeSniffer rules (sniffs) to enforce WordPress theme review coding conventions
MIT License
208 stars 38 forks source link

Add sniff: WordPress.PHP.IniSet #209

Closed jrfnl closed 5 years ago

jrfnl commented 5 years ago

WPCS 2.1.0 introduces a new sniff WordPress.PHP.IniSet.

From the changelog:

New WordPress.PHP.IniSet sniff to the WordPress-Extra ruleset. This sniff will detect calls to ini_set() and ini_alter() and warn against their use as changing configuration values at runtime leads to an unpredictable runtime environment, which can result in conflicts between core/plugins/themes.

  • The sniff will not throw notices about a very limited set of "safe" ini directives.
  • For a number of ini directives for which there are alternative, non-conflicting ways to achieve the same available, the sniff will throw an error and advise using the alternative.

Ref: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/releases/tag/2.1.0

I'm wondering if this sniff should be added to TRTCS. Opinions ?

dingo-d commented 5 years ago

I think that themes shouldn't set php config values at runtime, sounds like a plugin territory to me.

@carolinan @justintadlock @jocastaneda Thoughts?

aristath commented 5 years ago

The only times I can think of that would "require" a theme to change config values would be during a demo import - which is indeed plugin territory. Other than that I can't imagine a valid scenario for themes changing config values

ernilambar commented 5 years ago

INI customization is surely not a theme territory. We can add this sniff as ERROR.

dingo-d commented 5 years ago

Should we just add this in the ruleset, or should we extend it and remove all $whitelisted_options ?

jrfnl commented 5 years ago

IMO: just add it to the ruleset and possibly make it an error in all cases.

The $whitelisted_options is a very limited list and I don't think we need to worry about it unless you've seen people use any of those within themes. If you have, then yes, extending the class and overloading the property with an empty array should do the trick. I don't think any other changes should be needed.

For the details:

jrfnl commented 5 years ago

Closing as PR #219 has been merged.