Opened 5 years ago
Closed 4 years ago
#50848 closed defect (bug) (fixed)
Clarify the usage of null for "auto_update_{$type}" filter
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 5.5.1 | Priority: | normal |
Severity: | normal | Version: | 3.7 |
Component: | Upgrade/Install | Keywords: | has-patch commit dev-reviewed fixed-major |
Focuses: | docs | Cc: |
Description
The first parameter of the auto_update_{$type}
filter is documented as bool
:
/** * ... * @param bool $update Whether to update. * @param object $item The update offer. */ $update = apply_filters( "auto_update_{$type}", $update, $item );
In a few instances, as auto_update_plugin
or auto_update_theme
, it also receives null
.
The usage of null
should be clarified in the docs.
Attachments (3)
Change History (22)
#3
follow-up:
↓ 4
@
5 years ago
I don't think there is anything else to update than the expected types for this param. It looks pretty clear to me that null
value has the same result than false
. Thoughts?
#4
in reply to:
↑ 3
@
5 years ago
Replying to audrasjb:
I don't think there is anything else to update than the expected types for this param. It looks pretty clear to me that
null
value has the same result thanfalse
. Thoughts?
If null
is always interpreted as false
here, could we always pass false
instead then?
#5
@
4 years ago
- Keywords dev-feedback added
- Owner set to audrasjb
- Status changed from new to accepted
Good point @SergeyBiryukov.
In 50848-alt.diff
, I replaced null
values with false
and I updated the related conditional statements.
#7
follow-up:
↓ 9
@
4 years ago
Can a plugin return boolean false from this filter to prevent an auto update?
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
4 years ago
#9
in reply to:
↑ 7
@
4 years ago
Replying to johnbillion:
Can a plugin return boolean false from this filter to prevent an auto update?
Yes, that is the intention of the filter.
This ticket was mentioned in Slack in #core-auto-updates by audrasjb. View the logs.
4 years ago
#12
@
4 years ago
Following up from the #core-auto-updates meeting this morning, it seems to me that in all of the places where null
is passed, passing false
would be wrong.
For example, looking at wp-admin/includes/class-wp-plugins-list-table.php, L236-240:
$auto_update_forced = apply_filters( "auto_update_{$type}", null, $filter_payload );
if ( ! is_null( $auto_update_forced ) ) {
$plugin_data['auto-update-forced'] = $auto_update_forced;
}
The intention of that code is to detect whether anything has hooked into that filter and is returning a truthy or falsey value...so that the UI can display "Auto-updates enabled" or "Auto-updates disabled" as plain text instead of displaying the enable/disable links. Hence, passing false
would be the wrong thing to do there.
The uses in site health are similar: if the filter returns a truthy or falsey value (instead of null
) then that value overrides the user preference stored (or not) in the site option when constructing the text for whether the plugin/theme will auto-update. And so, passing false
there would be wrong as well.
And the use in single-site themes.php is also the same.
So, I think the code needs to stay as it is and the DocBlock for the filter needs to be updated to change the type of the $update
parameter to bool|null
and to describe under what circumstances null will be passed.
I should also point out that the potential complications of using these filters for other than their originally intended purpose (which is to decide, at the time auto-updates are about to be done, whether they should be done or not) was anticipated and discussed, for instance in the #core-auto-updates meeting on May 5, 2020.
In particular, the comments from @azaozz :
still feeling somewhat.. "unconfortable" doing that though as it will use an existing filter that is pretty popular (used in many plugins) and the new use is for the UI, totally unexpected
and
But the use for the UI will definitely be unexpected, so there will likely be edge cases
#13
@
4 years ago
- Keywords needs-refresh added
Wow… you're absolutely right, thank you for investigating in the project history.
It sounds absolutely clear to me now. I'll update the DocBlocks to provide some context about usage of null
.
Thanks 🍺
Since this is just about documentation, moving to the 5.5.1 milestone.