Make WordPress Core

Opened 5 months ago

Closed 5 months ago

Last modified 5 months ago

#61929 closed defect (bug) (fixed)

Update documentation for remaining autoload functions to only include `true|false` as parameter values

Reported by: flixos90's profile flixos90 Owned by: flixos90's profile flixos90
Milestone: 6.7 Priority: normal
Severity: normal Version:
Component: Options, Meta APIs Keywords: has-patch
Focuses: docs, performance Cc:

Description

As of WordPress 6.6, add_option() and update_option() should only be used with the boolean values true and false for the $autoload parameter (plus the special value null). The older values 'yes' and 'no' are supported for backward compatibility, but no longer encouraged.

There are other autoloading related functions that still have 'yes' and 'no' documented. Those should be updated to only recommend the boolean values (without actually dropping support, as we need to remain backward compatible).

Change History (17)

#1 @flixos90
5 months ago

  • Focuses docs added

This ticket was mentioned in PR #7250 on WordPress/wordpress-develop by @flixos90.


5 months ago
#2

  • Keywords has-patch added; needs-patch removed

This PR only updates documentation.

Trac ticket: https://core.trac.wordpress.org/ticket/61929

#3 follow-up: @joemcgill
5 months ago

I think this change makes sense, and aligns with the intent of [57920] and what was communicated in the associated Dev Note post. The argument against is that the function technically does support string values for those parameters for backwards compatibility.

Do you know if there other places where we intentionally limit the param values in docs to exclude values that are only supported for backwards compatibility purposes?

#4 in reply to: ↑ 3 @flixos90
5 months ago

Replying to joemcgill:

The argument against is that the function technically does support string values for those parameters for backwards compatibility.

While that's true, I think it's better to focus documentation on how the functions should be used. Explaining backward compatibility in parameter documentation is unnecessary detail to someone that intends to use the function and can lead to confusion. IMO backward compatibility is about avoiding breakage in the functionality, less about documentation. For instance, if we were now type-hinting bool in the actual parameters, that would be a functional change, but changing only the documentation won't cause breakage. If anything, it will lead to certain linting tools (e.g. PHPStan) complaining, which I think is something that we would want.

Do you know if there other places where we intentionally limit the param values in docs to exclude values that are only supported for backwards compatibility purposes?

I'm not sure off the top of my head. But I'm also not sure there are places where we explicitly document them, unless they're entire parameters that got (soft or hard) deprecated.

#5 @joemcgill
5 months ago

If anything, it will lead to certain linting tools (e.g. PHPStan) complaining, which I think is something that we would want.

I agree that this could help people adopt the new boolean values. Given that you're only targeting the two new functions that were added in 6.4, I think this makes a lot of sense, before they are widely adopted. We do include more context and a @since note on the changes we made to the more widely used functions, like add_option(), which you can see here. I think something similar would be valuable, otherwise I'm +1 this change.

#6 @flixos90
5 months ago

@joemcgill Based on your feedback, I have updated https://github.com/WordPress/wordpress-develop/pull/7250 to include notes on 'yes'|'no' being accepted for backward compatibility. This way they're still documented, but clearly just for BC. This is in line with how it's done for add_option().

I don't think we need a @since annotation for this, as there's no functional change being made.

Last edited 5 months ago by flixos90 (previous) (diff)

#7 follow-up: @jrf
5 months ago

Might it be an idea to official mark those old values as deprecated ? Either soft deprecated (documentation only) or hard deprecated and flagged via a call to the `_deprecated_argument()` function ?

And yes, there is precedent for this in WP, not just for a deprecated argument, but also for passing a specific argument value to an argument. See: https://github.com/WordPress/WordPress-Coding-Standards/blob/dfd6c3a8ee61eed6af15bda09f4a33d79eb2b86b/WordPress/Sniffs/WP/DeprecatedParameterValuesSniff.php#L67-L209 (the list in the sniff is based on calls to the _deprecated_argument() function within WP for deprecated argument values)

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


5 months ago

#9 in reply to: ↑ 7 ; follow-up: @flixos90
5 months ago

Replying to jrf:

Might it be an idea to official mark those old values as deprecated ? Either soft deprecated (documentation only) or hard deprecated and flagged via a call to the `_deprecated_argument()` function ?

Could you clarify what you think soft deprecation could look like in this context? The documentation already states that these values are only supported for backward compatibility.

Regarding hard deprecation, I don't think we should do that at this point. From a code perspective it would make sense, but historically we haven't hard-deprecated things when they're still very commonly used in the ecosystem and are easy to maintain (both of which applies here).

#10 @flixos90
5 months ago

In 58945:

Options, Meta APIs: Stop using 'yes' and 'no' for autoload parameter in favor of recommended boolean.

This changeset does not modify any behavior, it only updates the code to use the recommended type for the $autoload parameter as of WordPress 6.6. The old values 'yes' and 'no' are only maintained in certain tests that are explicitly about these backward compatibility values.

Props flixos90, joemcgill, mukesh27.
Fixes #61939.
See #61103, #61929.

#11 in reply to: ↑ 9 @jrf
5 months ago

Replying to flixos90:

Replying to jrf:

Might it be an idea to official mark those old values as deprecated ? Either soft deprecated (documentation only) or hard deprecated and flagged via a call to the `_deprecated_argument()` function ?

Could you clarify what you think soft deprecation could look like in this context? The documentation already states that these values are only supported for backward compatibility.

Happy to. Soft deprecation is a deprecation in documentation only, but does make the deprecation explicit (currently it is implied).

In practice, this would mean something along the lines of:

  • Adding a @since tag (or maybe even a @deprecated tag, this would need to be checked against prior art, but I seem to remember it should be @since) stating that passing 'yes|no' to the $autoload parameter is deprecated.
  • Probably updating the phrasing in the parameter description from "For backward compatibility 'yes' and 'no' are also accepted. <snip> ... it is recommended to autoload them, by using 'yes'|true." to something along the lines of "For backward compatibility, 'yes' and 'no' are still accepted, though using these values is deprecated. <snip> ... it is recommended to autoload them, by using true." (note the removal of the deprecated value from the recommendation)

Regarding hard deprecation, I don't think we should do that at this point. From a code perspective it would make sense, but historically we haven't hard-deprecated things when they're still very commonly used in the ecosystem and are easy to maintain (both of which applies here).

Fair point. Maybe a "Future release" ticket should be opened as a reminder to do this in WP 7.0 (or later) ?

The exact "when" could possibly be determined based on stats from the plugin directory. "If more than 50% of the relevant function calls in plugins use the true|false|null values, hard deprecate the soft deprecated values" (or something like that).

The sniff which is being worked on, could include "metrics" (a stats feature) which would make it straight forward to get such statistics about the values being passed to $autoload using PHPCS with the --report=info flag.

#12 @flixos90
5 months ago

@jrf Based on your suggestion, I have updated the PR https://github.com/WordPress/wordpress-develop/pull/7250 to more clearly soft-deprecate the values.

I went with a @since annotation (including this for add_option() and update_option() where this didn't formally happen during the 6.6 cycle). I've also indicated in the parameter docs throughout that using these values is deprecated.

I think this brings all 5 functions in line with each other in that they still support these values, but equally document that using them is deprecated. Please feel free to take a look.

@jrf commented on PR #7250:


5 months ago
#13

One question (though this doesn't change my approval): is the @since 6.7.0 correct or should this be @since 6.6.0 ?
After all, this is a documentation change and it could be argued that the changes in WP 6.6 implicitly deprecated the values, but that the documentation wasn't updated to match and that this PR corrects that oversight.

@flixos90 commented on PR #7250:


5 months ago
#14

One question (though this doesn't change my approval): is the @since 6.7.0 correct or should this be @since 6.6.0 ? After all, this is a documentation change and it could be argued that the changes in WP 6.6 implicitly deprecated the values, but that the documentation wasn't updated to match and that this PR corrects that oversight.

I thought about this too, but think it makes more sense to have this aligned to 6.7.0, as only now we're really thinking about this. There's no functional change in any of those places, but it still feels more appropriate to mark this only deprecated as of now, rather than retroactively. Also, core still uses 'yes'|'no' in a few places as of 6.6, so that would not be a good state to be in if they had been deprecated already. :)

#15 @flixos90
5 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 58949:

Options, Meta APIs: Soft-deprecate use of 'yes' and 'no' as $autoload parameter.

WordPress 6.6 option autoload enhancements included discouraging the use of 'yes' and 'no' to indicate whether to autoload an option when calling add_option() or update_option(). Instead, a boolean should be used.

This changeset brings the newer autoload related functions wp_set_option_autoload_values(), wp_set_options_autoload(), and wp_set_option_autoload() in line with those changes. Additionally, it soft-deprecates the values more formally, as they should no longer be used. No PHP warnings will be emitted though as this is not a hard deprecation. This change is purely about documentation.

Props flixos90, joemcgill, jrf, mukesh27.
Fixes #61929.
See #61103, #61939.

@jrf commented on PR #7250:


5 months ago
#17

One question (though this doesn't change my approval): is the @since 6.7.0 correct or should this be @since 6.6.0 ? After all, this is a documentation change and it could be argued that the changes in WP 6.6 implicitly deprecated the values, but that the documentation wasn't updated to match and that this PR corrects that oversight.

I thought about this too, but think it makes more sense to have this aligned to 6.7.0, as only now we're really thinking about this. There's no functional change in any of those places, but it still feels more appropriate to mark this only deprecated as of now, rather than retroactively.

I defer to you as the author of the underlying changes. Just wanted to make sure this had been considered.

Also, core still uses 'yes'|'no' in a few places as of 6.6, so that would not be a good state to be in if they had been deprecated already. :)

That's not actually an issue with a soft deprecation, though will need to be fixed before a hard deprecation happens (though I seem to remember seeing a commit fixing these already).

Note: See TracTickets for help on using tickets.