WordPress.org

Make WordPress Core

Opened 6 years ago

Last modified 11 months ago

#16156 new defect (bug)

update-core is oblivious to api.wp.org being unreachable

Reported by: nacin Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Upgrade/Install Keywords: needs-testing dev-feedback needs-refresh
Focuses: administration Cc:

Description

A server running 3.0.4 had trouble reaching *.wordpress.org for some unknown reason. (This wasn't during the brief api.wp.org outage, and it worked otherwise.)

Problem is, update-core was completely oblivious to this. It's even worse when running 3.1, because the 'Check Again' button will refresh the page and tell you we last checked for updates *just now*.

Try Again or Check Again should reflect that the API is unreachable when this can be determined. Claiming that we checked for updates is also really lame, so we should see if the transient tells us how long it's been since the last one.

Side note, the plugin install et al. HTTP error messages are rather cryptic, and we should also make those more user friendly.

Attachments (2)

16156.patch (5.4 KB) - added by solarissmoke 6 years ago.
16156.diff (4.6 KB) - added by dd32 5 years ago.

Download all attachments as: .zip

Change History (23)

#1 @Denis-de-Bernardy
6 years ago

  • Cc Denis-de-Bernardy added

#2 @garyc40
6 years ago

Version 0, edited 6 years ago by garyc40 (next)

#3 in reply to: ↑ description ; follow-up: @solarissmoke
6 years ago

I thought I would give this a crack.

Here's a patch - it allows us to pass error information from wp_version_check() to the user.

Regarding "last checked for updates" - this is a bit pointless, because visiting wp-admin/update-core.php triggers wp_version_check(), meaning WP will check for updates every time you load the page. Similarly, offering to "check again" immediately after we've just checked seems pointless. So I've removed it altogether. Only time it offers to try again is if there was an error.

#4 @solarissmoke
6 years ago

  • Keywords has-patch needs-testing dev-feedback added

#5 in reply to: ↑ 3 @SergeyBiryukov
6 years ago

Replying to solarissmoke:

Regarding "last checked for updates" - this is a bit pointless, because visiting wp-admin/update-core.php triggers wp_version_check(), meaning WP will check for updates every time you load the page. Similarly, offering to "check again" immediately after we've just checked seems pointless. So I've removed it altogether.

That was introduced in #14764 and is still needed, I believe. Also, s/Wordpress/WordPress/.

#6 @SergeyBiryukov
6 years ago

Seems to work fine otherwise.

#7 follow-up: @nacin
6 years ago

The Check Again message needs to change, as it would otherwise be inaccurate.

Basically, the message should mention the failure, probably when it was last successful, and a Try Again button.

I can get Jane to weigh in on complete UX and strings at a later point.

#8 in reply to: ↑ 7 @solarissmoke
6 years ago

Replying to nacin:

The Check Again message needs to change, as it would otherwise be inaccurate.

We could replace it with a "check for new updates" message or something.

Basically, the message should mention the failure, probably when it was last successful, and a Try Again button.

We don't store last successful information at present. last_checked on the transient is refreshed *before* sending the HTTP request, so doesn't help us here

#9 @solarissmoke
6 years ago

Also, I still don't think saying when it was last successful really helps - because unless WP can connect to wp.org right now, updating isn't going to work anyway. And if it did manage to connect, then the last success was, well, now.

#10 @nacin
6 years ago

That's probably to prevent race conditions. I would not mind figuring out a way to handle that separately.

#11 @solarissmoke
6 years ago

Added "check for new updates" link when Wordpress reports that it is up to date.

@solarissmoke
6 years ago

#12 @nacin
6 years ago

  • Keywords 3.2-early added
  • Milestone changed from Awaiting Review to Future Release

#13 follow-up: @dd32
5 years ago

I had independently started on this, and changed it based on a "last_successful" field instead, and displaying a message if the last successful update check was >24hrs ago.

Perhaps we need some goals here on what the user should be shown when an update fails to decide which line of attack is used here?

#14 @dd32
5 years ago

Closed #14204 as duplicate of this

#15 in reply to: ↑ 13 @solarissmoke
5 years ago

I had independently started on this, and changed it based on a "last_successful" field instead, and displaying a message if the last successful update check was >24hrs ago.

This sounds like a good idea.

Perhaps we need some goals here on what the user should be shown when an update fails to decide which line of attack is used here?

Currently, there is no way to force an update check - you have to wait till the update_core transient expires (or delete it manually). And anyway, the transient doesn't store the last successful check, so it could have been ages and you wouldn't know. The "Last checked on... " message just prints the current time and is totally misleading.

I propose something like this for update-core.php:

  • Display the last successful check time (by implementing a last_successful value).
  • If the last check offered no updates, present the option for the admin to check again now (possibly with some restriction on doing this too frequently)
  • If they choose to check again now, and the check fails, then we tell them it failed and why (and offer to try again). Otherwise we update the results accordingly.

#16 follow-up: @dd32
5 years ago

Currently, there is no way to force an update check

For core, every time wp_version_check() is called, an update request is made. Whilst the update-core.php page does just print the current date/time, it does so because an update was literally just made. Plugins and Themes are a different matter.

What I'm thinking is replacing the current update paragraph with something which has a icon (green/red) with a label of the current status, for example:

  • GREEN: You're up to date, The last update check occured at <time> (check again)
  • RED: The last successful update was over 24hours ago (2:34pm 1/2/2011), This means that WordPress is encountering an error whilst performing the update check, The error being reported is <HTTP Error Message>. Please see this Codex page to learn how to correct this. (try again)

I thought it'd be best to keep it green if an error occurred under 24hrs, It could be a network fault or server fault somewhere, if it's a continued issue however, switch to error condition mode and get the user to read the codex suggesting potential causes and what to remedy it with..

@dd32
5 years ago

#17 @dd32
5 years ago

attachment 16156.diff added

The changes I have locally. Note, that this includes some debugging (API change to localhost) as well as potential bug fixes ( see #14384 ), as well as me forcing it to always report an error condition (decrementing the last successful time by 30hrs)

I havn't attempted to style it well or anything like that, mainly just as a POC of something, It doesn't have the GREEN/RED styling ether, just the default WordPress Alert Error div for the error condition..

#18 in reply to: ↑ 16 @solarissmoke
5 years ago

For core, every time wp_version_check() is called, an update request is made.

Ok, my mistake. I like the patch :) but one small issue:

If the admin clicks "Check Again", and the check fails, they will only get an error message if the last successful check was more than 24 hours ago. If it wasn't, they will get "last checked on [last successful time], which makes them think that asking to check again didn't actually do anything.

#19 @dd32
5 years ago

Like I said, it was a Proof of concept more than anything else.

That could be handled something such as this:

if ( $current->last_success > time()-24*60*60 && ( ! isset( $_GET['check-again'] ) || $current->last_success == $current->last_checked ) ) {

#20 @dd32
3 years ago

#25792 was marked as a duplicate.

#21 @chriscct7
11 months ago

  • Focuses administration added
  • Keywords needs-refresh added; has-patch 3.2-early removed

Some minor formatting issues, mostly lack of space between ' and ) in the translated strings at the end.

Note: See TracTickets for help on using tickets.