Open felixarntz opened 1 month ago
@felixarntz Thanks for opening this issue.
I think this is a valid feature request and I support this proposal.
I do have some questions though which will need to be answered to fully understand the specs for the new sniff.
add_option()
, update_option()
, but there may be additional functions which I'm not aware of ?add_site_option()
and update_site_option()
(which currently don't support the $autoload
parameter) ?$autoload
parameter used to accept true|false|'yes'|'no'
as valid values and as of WP 6.6, it accepts null
as well.
Will older WP versions be able to handle a null
value being passed ? Or is there a risk of "passing null to non-nullable" deprecation notices or TypeErrors if null
is passed on WP < 6.6 ?
If passing null
is WP-cross-version compatible, we can include it in the recommendation. If not, we'll need a version check based on the minimum_wp_version
property (set by the plugin/theme) to determine what recommendation can be given and/or adjust the warning message based on the minimum supported WP version.Just some additional context how to decide whether an option should be autoloaded or not:
I think we'll need to have a careful think about the warning message text, but more than anything, I think this could be explained in more detail in the sniff XML documentation.
@felixarntz Would you be up to contributing these docs, or collaborating on them, once the sniff has been written ?
It used to be formally documented that the strings
'yes'
and'no'
could be passed alternatively totrue
andfalse
respectively. This is still supported and not formally deprecated, but it's also no longer encouraged, so it would be nice to clean that up over time. There are however no functional differences between passing one or the other variant, so it's certainly not as important as the main solution described above. But if it's trivial to include in the new sniff, it's worth considering.
Looking at commit WordPress/wordpress-develop@7d606a30e2f475ee434abadff7271205766157d8, I see some more potential values for the option - 'on'|'off'|'auto'|'auto-on'|'auto-off'
-, though those may only be intended for internal use (I haven't gone through the code in detail, just skimmed it for now). Could you clarify ?
And yes, it would be trivial for the sniff to flag anything which is non-true
|false
|null
and with some more, less trivial, code, I think the sniff could even auto-fix 'yes'
to true
and 'no'
to false
, which should greatly help with cleaning these things up.
Along the same lines, $autoload
parameter values which are only supposed to be for internal use and not for userland use, could also be flagged by the sniff.
This would mean the sniff logic would need to be along the following lines:
$autoload
parameter is not passed, recommend passing the parameter. Do not auto-fix as what the value should be should be a conscious decision by a developer.$autoload
parameter is passed and is true|false
, stay silent.$autoload
parameter is passed and is null
, stay silent with minimum WP 6.6, possibly flag for WP < 6.6 as unsupported until WP 6.6 ?$autoload
parameter is passed and is 'yes'|'no'
, flag as discouraged parameter values and auto-fix.$autoload
parameter is passed and is any of the internal-use only values, flag as unsupported and suggest replacing it with the appropriate true|false|null
value and auto-fix (if possible ?).Does this sound correct to you ?
@jrfnl Replying to your questions:
Which functions should be analyzed by the sniff ?
It's only add_option()
and update_option()
.
Are there (future) intentions to change the
add_site_option()
andupdate_site_option()
(which currently don't support the$autoload
parameter) ?
Not planned, no. They are predominantly for multisite network options, where there's no autoloading functionality like for regular options. On regular sites they pass through to regular options, so there's potential value here, but it's tricky from an API perspective and overall probably low impact, given how less common it is for those functions to be used.
Will older WP versions be able to handle a
null
value being passed ?
Yes, that won't be a problem at all from a functionality perspective. update_option()
formally used to accept null
prior to 6.6 already, and for add_option()
, while not documented, it would result in the correct behavior prior to 6.6, which would be the same as passing 'yes'
back then, which was the previous default. That's because add_option()
handles the parameter value like this:
$autoload = ( 'no' === $autoload || false === $autoload ) ? 'no' : 'yes';
Would you be up to contributing these docs, or collaborating on them, once the sniff has been written ?
Definitely happy to help with that!
'on'|'off'|'auto'|'auto-on'|'auto-off'
-, though those may only be intended for internal use (I haven't gone through the code in detail, just skimmed it for now). Could you clarify ?
All those values are internal values stored in the database, nothing to be concerned with for someone calling add_option()
or update_option()
. Brief explanation:
'on'|'off'
will be stored if the developer explicitly passes true|false
(or the old 'yes'|'no'
, just for BC).'auto'|'auto-on'|'auto-off'
will be stored if the developer doesn't pass anything or passes null
, since WordPress core makes the decision based on various heuristics.Along the same lines,
$autoload
parameter values which are only supposed to be for internal use and not for userland use, could also be flagged by the sniff.
I like that idea. Yeah, none of the additional above 'on'|'off'|'auto'|'auto-on'|'auto-off'
should be passed to the add_option()
or update_option()
functions.
Your summary sounds great, plus taking the above into account on the TBD sections:
null
is fine even prior to WP 6.6.'on'|'off'
can be auto-fixed to true|false
. I'm not sure about 'auto'|'auto-on'|'auto-off'
though since those are only for a decision by core. If someone passes them, I'd be worried they haven't understood how this works, so I think it's better in that case to only flag it without trying to auto-fix.Let me know if you have any follow up questions.
Thanks for your response @felixarntz. That clarifies all questions I had.
@rodrigoprimo Is this something you may like to work on ? And if so, have you got time to do so ?
@rodrigoprimo Is this something you may like to work on ? And if so, have you got time to do so ?
@jrfnl, yes, I would like to work on this new sniff. I can start next week.
Updating the sniff summary that @jrfnl created with the latest information that @felixarntz provided:
add_option()
and update_option()
are found in the code.$autoload
parameter is not passed, recommend passing the parameter. Do not auto-fix as what the value should be should be a conscious decision by a developer.$autoload
parameter is passed and is true|false|null
, stay silent.$autoload
parameter is passed and is 'yes'|'no'
, flag as discouraged parameter values and auto-fix to true|false
.$autoload
parameter, if the value is 'auto'|'auto-on'|'auto-off'
flag as unsupported without auto-fix. If the value is 'on'|'off'
, flag it as unsupported and auto-fix to true|false.
Thanks @rodrigoprimo for this summary, it looks accurate to me.
I just remembered the additional option autoloading related core functions introduced in WordPress 6.4:
wp_set_options_autoload( $options, $autoload )
wp_set_option_autoload( $option, $autoload )
wp_set_option_autoload_values( $options )
(here the autoload values are within an associative $options
array, so maybe trickier, if not impossible to lint)I just opened a WordPress Core ticket to update the parameter documentation of those functions to as well no longer recommend 'yes'|'no'
: https://core.trac.wordpress.org/ticket/61929
While they probably aren't used too much in the ecosystem compared to add_option()
and update_option()
, we may want to warn and auto-fix 'yes'|'no'
to true|false
for those functions as well. WDYT?
cc @jrfnl
I just remembered the additional option autoloading related core functions introduced in WordPress 6.4:
* `wp_set_options_autoload( $options, $autoload )` * `wp_set_option_autoload( $option, $autoload )` * `wp_set_option_autoload_values( $options )` (here the autoload values are within an associative `$options` array, so maybe trickier, if not impossible to lint)
While they probably aren't used too much in the ecosystem compared to
add_option()
andupdate_option()
, we may want to warn and auto-fix'yes'|'no'
totrue|false
for those functions as well. WDYT?
I concur and I think it would be good to include this in the sniff.
I don't think the wp_set_option_autoload_values( $options )
function will be a problem to handle as the array format is clearly defined, though it will need special casing in the sniff.
Caveat for all detection: if the values aren't being passed directly in the function call/hard-coded, we're out of luck as tracing variable definitions/function calls throughout the codebase is not something PHPCS is suitable for.
(finding the $options
definition if defined within the same scope (= within the function body of the function which calls the wp_set_option_autoload_values()
function) may be possible, but that's about it)
In other words: the below can be flagged and fixed:
add_option( $option, $value, '', 'yes' );
wp_set_option_autoload_values(
array(
'optionA' => 'no',
'optionB' => 'yes'
)
);
... but these can't be:
add_option( $option, $value, '' AUTOLOAD_VALUE );
wp_set_option_autoload_values($options_array);
Including those three functions sounds like a good idea to me as well.
We discussed generating a warning when the $autoload
parameter is missing for calls to add_option()
and update_option()
as this parameter is optional.
The same parameter is mandatory for the wp_set_options_autoload()
and wp_set_option_autoload()
functions, so I believe that for those two functions when the $autoload
parameter is missing, the sniff should generate an error. What do you think?
The same parameter is mandatory for the
wp_set_options_autoload()
andwp_set_option_autoload()
functions, so I believe that for those two functions when the$autoload
parameter is missing, the sniff should generate an error. What do you think?
I personally think for those functions we don't necessarily have to sniff for the parameter missing on those functions. In those cases it would already cause a PHP error anyway because it is technically required. So for wp_set_options_autoload()
and wp_set_option_autoload()
, I think WPCS should only focus on whether the values given are the recommended booleans. If none are given, this will already fail with WordPress core alone.
I think that is a fair point and a good option as well.
I prefer slightly the error as it seems to me as a more consistent behavior from the sniff perspective. The sniff will always generate something (a warning or a error) when the $autoload
parameter is missing instead of a warning when an optional $autoload
parameter is missing and nothing when a mandatory parameter is missing. That being said, I'm fairly new to the world of sniffs.
What is your take on this, @jrfnl?
I agree with @felixarntz on this.
It is not the job of the sniff to flag PHP errors. That's a case of scope creep for this sniff, but also for sniffs in general. Some examples of prior art: https://github.com/WordPress/WordPress-Coding-Standards/blob/7f766304b0654cee7c1dfa8e548f65fce197e05c/WordPress/Sniffs/PHP/IniSetSniff.php#L148-L151 https://github.com/WordPress/WordPress-Coding-Standards/blob/7f766304b0654cee7c1dfa8e548f65fce197e05c/WordPress/Sniffs/DateTime/CurrentTimeTimestampSniff.php#L77-L81
It is not the job of the sniff to flag PHP errors
Ok, and thanks for the examples of prior art.
FYI, I already have a functional draft of the sniff, but I won't be able to work on it for the next three weeks. I will resume this work in the last week of September.
Below I'm sharing some metrics gathered running the sniff against a local and up to date copy of the WP.org plugins repository. This gives us an idea of how the $autoload
parameter in add_option()
and update_option()
is used in the ecosystem (the current version of the sniff does not support yet the other functions). Here are the results:
PHP CODE SNIFFER INFORMATION REPORT
----------------------------------------------------------------------
`$autoload` parameter value:
param missing => 351,847 ( 91.55%)
other value => 15,845 ( 4.12%)
`true`|`false`|`null` => 12,694 ( 3.30%)
undetermined value => 3,922 ( 1.02%)
--------------------------------------------
total => 384,308 (100.00%)
----------------------------------------------------------------------
I can run it again once the sniff is finalized and then once more after a few months that it is released. This should give us an idea of how the community is responding to this change in WP.
Enjoy your time off @rodrigoprimo!
Regarding the metrics, here's some feedback:
$autoload
param for the options functions is unclear).
Keep in mind, this metric might be part of a large info report and shared outside the context of PHPCS/WPCS.true
|false
|null
- I would suggest splitting this to buckets for each individual value (as it has value to see what choices people make in the context of the performance team)'yes'
/'no'
values (and the other "known" ones), in which case, I would again suggest splitting this to report those "known" values the sniff actively looks for individually and only report whatever is left after that as "other value".Edit: if the "known" values were already in a separate bucket and just didn't show up, that would be a good thing, of course ;-)
Is your feature request related to a problem?
Several option functions (e.g.
add_option()
,update_option()
) have been supporting an$autoload
parameter that is optional and has had a default value of'yes'
. This has led to notable problems where far too many options that plugins or themes add are being autoloaded, even if they're only needed on a few specific pages of the application.WordPress 6.6 introduced changes in the API and how it should be used. While we can't require a parameter that used to be optional, it is now more than ever encouraged to always pass it. Going forward, not passing the parameter will be interpreted by core as implicit, i.e. core is able to determine whether or not to autoload the option by itself. So far this effectively still means an option without any provided value will be autoloaded, unless it is a particularly large option (the only condition implemented in WordPress core so far). But this current behavior to still mostly autoload by default is only for backward compatibility and could change in the future.
See https://make.wordpress.org/core/2024/06/18/options-api-disabling-autoload-for-large-options/ for additional context. And somewhat related: https://make.wordpress.org/core/2023/10/17/new-option-functions-in-6-4/
Describe the solution you'd like
WordPress-Extra
should include a sniff that warns whenever the$autoload
parameter is missing onadd_option()
orupdate_option()
calls.While they should typically use either
true
orfalse
, passingnull
is fine too as explicit acknowledgement that core will be able to make the decision.Nice to have
It used to be formally documented that the strings
'yes'
and'no'
could be passed alternatively totrue
andfalse
respectively. This is still supported and not formally deprecated, but it's also no longer encouraged, so it would be nice to clean that up over time. There are however no functional differences between passing one or the other variant, so it's certainly not as important as the main solution described above. But if it's trivial to include in the new sniff, it's worth considering.Additional context (optional)
Just some additional context how to decide whether an option should be autoloaded or not:
true
if the option is retrieved in the majority of page loads, including frontend requests.false
if the option is only retrieved in particular page loads, e.g. a specific page in the frontend or a specific admin screen.null
to leave the decision whether or not to autoload the option to WordPress core.One additional explanation for a somewhat common situation:
wp_prime_option_caches()
orwp_prime_option_caches_by_group()
, which were introduced in WordPress 6.4. With these functions, multiple options can be retrieved ("pre-loaded") from the database with a single request, without "polluting" the list of autoloaded options, which should remain reserved for options that are loaded across most of the WordPress site.