-
-
Notifications
You must be signed in to change notification settings - Fork 503
✨ 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
base: develop
Are you sure you want to change the base?
Conversation
The only problem is that that is not official documentation. And if their site goes down, you have a dead link in the documentation.
Does the validation passes against the phpcsdocs.xsd? |
There was a problem hiding this 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.
9d70828
to
a3da583
Compare
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)).
That makes sense. As @jrfnl suggested, I removed the paragraph that included this link.
I believe so. This is how I tested:
Let me know if I'm missing something. |
cd264e6
to
fc98b8e
Compare
@jrfnl, I addressed the two points that were pending. Could you please take another look at this PR when you get a chance? |
There was a problem hiding this 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[ |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
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. |
There was a problem hiding this comment.
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."
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()
andupdate_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 ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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'; |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
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. |
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>
…he rest of the sniff code
This allows to use isset() instead of in_array()
Co-authored-by: Juliette <663378+jrfnl@users.noreply.github.com>
…t_option_functions`
f80b867
to
325f3ba
Compare
Rebased without changes to fix an unrelated failing test. 39524bc is the first commit addressing the latest feedback. |
…n discouraged values
…xable internal values
…st to reach that condition
…t the condition on OptionAutoloadSniff.php:247 correctly
31270ce
to
4c7d770
Compare
@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):
![]() |
Regarding the comment quoted above, I'm not sure why I cannot reply directly to it, but it was addressed, and now the |
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:
For
add_option()
andupdate_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 ofwp_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 istrue|false|null
foradd_option()
andupdate_option()
ortrue|false
forwp_set_options_autoload()
,wp_set_option_autoload()
andwp_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 totrue|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 totrue|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:
$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.<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.<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.