WordPress.org

Make WordPress Core

Opened 9 days ago

Closed 6 days ago

#51827 closed defect (bug) (fixed)

When a plugin makes use of the 'automatic_updater_disabled' filter, it causes a PHP Warning to be thrown.

Reported by: jamesros161 Owned by: audrasjb
Milestone: 5.6 Priority: highest omg bbq
Severity: normal Version: 5.6
Component: Upgrade/Install Keywords: has-screenshots has-patch commit dev-reviewed
Focuses: Cc:

Description

When using a plugin that makes use of the 'automatic_updater_disabled' filter, the following PHP Warning is thrown:

PHP Warning:  Use of undefined constant AUTOMATIC_UPDATER_DISABLED - assumed 'AUTOMATIC_UPDATER_DISABLED' (this will throw an Error in a future version of PHP) in /var/www/html/wp56tu/wp-admin/update-core.php on line 327
PHP Stack trace:
PHP   1. {main}() /var/www/html/wp56tu/wp-admin/update-core.php:0
PHP   2. core_auto_updates_settings() /var/www/html/wp56tu/wp-admin/update-core.php:1008

Steps to Replicate.

  1. Install a plugin that makes use of the 'automatic_updater_disabled' filter, or add the following line to your theme's functions.php:
    add_filter( 'automatic_updater_disabled', '__return_false' );
    
  2. Set WP_DEBUG to true.
  3. Go to the Dashboard > Updates page, and see the error presented.

Suggested Resolution:

If the following changes are made to lines 324-337, this issue should be resolved.

Change This:

if ( defined( 'AUTOMATIC_UPDATER_DISABLED' )
                || has_filter( 'automatic_updater_disabled' )
        ) {
                if ( true === AUTOMATIC_UPDATER_DISABLED
                        /** This filter is documented in wp-admin/includes/class-wp-automatic-updater.php */
                        || true === apply_filters( 'automatic_updater_disabled', false )
                ) {
                        $upgrade_dev   = false;
                        $upgrade_minor = false;
                        $upgrade_major = false;
                }
                // The UI is overridden by the AUTOMATIC_UPDATER_DISABLED constant.
                $can_set_update_option = false;
        }

to:

if ( defined( 'AUTOMATIC_UPDATER_DISABLED' ) && true === AUTOMATIC_UPDATER_DISABLED
                || has_filter( 'automatic_updater_disabled' ) && true === apply_filters( 'automatic_updater_disabled', false )
        ) {
                $upgrade_dev   = false;
                $upgrade_minor = false;
                $upgrade_major = false;
        } else if ( defined( 'AUTOMATIC_UPDATER_DISABLED' ) || has_filter( 'automatic_updater_disabled' ) ) {
                // The UI is overridden by the AUTOMATIC_UPDATER_DISABLED constant.
                $can_set_update_option = false;
        }

Attachments (3)

Screenshot 2020-11-19 105145.png (19.9 KB) - added by jamesros161 9 days ago.
Screenshot of issue
51827.patch (1.3 KB) - added by jamesros161 9 days ago.
Proposed patch
51827.2.patch (1.8 KB) - added by azaozz 7 days ago.

Download all attachments as: .zip

Change History (25)

@jamesros161
9 days ago

Screenshot of issue

#1 @johnbillion
9 days ago

  • Milestone changed from Awaiting Review to 5.6

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


9 days ago

#3 @pbiron
9 days ago

@jamesros161 Welcome to Trac and thank you very much for the report.

Are you able to create a patch with your proposed changes? You can find info on how to do that at https://make.wordpress.org/core/handbook/tutorials/working-with-patches.

#4 follow-up: @audrasjb
9 days ago

  • Keywords needs-patch has-screenshots added
  • Owner set to audrasjb
  • Status changed from new to accepted

Hi, welcome to WordPress Trac and thank you for reporting this,

I am able to reproduce the issue using the steps provided by @jamesros161.

But at a glance, I think the following logic should be enough:

if ( defined( 'AUTOMATIC_UPDATER_DISABLED' ) && true === AUTOMATIC_UPDATER_DISABLED || has_filter( 'automatic_updater_disabled' ) && true === apply_filters( 'automatic_updater_disabled', false ) ) {
    $upgrade_dev   = false;
    $upgrade_minor = false;
    $upgrade_major = false;

    // The UI is overridden using a constant or a filter.
    $can_set_update_option = false;
}

I should be able to test it deeper later today.

#5 in reply to: ↑ 4 @jamesros161
9 days ago

Replying to audrasjb:

Hi, welcome to WordPress Trac and thank you for reporting this,

I am able to reproduce the issue using the steps provided by @jamesros161.

But at a glance, I think the following logic should be enough:

if ( defined( 'AUTOMATIC_UPDATER_DISABLED' ) && true === AUTOMATIC_UPDATER_DISABLED || has_filter( 'automatic_updater_disabled' ) && true === apply_filters( 'automatic_updater_disabled', false ) ) {
    $upgrade_dev   = false;
    $upgrade_minor = false;
    $upgrade_major = false;

    // The UI is overridden using a constant or a filter.
    $can_set_update_option = false;
}

I should be able to test it deeper later today.

The issue is, that in the original code, the $can_set_update_option is executed regardless of whether or not AUTOMATIC_UPDATER_DISABLED or apply_filters( 'automatic_updater_disabled') are true, as long as the constant is defined, and the filter exists.

#6 @audrasjb
9 days ago

OK, got it. Thanks for clarifying, you're right.

@jamesros161
9 days ago

Proposed patch

#7 @jamesros161
9 days ago

  • Keywords has-patch added; needs-patch removed

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


9 days ago

This ticket was mentioned in Slack in #core-auto-updates by jamesros161. View the logs.


9 days ago

#10 @audrasjb
9 days ago

Hey thanks for the patch @jamesros161.
Would be nice to replace else if with elseif for consistency with WordPress Sourcecode :)

#11 follow-up: @pbiron
9 days ago

Thanx for the patch James.

Taking a quick look, I think the original logic is incorrect regarding setting $can_set_update_option = false; when AUTOMATIC_UPDATER_DISABLED is defined but false.

That is, if I set that constant to false (and don't return true from automatic_updater_disabled) I think the enable/disable action links should be displayed.

So, the proposal by @audrasjb might be a better fix. @helen what do you think?

#12 in reply to: ↑ 11 ; follow-up: @azaozz
7 days ago

  • Keywords has-patch removed

Replying to pbiron:

Taking a quick look, I think the original logic is incorrect regarding setting $can_set_update_option = false; when AUTOMATIC_UPDATER_DISABLED is defined but false.

Right. The logic there is two-fold (afaik):

  1. If AUTOMATIC_UPDATER_DISABLED is defined OR if the 'automatic_updater_disabled' filter is used, the UI to set user preference should be disabled.
  2. If true === AUTOMATIC_UPDATER_DISABLED OR true === apply_filters( 'automatic_updater_disabled', false ), set $upgrade_dev = false; $upgrade_minor = false; $upgrade_major = false;.

#13 in reply to: ↑ 12 ; follow-up: @azaozz
7 days ago

Replying to azaozz:

Replying to pbiron:

Taking a quick look, I think the original logic is incorrect regarding setting $can_set_update_option = false; when AUTOMATIC_UPDATER_DISABLED is defined but false.

Right. The logic there is two-fold (afaik):

Hmm, actually no, that's incorrect (double negatives, lol).

The logic is/should be that if AUTOMATIC_UPDATER_DISABLED is set and false, it is equivalent to not being set, i.e. the updater is not disabled. Same for the 'automatic_updater_disabled' filter. So it should be checking only if any of the two is true (updater is disabled). Also the filter overrides the constant.

In addition, seems that wp_is_file_mod_allowed( 'automatic_updater' ) is not checked.

Thinking perhaps this should use WP_Automatic_Updater::is_disabled() instead of trying to duplicate the logic? It already uses the WP_Automatic_Updater to check for version control.

Last edited 7 days ago by azaozz (previous) (diff)

@azaozz
7 days ago

#14 @azaozz
7 days ago

  • Keywords has-patch 2nd-opinion needs-testing added
  • Priority changed from normal to highest omg bbq

In 51827.2.patch:

  • Replace the conditionals that check AUTOMATIC_UPDATER_DISABLED and automatic_updater_disabled filter in update-core.php with a call to WP_Automatic_Updater::is_disabled().
  • Also check wp_is_file_mod_allowed( 'automatic_updater' ) when disabling the UI.

Please test! :)

#15 in reply to: ↑ 13 ; follow-up: @pbiron
7 days ago

Replying to azaozz:

The logic is/should be that if AUTOMATIC_UPDATER_DISABLED is set and false, it is equivalent to not being set

That's what I meant by "the original logic is incorrect" because it does not do act like that.

Thinking perhaps this should use WP_Automatic_Updater::is_disabled() instead of trying to duplicate the logic? It already uses the WP_Automatic_Updater to check for version control.

YES: calling is_disabled() instead of (incorrectly) recreating most of it's logic is a great idea.

51827.2.patch tests correctly for me.

Note to others testing, if you have wp-admin/update-core.php loaded in your browser and you then make changes that cause wp_is_file_mod_allowed( 'automatic_updater' ) to return false and then reload in your browser you'll get a Sorry, you are not allowed to access this page. error from WP...that is to be expected, since the Updates screen is not allowed when file mods are disallowed :-)

#16 in reply to: ↑ 15 @azaozz
7 days ago

Replying to pbiron:

51827.2.patch tests correctly for me.

Thanks!

you'll get a Sorry, you are not allowed to access this page. error from WP...that is to be expected, since the Updates screen is not allowed when file mods are disallowed :-)

Right. Checking wp_is_file_mod_allowed() there is mostly for completeness. In theory a plugin would be able to use the file_mod_allowed filter and allow loading of the Update screen even when DISALLOW_FILE_MODS is defined and true, but disallow the actual update (see the #context param in https://developer.wordpress.org/reference/hooks/file_mod_allowed/).

Last edited 7 days ago by azaozz (previous) (diff)

#17 @audrasjb
6 days ago

  • Keywords 2nd-opinion needs-testing removed

51827.2.patch looks good on my side as well, thanks 👍

#18 @azaozz
6 days ago

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

In 49677:

Upgrade/Install: Replace the conditionals that check the AUTOMATIC_UPDATER_DISABLED constant and the automatic_updater_disabled filter in update-core.php with a call to WP_Automatic_Updater::is_disabled(). This prevents a PHP warning, fixes the logic, and considers wp_is_file_mod_allowed( 'automatic_updater' ) when determining the UI state.

Props jamesros161, pbiron, audrasjb, azaozz.
Fixes #51827.

#19 @azaozz
6 days ago

  • Keywords 2nd-opinion commit added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopen for the 5.6 branch.

#20 @SergeyBiryukov
6 days ago

In 49681:

Docs: Update syntax for multi-line comment in core_auto_updates_settings() per the documentation standards.

Follow-up to [49677].

See #51827.

#21 @SergeyBiryukov
6 days ago

  • Keywords dev-reviewed added; 2nd-opinion removed

[49677] looks good to backport.

#22 @SergeyBiryukov
6 days ago

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

In 49682:

Upgrade/Install: Replace the conditionals that check the AUTOMATIC_UPDATER_DISABLED constant and the automatic_updater_disabled filter in update-core.php with a call to WP_Automatic_Updater::is_disabled().

This prevents a PHP warning, fixes the logic, and considers wp_is_file_mod_allowed( 'automatic_updater' ) when determining the UI state.

Props jamesros161, pbiron, audrasjb, azaozz.
Merges [49677] and [49681] to the 5.6 branch.
Fixes #51827.

Note: See TracTickets for help on using tickets.