Make WordPress Core

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: sergeybiryukov's profile SergeyBiryukov Owned by: audrasjb's profile audrasjb
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)

50848.diff (625 bytes) - added by audrasjb 5 years ago.
Upgrade/Install: Clarify the usage of null for auto_update_{$type} filter
50848-alt.diff (3.8 KB) - added by audrasjb 4 years ago.
Upgrade/Install: use false instead null value in auto_update_{$type} filter
50848.1.diff (744 bytes) - added by audrasjb 4 years ago.
Upgrade/Install: Docs: Clarify the usage of null in auto_update_{$type} filter

Download all attachments as: .zip

Change History (22)

#1 @pbiron
5 years ago

  • Milestone changed from 5.6 to 5.5.1

Since this is just about documentation, moving to the 5.5.1 milestone.

@audrasjb
5 years ago

Upgrade/Install: Clarify the usage of null for auto_update_{$type} filter

#2 @audrasjb
5 years ago

  • Keywords has-patch added

#3 follow-up: @audrasjb
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 @SergeyBiryukov
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 than false. Thoughts?

If null is always interpreted as false here, could we always pass false instead then?

@audrasjb
4 years ago

Upgrade/Install: use false instead null value in auto_update_{$type} filter

#5 @audrasjb
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.

#6 @johnbillion
4 years ago

  • Version set to 3.7

#7 follow-up: @johnbillion
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 @pbiron
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

#11 @audrasjb
4 years ago

  • Keywords needs-testing added

#12 @pbiron
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

Last edited 4 years ago by pbiron (previous) (diff)

#13 @audrasjb
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 🍺

@audrasjb
4 years ago

Upgrade/Install: Docs: Clarify the usage of null in auto_update_{$type} filter

#14 @audrasjb
4 years ago

  • Keywords dev-feedback needs-testing needs-refresh removed

This ticket was mentioned in Slack in #core by desrosj. View the logs.


4 years ago

#16 @SergeyBiryukov
4 years ago

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

In 48909:

Docs: Clarify the usage of null for auto_update_{$type} filter.

The value is internally used to detect whether nothing has hooked into this filter.

Props audrasjb, pbiron, johnbillion, SergeyBiryukov.
Fixes #50848.

#17 @SergeyBiryukov
4 years ago

  • Keywords commit dev-feedback fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backporting to the 5.5 branch after a second committer's review.

#18 @desrosj
4 years ago

  • Keywords dev-reviewed added; dev-feedback removed

Looks good for backporting.

#19 @desrosj
4 years ago

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

In 48916:

Docs: Clarify the usage of null for auto_update_{$type} filter.

The value is internally used to detect whether nothing has hooked into this filter.

Props audrasjb, pbiron, johnbillion, SergeyBiryukov.
Merges [48909] to the 5.5 branch.
Fixes #50848.

Note: See TracTickets for help on using tickets.