WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#16132 closed defect (bug) (fixed)

Duplicate "try again" links when themes_api fails

Reported by: solarissmoke Owned by:
Milestone: 3.1 Priority: normal
Severity: minor Version: 3.1
Component: Themes Keywords:
Focuses: Cc:

Description

When the Themes API fails, for example when searching for themes (in my case because of timed out connection to wp.org, two "Try again" links are presented to the user.

The issue is with this line in WP_Theme_Install_List_Table->prepare_items()

wp_die( $api->get_error_message() . '</p> <p class="hide-if-no-js"><a href="#" onclick="document.location.reload(); return false;">' . __( 'Try again' ) . '</a>' );

$api->get_error_message() already returns a try again link ( set by themes_api() ), but this line adds another one in addition. I suggest removing the latter? Also, there are no CSS rules of .hide-if-no-js on Error pages, so it doesn't do anything.

Attachments (2)

16132.patch (1.6 KB) - added by solarissmoke 3 years ago.
Don't print duplicate "try again" links when themes_api fails
16132-more.diff (762 bytes) - added by solarissmoke 3 years ago.
... and show more useful error information

Download all attachments as: .zip

Change History (15)

comment:1 solarissmoke3 years ago

  • Keywords has-patch added

comment:2 follow-ups: nacin3 years ago

.hide-if-no-js is in global.dev.css.

Is this is a regression?

comment:3 in reply to: ↑ 2 solarissmoke3 years ago

Replying to nacin:

.hide-if-no-js is in global.dev.css.

But the error page only links tp wp-admin/css/install.css (see _default_wp_die_handler() ) - I think this has always been the case.

Also, my original suggestion that we should remove the second "try again" link is wrong, because themes_api can return two different errors. I've changed the patch. As far as I can see this shouldn't negatively affect other calls to themes_api().

Version 0, edited 3 years ago by solarissmoke (next)

solarissmoke3 years ago

Don't print duplicate "try again" links when themes_api fails

comment:4 in reply to: ↑ 2 SergeyBiryukov3 years ago

Replying to nacin:

Is this is a regression?

Yep. Tested on wp-admin/theme-install.php?tab=featured screen: 3.0.4 didn't use wp_die() here and there was only one “Try Again” link.

comment:5 nacin3 years ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to 3.1

comment:6 nacin3 years ago

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

(In [17264]) Leave the 'Try Again' link for the wp_die() when the themes API fails. props solarissmoke, fixes #16132.

comment:7 nacin3 years ago

Spoke with dd32 who added the original code, and he agreed that removing it from the API abstraction layer would be a better choice, and keep it with the wp_die().

comment:8 solarissmoke3 years ago

  • Keywords has-patch commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Hmm, I think this needs more work. Now, when themes_api fails we get "An Unexpected HTTP Error occurred during the API request." but not the error from wp_remote_post() which actually has the more meaningful information about what went wrong.

Knocking up a patch now.

Last edited 3 years ago by solarissmoke (previous) (diff)

comment:9 solarissmoke3 years ago

  • Keywords has-patch dev-feedback added

solarissmoke3 years ago

... and show more useful error information

comment:10 solarissmoke3 years ago

Alternatively, we might do away with the "unexpected HTTP error" bit altogether (it isn't particularly helpful) and just pass on the wp_remote_post error:

$res = new WP_Error( 'themes_api_failed', $request->get_error_message() );

comment:11 follow-up: dd323 years ago

IMO, We leave it as-is for the 3.1 release and revisit in 3.2 with the intention of cleaning up some of the update ui's

comment:12 in reply to: ↑ 11 westi3 years ago

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

Replying to dd32:

IMO, We leave it as-is for the 3.1 release and revisit in 3.2 with the intention of cleaning up some of the update ui's

I agree.

@solarissmoke please can you create a new ticket for the improves and add your patch to it.

Thanks.

comment:13 solarissmoke3 years ago

  • Keywords has-patch dev-feedback removed

Yep will do

Note: See TracTickets for help on using tickets.