WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#18876 closed defect (bug) (fixed)

Network admin update-core.php doesn't check for plugin or theme updates

Reported by: johnbillion Owned by: ryan
Milestone: 3.4 Priority: normal
Severity: normal Version: 3.3
Component: Network Admin Keywords: has-patch early
Focuses: multisite Cc:

Description

Visiting wp-admin/update-core.php on a single site install or on the main site of a network install will fire off update checks for everything -- core, plugins and themes.

Visiting this screen within network admin (wp-admin/network/update-core.php) only checks for updates to core. I believe this is unexpected behaviour based on discussion in #14764 and it's inconsistent with the behaviour of the screen on the main site or a single site installation.

The problem occurs because wp_update_plugins|themes() are only added to the load-update-core.php hook when is_main_site() is true.

Attachments (3)

18876.patch (416 bytes) - added by johnbillion 3 years ago.
18876.diff (4.8 KB) - added by nacin 2 years ago.
18876.short-cache.patch (5.2 KB) - added by kurtpayne 2 years ago.
Short lived cache of updated responses

Download all attachments as: .zip

Change History (31)

johnbillion3 years ago

comment:1 johnbillion3 years ago

  • Keywords has-patch added

Patch

comment:2 johnbillion3 years ago

Hmm. There's a check inside wp_update_plugins() and wp_update_themes() which prevents the checks from happening if a check was performed within the last 12 hours and no plugins/themes have changed. This check is also present in _maybe_update_plugins|themes() so surely it's redundant?

comment:3 johnbillion3 years ago

  • Keywords needs-patch added; has-patch removed

comment:4 nacin3 years ago

  • Milestone changed from Awaiting Review to 3.2.2

Interesting. Normally, the network admin is only available from is_main_site(), but it's technically $site->domain . $site.path, which may not be the case.

No objections to the patch at a glance, then, though we use early logical operators rather than the ones with later precedence. http://www.php.net/manual/en/language.operators.precedence.php

comment:5 johnbillion3 years ago

My patch doesn't fix anything. I misunderstood is_main_site() (thanks nacin).

It seems there's some logic in wp_update_plugins() which should be in _maybe_update_plugins() instead. It's not possible to force a check for plugin updates (same goes for themes).

This means plugin and theme update checks are inconsistent with core update checks. I would expect wp_update_plugins() to work the same as wp_version_check(), in that it fires off an update check whenever it's called. The logic should be left to the _maybe_update_* functions.

comment:6 follow-up: nacin3 years ago

It's not possible to force a check for plugin updates (same goes for themes).

It should be. Especially since we have a "Check Now" button.

My patch doesn't fix anything. I misunderstood is_main_site() (thanks nacin).

It does, in the rare situation where the main site is not the network admin.

comment:7 in reply to: ↑ 6 johnbillion3 years ago

Replying to nacin:

It's not possible to force a check for plugin updates (same goes for themes).

It should be. Especially since we have a "Check Now" button.

Agreed.

My patch doesn't fix anything. I misunderstood is_main_site() (thanks nacin).

It does, in the rare situation where the main site is not the network admin.

Ah yes, in that situation it does cause wp_update_plugins() to fire, however the problem remains that wp_update_plugins() doesn't actually fire an update reeuest each time it's called.

comment:8 scribu3 years ago

  • Cc scribu added

comment:9 ryan3 years ago

Let's fix the main site not matching network admin case and punt the rest.

comment:10 nacin2 years ago

I think we need a is_network_admin_site() function. (Lame name, to be fair.) I see a few situations where is_main_site() is used in a situation where we really want to know what blog is the network admin root.

This can then be filtered potentially reusing the filter in wp-admin/network/admin.php.

comment:11 nacin2 years ago

In [19021]:

Attach update hooks in the network admin. Accounts for the rare situation where the network admin is not the main site. props johnbillion, see #18876.

comment:12 nacin2 years ago

In [19022]:

Check for plugin/theme updates every hour when hitting update-core.php, not just themes.php/plugins.php. see #18876.

comment:13 follow-up: nacin2 years ago

I noticed a few days ago that there was code missing to refresh the plugin/theme updates as often on update-core.php as we do on plugins.php/themes.php. [19022] corrects this.

It's worth noting that this code will also fire on network/themes.php and network/plugins.php as the same load-* hooks fire. (Which I'm not sure I like.)

Thoughts on this? I actually think that update-core.php's timeout should be 0. It makes sense to not fire it off every time someone goes to do general theme or plugin management, but when you click "Check Again", you should get up-to-the-minute results. I'd think that update.php might want to get the same treatment.) This seems easy to implement now (just a matter of changing the logic changed in [19022]).

comment:14 scribu2 years ago

It makes sense to not fire it off every time someone goes to do general theme or plugin management, but when you click "Check Again", you should get up-to-the-minute results. I'd think that update.php might want to get the same treatment.)

Agreed.

nacin2 years ago

comment:15 nacin2 years ago

18876.diff optimizes how the code is branched. update-core.php now always refreshes plugins/themes.

comment:16 follow-up: scribu2 years ago

The lines below // Check for updated [...] depending on the page. seem to be duplicated.

Also, couldn't we check WP_Screen instead of current_filter()?

Version 0, edited 2 years ago by scribu (next)

comment:17 in reply to: ↑ 16 nacin2 years ago

Replying to scribu:

There seem to be two identical // Check for updated [...] depending on the page. blocks.

One for plugins, one for themes. Abstracting this further needs a lot more work.

Also, couldn't we check WP_Screen instead of current_filter()?

We could.

comment:18 in reply to: ↑ 13 kurtpayne2 years ago

  • Cc kpayne@… added

Replying to nacin:

when you click "Check Again", you should get up-to-the-minute results.

I'd advocate for a short-lived cache (e.g. 30 seconds). There are several scenarios where this page could be requested multiple times in a short span.

Consider clients that pre-fetch content, or re-fetch content (extensions, scripts). Or possibly a plugin mistake that contains a self-reference error (example below) that would cause the browser to fetch the same URL again:

add_action( 'admin_notices', function() {echo '<img src="?" />';} );

This may easily save some traffic for api.wordpress.org in some edge cases.

comment:19 nacin2 years ago

Short-lived makes sense, especially considering the race conditions already baked into the code.

comment:21 nacin2 years ago

Well then.

comment:22 nacin2 years ago

In [19312]:

Revert update changes that snuck in with [19311]. see #19255, see #18876.

comment:23 nacin2 years ago

  • Keywords has-patch added; needs-patch removed

Just need to tweak the existing patch for the short-lived timeouts.

kurtpayne2 years ago

Short lived cache of updated responses

comment:24 ryan2 years ago

  • Keywords early added
  • Milestone changed from 3.2.2 to Future Release

comment:25 nacin2 years ago

Happy with this for 3.3. We'll make it shorter-lived in 3.4.

comment:26 ryan2 years ago

  • Milestone changed from Future Release to 3.4

comment:27 ryan2 years ago

  • Owner set to ryan
  • Resolution set to fixed
  • Status changed from new to closed

In [19683]:

Check for plugin and theme updates more often when visitng update-core.php. Props kurtpayne. fixes #18876

comment:28 duck_2 years ago

[20286] to fix a copy and paste error.

Note: See TracTickets for help on using tickets.