#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: |
|
Owned by: |
|
---|---|---|---|
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.
- 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' );
- Set WP_DEBUG to true.
- 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)
Change History (27)
This ticket was mentioned in Slack in #core by jamesros161. View the logs.
4 years ago
#3
@
4 years 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:
↓ 5
@
4 years 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
@
4 years 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.
This ticket was mentioned in Slack in #core by jamesros161. View the logs.
4 years ago
This ticket was mentioned in Slack in #core-auto-updates by jamesros161. View the logs.
4 years ago
#10
@
4 years ago
Hey thanks for the patch @jamesros161.
Would be nice to replace else if
with elseif
for consistency with WordPress Sourcecode :)
#11
follow-up:
↓ 12
@
4 years 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:
↓ 13
@
4 years 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;
whenAUTOMATIC_UPDATER_DISABLED
is defined but false.
Right. The logic there is two-fold (afaik):
- If
AUTOMATIC_UPDATER_DISABLED
is defined OR if the'automatic_updater_disabled'
filter is used, the UI to set user preference should be disabled. - If
true === AUTOMATIC_UPDATER_DISABLED
ORtrue === apply_filters( 'automatic_updater_disabled', false )
, set$upgrade_dev = false; $upgrade_minor = false; $upgrade_major = false;
.
#13
in reply to:
↑ 12
;
follow-up:
↓ 15
@
4 years 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;
whenAUTOMATIC_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.
#14
@
4 years 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
andautomatic_updater_disabled
filter in update-core.php with a call toWP_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:
↓ 16
@
4 years 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
@
4 years 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/).
#17
@
4 years ago
- Keywords 2nd-opinion needs-testing removed
51827.2.patch looks good on my side as well, thanks 👍
#19
@
4 years ago
- Keywords 2nd-opinion commit added
- Resolution fixed deleted
- Status changed from closed to reopened
Reopen for the 5.6 branch.
#23
follow-up:
↓ 24
@
4 years ago
Nice work @azaozz . Wish I would have caught that in my patch so I could of gotten that 'Core Contributor' badge. lol
#24
in reply to:
↑ 23
@
4 years ago
Replying to jamesros161:
Nice work @azaozz
Thanks :)
I could of gotten that 'Core Contributor' badge.
You will, when WP 5.6 is released (I think). You're one of the contributors on this ticket and patch/changeset as mentioned in the "Props" on the commit messages.
Thanks again for opening this ticket, the nice, detailed description, and for the initial patch.
Screenshot of issue