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.

Sign up for GitHub

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 8 commits into
base: develop
Choose a base branch
from
Open

✨ New WordPress.WP.OptionAutoload sniff #2520

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f384fb0
✨ New WordPress.WP.OptionAutoload sniff
rodrigoprimo Aug 26, 2024
95afb67
Apply suggestions from code review
rodrigoprimo Feb 28, 2025
a3da583
Attemp to improve the XML documentation by combining standards
rodrigoprimo Feb 28, 2025
3aad7d3
Fix unintentional coding standard violation in a test case
rodrigoprimo Mar 11, 2025
571b0ed
Update some tests to have more function name case variations
rodrigoprimo Mar 12, 2025
1575735
Update some tests to have more whitespaces, comments and annotations
rodrigoprimo Mar 12, 2025
4dbe72a
Use `false === isset()` instead of `! isset()` for consistency with t…
rodrigoprimo Mar 12, 2025
fc98b8e
Improve sniff performance by changing the format of some properties
rodrigoprimo Mar 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions WordPress-Extra/ruleset.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,13 @@
https://github.com/WordPress/WordPress-Coding-Standards/issues/2459 -->
<rule ref="WordPress.WP.GetMetaSingle"/>

<!-- Flags calls to add_option(), update_option(), wp_set_options_autoload(),
wp_set_option_autoload(), wp_set_option_autoload_values() functions when the
`$autoload` parameter is missing or contain an invalid, internal-only or
deprecated value.
https://github.com/WordPress/WordPress-Coding-Standards/issues/2473 -->
<rule ref="WordPress.WP.OptionAutoload"/>

<!--
#############################################################################
Code style sniffs for more recent PHP features and syntaxes.
Expand Down
135 changes: 135 additions & 0 deletions WordPress/Docs/WP/OptionAutoloadStandard.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
<?xml version="1.0"?>
<documentation xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:noNamespaceSchemaLocation="https://phpcsstandards.github.io/PHPCSDevTools/phpcsdocs.xsd"
title="Option Autoload"
>
<standard>
<![CDATA[
When using `add_option()`, `update_option()`, `wp_set_options_autoload()`,
`wp_set_option_autoload()`, or `wp_set_option_autoload_values()`, it is recommended to
explicitly set the autoload value to ensure predictable behavior. This value determines whether
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
explicitly set the autoload value to ensure predictable behavior. This value determines whether
explicitly set the autoload parameter to ensure predictable behavior. This paremeter determines whether

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 tried not to use parameter as for wp_set_option_autoload_values() autoload is not a parameter. But I agree that value is probably worst. I will accept your suggestion, and I will let you know if I can think of a better approach.

the option is automatically loaded on every page load, which can impact site performance.

Check https://felix-arntz.me/blog/autoloading-wordpress-options-efficiently-and-responsibly/ for
more information on when to set autoload to `true` or `false`.
]]>
</standard>
<standard>
<![CDATA[
Even though the autoload parameter is optional for `add_option()` and `update_option()`, it is
recommended to always explicitly set it.
]]>
</standard>
Copy link
Member

Choose a reason for hiding this comment

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

IMO, this block is redundant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See #2520 (comment)

<code_comparison>
<code title="Valid: Explicitly setting the autoload parameter.">
<![CDATA[
add_option(
'my_option',
'value',
'',
<em>true</em>
);
]]>
</code>
<code title="Invalid: Autoload parameter missing.">
<![CDATA[
add_option(
'my_option',
'value',
''
);
]]>
</code>
</code_comparison>
<standard>
<![CDATA[
Using 'yes' or 'no' is not recommended. Those values are deprecated. Instead, use
`true`|`false`|`null` if using `add_option()` or `update_option()` or `true`|`false` if using
`wp_set_option_autoload_values()`, `wp_set_option_autoload()`, or `wp_set_options_autoload()`.
Copy link
Member

Choose a reason for hiding this comment

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

Please rephrase this from a "negative" paragraph to a "positive" paragraph.

Maybe something along the lines of:

Suggested change
Using 'yes' or 'no' is not recommended. Those values are deprecated. Instead, use
`true`|`false`|`null` if using `add_option()` or `update_option()` or `true`|`false` if using
`wp_set_option_autoload_values()`, `wp_set_option_autoload()`, or `wp_set_options_autoload()`.
When calling the `add_option()` or `update_option()` functions, use `true`, `false` or `null` as the value for the autoload parameter.
For the `wp_set_option_autoload_values()`, `wp_set_option_autoload()`, or `wp_set_options_autoload()` functions, use `true` or `false`.
Using 'yes' or 'no' as the value for the autoload parameter is deprecated since WordPress x.x.x.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See #2520 (comment)

]]>
</standard>
<code_comparison>
<code title="Valid: Using boolean values.">
<![CDATA[
update_option(
'my_option',
'value',
<em>true</em>
);
]]>
</code>
<code title="Invalid: Using deprecated values.">
<![CDATA[
update_option(
'my_option',
'value',
<em>'yes'</em>
);
]]>
</code>
</code_comparison>
<standard>
<![CDATA[
It is not recommended to use internal-only values ('auto', 'auto-on', 'auto-off', 'on', 'off')
in plugin or theme code. Instead, use `true`|`false`|`null` if using `add_option()` or
`update_option()` or `true`|`false` if using `wp_set_option_autoload_values()`,
`wp_set_option_autoload()`, or `wp_set_options_autoload()`.
Copy link
Member

Choose a reason for hiding this comment

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

See above, please rephrase. "It is not recommended..." => "It is strongly discouraged..."

Might even be better to combine this standard block with the previous one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See #2520 (comment)

]]>
</standard>
<code_comparison>
<code title="Valid: Using boolean values.">
<![CDATA[
wp_set_option_autoload_values(
array(
'my_option_1' => <em>true</em>,
'my_option_2' => <em>false</em>
)
);
]]>
</code>
<code title="Invalid: Using internal-only values.">
<![CDATA[
wp_set_option_autoload_values(
array(
'my_option_1' => <em>'auto-on'</em>,
'my_option_2' => <em>'auto-off'</em>
)
);
]]>
</code>
</code_comparison>
<standard>
<![CDATA[
Only `true` and `false` are valid values for the autoload parameter, with one exception: `null`
is also valid for `add_option()` and `update_option()`.
]]>
Copy link
Member

Choose a reason for hiding this comment

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

Again, the rule is the same as the previous two standard blocks, so for the purposes of documentation, these should be combined.

Also not so sure if the below code comparison adds value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In a separate commit, I tried again to structure the XML documentation. I kept the "introductory" standard block and the block for the presence of the autoload parameter. The other three blocks were combined into a single one that describes the autoload parameter's valid and invalid values. Any input on this new structure is welcome. I'm unsure if I should combine everything into a single <standard> and if I'm insisting too much on separating the parameter is missing warning from the other warnings.

Copy link
Member

@jrfnl jrfnl Mar 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I do think the "missing parameter" and the "invalid parameter value" issues are two separate issues, but I'm not convinced the changed structure of the docs makes this clear enough.

If I look at the latest version, my recommendation would be:

  • Move the last code_comparison to be directly under the first standard block (and drop the third standard block which was originally associated with that last code_comparison).

</standard>
<code_comparison>
<code title="Valid: Using `true`, `false`, or `null`.">
<![CDATA[
wp_set_options_autoload(
array( 'my_option_1', 'my_option_2' ),
<em>false</em>
);

wp_set_option_autoload(
'my_option',
<em>false</em>
);
]]>
</code>
<code title="Invalid: Using invalid values.">
<![CDATA[
wp_set_options_autoload(
array( 'my_option_1', 'my_option_2' ),
<em>1</em>
);

wp_set_option_autoload(
'my_option',
<em>null</em> // `null` is invalid for this function.
);
]]>
</code>
</code_comparison>
</documentation>
Loading
Loading