WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 22 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: has-patch needs-testing dev-feedback 3.2-early
Focuses: 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 5 years ago.
16156.diff (4.6 KB) - added by dd32 4 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 @Denis-de-Bernardy5 years ago

  • Cc Denis-de-Bernardy added

comment:2 @garyc405 years ago

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

comment:3 in reply to: ↑ description ; follow-up: @solarissmoke5 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.

comment:4 @solarissmoke5 years ago

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

comment:5 in reply to: ↑ 3 @SergeyBiryukov5 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/.

comment:6 @SergeyBiryukov5 years ago

Seems to work fine otherwise.

comment:7 follow-up: @nacin5 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.

comment:8 in reply to: ↑ 7 @solarissmoke5 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

comment:9 @solarissmoke5 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.

comment:10 @nacin5 years ago

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

comment:11 @solarissmoke5 years ago

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

@solarissmoke5 years ago

comment:12 @nacin5 years ago

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

comment:13 follow-up: @dd324 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?

comment:14 @dd324 years ago

Closed #14204 as duplicate of this

comment:15 in reply to: ↑ 13 @solarissmoke4 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.

comment:16 follow-up: @dd324 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..

@dd324 years ago

comment:17 @dd324 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..

comment:18 in reply to: ↑ 16 @solarissmoke4 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.

comment:19 @dd324 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 ) ) {

comment:20 @dd3222 months ago

#25792 was marked as a duplicate.

Note: See TracTickets for help on using tickets.