Skip to content

✨ New WordPress.WP.OptionAutoload sniff #2520

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 26 commits into
base: develop
Choose a base branch
from

Conversation

rodrigoprimo
Copy link
Collaborator

This sniff warns users about missing $autoload parameter when calling relevant WordPress option functions.

It includes tests and sniff documentation in the XML format.

Closes #2473

Sniff behavior

The sniff runs when the following functions are found in the code:

  • 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 )

For add_option() and update_option(), if the $autoload parameter is not passed, it simply recommends passing it. For the other three functions, the $autoload parameter (or $options parameter in the case of wp_set_option_autoload_values()) is mandatory, so it is not needed to check if the parameter is omitted.

If the $autoload parameter is passed and is true|false|null for add_option() and update_option() or true|false for wp_set_options_autoload(), wp_set_option_autoload() and wp_set_option_autoload_values(), the sniff stays silent as those are valid values.

If the $autoload parameter is passed and is 'yes'|'no', the sniff flags as discouraged parameter values and auto-fixes to true|false.

For the internal-use only values of the $autoload parameter, if the value is 'auto'|'auto-on'|'auto-off' the sniff flags it as unsupported without auto-fixing. If the value is 'on'|'off', the sniff flags it as unsupported and auto-fixes to true|false.

Any other value passed is flagged as unsupported, and a list of supported values is provided.

Open questions

I'm unsure about a few choices that I made in the XML documentation and could use some feedback on the following:

  • I opted to include a link to a document explaining how to decide which value to use for the $autoload parameter. Since I could not find something wp.org, I ended up linking to a post written by a core committer in their blog. I'm not sure if this is a problem or not.
  • Maybe combining the <standard> blocks for invalid, deprecated, and internal-only values in a single block would be better? I created a block for each, but there is some repetition in the block description and the valid examples.
  • I added a <standard> block without code examples at the top with a general description of what the sniff does before creating blocks for each of the warnings generated by the sniff.

@dingo-d
Copy link
Member

dingo-d commented Feb 27, 2025

I opted to include a link to a document explaining how to decide which value to use for the $autoload parameter. Since I could not find something wp.org, I ended up linking to a post written by a core committer in their blog. I'm not sure if this is a problem or not.

The only problem is that that is not official documentation. And if their site goes down, you have a dead link in the documentation.

I added a block without code examples at the top with a general description of what the sniff does before creating blocks for each of the warnings generated by the sniff.

Does the validation passes against the phpcsdocs.xsd?

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rodrigoprimo Thank you for working on this.

I've so far only done a first pass reading through it (without running the sniff/docs etc), but seeing the amount of remarks I've already left, I figured I should give you the chance to iterate on the current state before continuing my review.

@rodrigoprimo
Copy link
Collaborator Author

Thanks for your review, @dingo-d and @jrfnl! I believe I addressed all of your comments except two that are still pending, and I will need a bit more time (#2520 (comment) and #2520 (comment)).

The only problem is that that is not official documentation. And if their site goes down, you have a dead link in the documentation.

That makes sense. As @jrfnl suggested, I removed the paragraph that included this link.

Does the validation passes against the phpcsdocs.xsd?

I believe so. This is how I tested:

$ xmllint --schema phpcsdocs.xsd WordPress/Docs/WP/OptionAutoloadStandard.xml --noout 
WordPress/Docs/WP/OptionAutoloadStandard.xml validates

Let me know if I'm missing something.

@rodrigoprimo
Copy link
Collaborator Author

@jrfnl, I addressed the two points that were pending. Could you please take another look at this PR when you get a chance?

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @rodrigoprimo thanks for making those initial updates.

I've now done a proper code review (with the exception of the tests). Hope this helps!

</standard>
<code_comparison>
<code title="Valid: Using a boolean value or `null`">
<![CDATA[
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the valid example have an example from both function groups ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 22 to 24
Using 'yes' or 'no' is deprecated since WordPress 6.6.0. Using 'auto', 'auto-on', 'auto-off',
'on', or 'off' in plugin/theme code is strongly discouraged as those values are meant for
internal-use only.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we're not giving people ideas by explicitly listing the internal values.... ?
Maybe the second sentence should be removed ? Or maybe just mention something along the lines of "Plugin/theme code should also not use values for the parameter which are for WP-Core internal use only."

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opted to add the sentence that you suggested to mention that plugin/theme code should not use internal values, while not mentioning the values directly to avoid giving people ideas.

]]>
</code>
</code_comparison>
<standard>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to move this <standard> block up ? This is about the parameter being missing, while the first block also talks about that, so it kind of feels like having this above the block about the values reads more fluently.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand your suggestion. As far as I can see, the first block does not mention the parameter being missing for add_option() and update_option(), so I'm not sure why it would read better if this <standard> block were moved up.

My reasoning for ordering the blocks the way they are was:

  • First block: introduces what the sniff is about and what the autoload parameter is.
  • Second block: explains the rule about the parameter value that applies to all the functions that this sniff cares about.
  • Third block: explains the rule about the parameter missing that only applies to add_option() and update_option().

I think I prefer the current order, but I'm happy to reconsider if I'm missing something.


$autoload_value = TextStrings::stripQuotes( $autoload_info['clean'] );

$known_discouraged_values = array_merge( $this->deprecated_values, $this->internal_values_non_fixable, $this->internal_values_fixable );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason to use array_merge() instead of array union (+) ? The arrays being merged are perfectly suitable for use with array union.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

$data = array( $autoload_info['clean'], $this->fixable_values[ $autoload_value ] );
} elseif ( isset( $this->internal_values_fixable[ $autoload_value ] ) ) {
$message = 'The use of `%s` as the value of the `$autoload` parameter is discouraged. Use `%s` instead.';
$error_code = 'InternalUseOnly';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I like the same error code being used for two - closely related, but still different - errors here.... (line 397 + 401)

Maybe the second error code (line 401) could be something like InternalUseOnlyAuto (as they values which will be flagged all start with "auto") ?

And maybe this one should then be InternalUseOnlyOnOff ? (to keep things consistent)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@rodrigoprimo
Copy link
Collaborator Author

Thanks for taking another look at this PR, @jrfnl. I started replying to your feedback and implementing the changes, but I was unable to finish everything today. I won't be able to look into this next week. The following week, I will address the comments that I was unable to address today.

rodrigoprimo and others added 15 commits July 4, 2025 18:31
This sniff warns users about missing `$autoload` parameter when calling relevant WordPress option functions.

The sniff runs when the following functions are found in the code:
  - 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 )

For `add_option()` and `update_option()`, if the `$autoload` parameter is not passed, it simply recommends passing it. For the other three functions, the `$autoload` parameter (or `$options` parameter in the case of `wp_set_option_autoload_values()`) is mandatory, so it is not needed to check if the parameter is omitted.

If the `$autoload` parameter is passed and is `true|false|null` for `add_option()` and `update_option()` or `true|false` for `wp_set_options_autoload()`, `wp_set_option_autoload()` and `wp_set_option_autoload_values()`, the sniff stays silent as those are valid values.

If the `$autoload` parameter is passed and is `'yes'|'no'`, the sniff flags as discouraged parameter values and auto-fixes to `true|false`.

For the internal-use only values of the `$autoload` parameter, if the value is `'auto'|'auto-on'|'auto-off'` the sniff flags it as unsupported without auto-fixing. If the value is `'on'|'off'`, the sniff flags it as unsupported and auto-fixes to `true|false`.

Any other value passed is flagged as an unsupported value and a list of supported values is provided.

The sniff ignores the function call when it is not able to determine the value of the `$autoload` parameter. For example, when a variable or a constant is passed.
Co-authored-by: Juliette <663378+jrfnl@users.noreply.github.com>
This allows to use isset() instead of in_array()
Co-authored-by: Juliette <663378+jrfnl@users.noreply.github.com>
@rodrigoprimo rodrigoprimo force-pushed the new-option-autoload branch from f80b867 to 325f3ba Compare July 4, 2025 21:35
@rodrigoprimo
Copy link
Collaborator Author

rodrigoprimo commented Jul 4, 2025

Rebased without changes to fix an unrelated failing test. 39524bc is the first commit addressing the latest feedback.

@rodrigoprimo rodrigoprimo force-pushed the new-option-autoload branch from 31270ce to 4c7d770 Compare July 15, 2025 17:43
@rodrigoprimo
Copy link
Collaborator Author

@jrfnl, thanks again for your review! I finished addressing all the comments that I was not able to address before, and now this PR is ready for another review. Just a reminder that 39524bc is the first commit addressing the latest feedback, as I had to rebase without changes to fix an unrelated failing test.

Also, just noting here that I had to force push the last commit as the build was failing due to a temporary PHPStan error. Documenting here in case we start seeing this error more often (https://github.com/rodrigoprimo/WordPress-Coding-Standards/actions/runs/16300169675/job/46032411241):

==> Setup Tools
✓ composer Added composer 2.8.10
✗ phpstan 403: rate limit exceeded
Screenshot from 2025-07-15 14-44-21

@rodrigoprimo
Copy link
Collaborator Author

What about moving it into the condition ?

Regarding the comment quoted above, I'm not sure why I cannot reply directly to it, but it was addressed, and now the param missing metric will only be recorded for add_option() and update_option().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add sniff to warn about missing $autoload parameter when calling relevant WordPress option functions
3 participants