#61929 closed defect (bug) (fixed)
Update documentation for remaining autoload functions to only include `true|false` as parameter values
Reported by: | flixos90 | Owned by: | 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)
This ticket was mentioned in PR #7250 on WordPress/wordpress-develop by @flixos90.
5 months ago
#2
- Keywords has-patch added; needs-patch removed
#3
follow-up:
↓ 4
@
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
@
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
@
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
@
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.
#7
follow-up:
↓ 9
@
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:
↓ 11
@
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).
#11
in reply to:
↑ 9
@
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
@
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.
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. :)
@flixos90 commented on PR #7250:
5 months ago
#16
Committed in https://core.trac.wordpress.org/changeset/58949
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).
This PR only updates documentation.
Trac ticket: https://core.trac.wordpress.org/ticket/61929