Repository navigation
Add sniff to warn about missing $autoload parameter when calling relevant WordPress option functions #2473
Description
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 on add_option() or update_option() calls.
While they should typically use either true or false, passing null 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 to true and false 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:
- Use
trueif the option is retrieved in the majority of page loads, including frontend requests. - Use
falseif the option is only retrieved in particular page loads, e.g. a specific page in the frontend or a specific admin screen. - Use
nullto leave the decision whether or not to autoload the option to WordPress core.
One additional explanation for a somewhat common situation:
- If a plugin or theme uses several options only in a shared place (e.g. a specific admin screen), not autoloading them may intuitively feel like the wrong decision because it would make several database requests for them instead of a combined one. However, in this situation the options should still not be autoloaded because it would lead those options to be fetched in all kinds of other areas where they are never used.
- Instead, for such a scenario plugin developers should rely on the functions
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.
Activity
jrfnl commentedon Aug 12, 2024
@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.
My guess is:
add_option(),update_option(), but there may be additional functions which I'm not aware of ?add_site_option()andupdate_site_option()(which currently don't support the$autoloadparameter) ?$autoloadparameter used to accepttrue|false|'yes'|'no'as valid values and as of WP 6.6, it acceptsnullas well.Will older WP versions be able to handle a
nullvalue being passed ? Or is there a risk of "passing null to non-nullable" deprecation notices or TypeErrors ifnullis passed on WP < 6.6 ?If passing
nullis WP-cross-version compatible, we can include it in the recommendation. If not, we'll need a version check based on theminimum_wp_versionproperty (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.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 ?
Looking at commit WordPress/wordpress-develop@7d606a3, 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|nulland with some more, less trivial, code, I think the sniff could even auto-fix'yes'totrueand'no'tofalse, which should greatly help with cleaning these things up.Along the same lines,
$autoloadparameter 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:
$autoloadparameter 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.$autoloadparameter is passed and istrue|false, stay silent.$autoloadparameter is passed and isnull, stay silent with minimum WP 6.6, possibly flag for WP < 6.6 as unsupported until WP 6.6 ?$autoloadparameter is passed and is'yes'|'no', flag as discouraged parameter values and auto-fix.$autoloadparameter is passed and is any of the internal-use only values, flag as unsupported and suggest replacing it with the appropriatetrue|false|nullvalue and auto-fix (if possible ?).Does this sound correct to you ?
felixarntz commentedon Aug 13, 2024
@jrfnl Replying to your questions:
It's only
add_option()andupdate_option().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.
Yes, that won't be a problem at all from a functionality perspective.
update_option()formally used to acceptnullprior to 6.6 already, and foradd_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 becauseadd_option()handles the parameter value like this:Definitely happy to help with that!
All those values are internal values stored in the database, nothing to be concerned with for someone calling
add_option()orupdate_option(). Brief explanation:'on'|'off'will be stored if the developer explicitly passestrue|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 passesnull, since WordPress core makes the decision based on various heuristics.I like that idea. Yeah, none of the additional above
'on'|'off'|'auto'|'auto-on'|'auto-off'should be passed to theadd_option()orupdate_option()functions.Your summary sounds great, plus taking the above into account on the TBD sections:
nullis fine even prior to WP 6.6.'on'|'off'can be auto-fixed totrue|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.
jrfnl commentedon Aug 13, 2024
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 commentedon Aug 13, 2024
@jrfnl, yes, I would like to work on this new sniff. I can start next week.
rodrigoprimo commentedon Aug 22, 2024
Updating the sniff summary that @jrfnl created with the latest information that @felixarntz provided:
add_option()andupdate_option()are found in the code.$autoloadparameter 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.$autoloadparameter is passed and istrue|false|null, stay silent.$autoloadparameter is passed and is'yes'|'no', flag as discouraged parameter values and auto-fix totrue|false.$autoloadparameter, 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 totrue|false.felixarntz commentedon Aug 26, 2024
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$optionsarray, 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/61929While 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|falsefor those functions as well. WDYT?cc @jrfnl
jrfnl commentedon Aug 27, 2024
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
$optionsdefinition if defined within the same scope (= within the function body of the function which calls thewp_set_option_autoload_values()function) may be possible, but that's about it)In other words: the below can be flagged and fixed:
... but these can't be:
rodrigoprimo commentedon Aug 28, 2024
Including those three functions sounds like a good idea to me as well.
We discussed generating a warning when the
$autoloadparameter is missing for calls toadd_option()andupdate_option()as this parameter is optional.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$autoloadparameter is missing, the sniff should generate an error. What do you think?felixarntz commentedon Aug 28, 2024
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()andwp_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.rodrigoprimo commentedon Aug 28, 2024
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
$autoloadparameter is missing instead of a warning when an optional$autoloadparameter 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?
jrfnl commentedon Aug 28, 2024
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:
WordPress-Coding-Standards/WordPress/Sniffs/PHP/IniSetSniff.php
Lines 148 to 151 in 7f76630
WordPress-Coding-Standards/WordPress/Sniffs/DateTime/CurrentTimeTimestampSniff.php
Lines 77 to 81 in 7f76630
rodrigoprimo commentedon Aug 31, 2024
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
$autoloadparameter inadd_option()andupdate_option()is used in the ecosystem (the current version of the sniff does not support yet the other functions). Here are the results: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.
jrfnl commentedon Aug 31, 2024
Enjoy your time off @rodrigoprimo!
Regarding the metrics, here's some feedback:
$autoloadparam 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 ;-)
rodrigoprimo commentedon Dec 12, 2024
Sorry for the long delay in responding. I have resumed work on this sniff today.
Thanks for your feedback regarding the metrics, @jrfnl. I have implemented the changes that you suggested.
Here are the updated metrics. The numbers are slightly different because now the sniff also checks for the functions
wp_set_options_autoload()andwp_set_options_autoload(), and a few weeks after gathering the metrics for the first time, I updated my local copy of the WP.org plugins:rodrigoprimo commentedon Dec 12, 2024
Updating once again the sniff summary to include what was discussed in the previous comments:
add_option( $option, $value = '', $deprecated = '', $autoload = null)update_option( $option, $value, $autoload = null )wp_set_options_autoload( $options, $autoload )wp_set_option_autoload( $option, $autoload )wp_set_option_autoload_values( $options )add_option()andupdate_option(), if the$autoloadparameter 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. For the other functions (wp_set_options_autoload(),wp_set_option_autoload()andwp_set_option_autoload_values()), the$autoloadparameter (or$optionsparameter in the case ofwp_set_option_autoload_values()) is mandatory so it is not needed to check if the parameter is omitted. It is already a PHP fatal error.$autoloadparameter is passed and istrue|false|nullforadd_option()andupdate_option()ortrue|falseforwp_set_options_autoload(),wp_set_option_autoload()andwp_set_option_autoload_values(), stay silent.$autoloadparameter is passed and is'yes'|'no', flag as discouraged parameter values and auto-fix totrue|false.$autoloadparameter, 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 totrue|false.wp_set_option_autoload_values()will require special handling as the autoload value is passed as part of the$optionsparameter.felixarntz commentedon Dec 16, 2024
@rodrigoprimo Thank you for the update and for continuing to work on this!
Your summary looks correct to me, only a few very minor clarifications (potentially they were already clear to you, but I want to make sure): The three newer functions have slightly different implication than
add_option()andupdate_option()on the$autoloadvalue:$autoloadparameter is required, so the check whether the parameter is omitted shouldn't be relevant there, as that would already lead to a PHP fatal error.null. So onlytrue|falsewould be correct values, with'yes'|'no'also working but being discouraged, and'on'|'off'also working but being unsupported. So basically the same asadd_option()andupdate_option(), but withoutnull. So passingnullto those functions should be flagged (which I honestly don't think would be common for them as they're purely autoload-related functions).rodrigoprimo commentedon Feb 14, 2025
Thanks for the remarks, @felixarntz. I resumed working on this sniff and have the commit ready to submit a PR. I'm just waiting for feedback on #2518, which fixes a bug in an abstract class the sniff uses.
I updated the summary above to reflect your points. I had missed that the newer functions don't work with
null. Thanks for pointing that out.I also removed the last phrase from the last item of the summary that describes the special handling for
wp_set_option_autoload_values():I discussed this with @jrfnl, and it is not trivial to implement. So, the sniff will ignore calls to
wp_set_option_autoload_values()if anything other than an array is passed as the first parameter, at least in this initial version.sybrew commentedon Feb 23, 2025
Contrary to the statement above in the second comment, I want to point out that the use of
update_site_option()is not uncommon: https://wpdirectory.net/search/01JMTK5K9MWA8VCCXS0RFA2ZCT. Autoloading for those should be considered in a future update for WP; thus also this sniff.jrfnl commentedon Feb 24, 2025
sybrew commentedon Feb 24, 2025
jrfnl commentedon Feb 24, 2025
sybrew commentedon Feb 24, 2025