✨ New WordPress.WP.OptionAutoload sniff #2520

rodrigoprimo · 2025-02-26T23:27:54Z

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

It includes tests and sniff documentation in the XML format.

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.

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.

dingo-d · 2025-02-27T10:09:24Z

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?

jrfnl

@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.

WordPress/Sniffs/WP/OptionAutoloadSniff.php

jrfnl · 2025-02-28T02:19:03Z

WordPress/Sniffs/WP/OptionAutoloadSniff.php

+		$this->phpcsFile->recordMetric( $stackPtr, self::METRIC_NAME, 'param missing' );
+
+		// Only display a warning for the functions in which the `$autoload` parameter is optional.
+		if ( in_array( $function_name, self::$autoload_is_optional, true ) ) {


Function names are case-insensitive. Without the change in the docblock, it is not immediately apparent this will be handled correctly.

Please change the $autoload_is_optional array to 'name' => true format and check using isset() for a small performance improvement.
☝🏻 This second point should be fixed in quite a few places in this sniff.

Good point. I believe we discussed this already in the past and I failed to take it into account while creating this sniff. I will think a bit more about how to implement this in the other places and get back to you.

Please change the $autoload_is_optional array to 'name' => true format and check using isset() for a small performance improvement.
☝🏻 This second point should be fixed in quite a few places in this sniff.

I implemented this change in all the places that seemed relevant to me. For $valid_values_add_and_update and $valid_values_other_functions, I used the same format as what is used in the \PHPCSUtils\Tokens\Collections properties and repeated the value in the index of each element of the array. I opted to do this to avoid calling array_keys() when passing $valid_values to array_map():

WordPress-Coding-Standards/WordPress/Sniffs/WP/OptionAutoloadSniff.php

Lines 404 to 409 in fc98b8e

$valid_values = array_map(

function ( $value ) {

return '`' . $value . '`';

},

$valid_values

);

WordPress/Tests/WP/OptionAutoloadUnitTest.inc.fixed

jrfnl · 2025-02-28T02:58:03Z

WordPress/Tests/WP/OptionAutoloadUnitTest.inc.fixed

These tests could probably use some more variation in whitespace and comments/annotations in unexpected places.
Some more tests with function name case variations (especially for the "okay"/ignored tests) would also not go amiss.

I will create more tests and push a new commit in the next few days.

I don't think you need more tests. Probably just updating some of the existing tests to include those variations would be enough.

I updated some of the tests to add more variation in whitespace, comments/annotations in unexpected places, and function names using different cases.

Co-authored-by: Juliette <663378+jrfnl@users.noreply.github.com>

rodrigoprimo · 2025-02-28T21:10:02Z

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.

…he rest of the sniff code

This allows to use isset() instead of in_array()

rodrigoprimo · 2025-03-12T18:38:16Z

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

rodrigoprimo added Component: Extra Type: Enhancement labels Feb 26, 2025

jrfnl requested changes Feb 28, 2025

View reviewed changes

jrfnl added the Status: Awaiting feedback label Feb 28, 2025

rodrigoprimo and others added 2 commits February 28, 2025 17:47

Apply suggestions from code review

95afb67

Co-authored-by: Juliette <663378+jrfnl@users.noreply.github.com>

Attemp to improve the XML documentation by combining standards

a3da583

rodrigoprimo force-pushed the new-option-autoload branch from 9d70828 to a3da583 Compare February 28, 2025 20:47

rodrigoprimo mentioned this pull request Mar 6, 2025

Check if sniffs using AbstractFunctionParameterSniff are handling variable unpacking correctly #2521

Open

18 tasks

rodrigoprimo added 5 commits March 11, 2025 13:57

Fix unintentional coding standard violation in a test case

3aad7d3

Update some tests to have more function name case variations

571b0ed

Update some tests to have more whitespaces, comments and annotations

1575735

Use false === isset() instead of ! isset() for consistency with t…

4dbe72a

…he rest of the sniff code

Improve sniff performance by changing the format of some properties

fc98b8e

This allows to use isset() instead of in_array()

rodrigoprimo force-pushed the new-option-autoload branch from cd264e6 to fc98b8e Compare March 12, 2025 18:34

rodrigoprimo removed the Status: Awaiting feedback label Mar 12, 2025

	$valid_values = array_map(
	function ( $value ) {
	return '`' . $value . '`';
	},
	$valid_values
	);

Uh oh!

✨ New WordPress.WP.OptionAutoload sniff #2520

Are you sure you want to change the base?

✨ New WordPress.WP.OptionAutoload sniff #2520

Uh oh!

Conversation

rodrigoprimo commented Feb 26, 2025

Sniff behavior

Open questions

Uh oh!

dingo-d commented Feb 27, 2025

Uh oh!

jrfnl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jrfnl Feb 28, 2025

Choose a reason for hiding this comment

Uh oh!

rodrigoprimo Feb 28, 2025

Choose a reason for hiding this comment

Uh oh!

rodrigoprimo Mar 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jrfnl Feb 28, 2025

Choose a reason for hiding this comment

Uh oh!

rodrigoprimo Feb 28, 2025

Choose a reason for hiding this comment

Uh oh!

jrfnl Mar 1, 2025

Choose a reason for hiding this comment

Uh oh!

rodrigoprimo Mar 12, 2025

Choose a reason for hiding this comment

Uh oh!

rodrigoprimo commented Feb 28, 2025

Uh oh!

rodrigoprimo commented Mar 12, 2025

Uh oh!

Uh oh!