Make WordPress Core

Opened 13 years ago

Closed 13 years ago

Last modified 13 years ago

#16132 closed defect (bug) (fixed)

Duplicate "try again" links when themes_api fails

Reported by: solarissmoke's profile 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 13 years ago.
Don't print duplicate "try again" links when themes_api fails
16132-more.diff (762 bytes) - added by solarissmoke 13 years ago.
... and show more useful error information

Download all attachments as: .zip

Change History (15)

#1 @solarissmoke
13 years ago

  • Keywords has-patch added

#2 follow-ups: @nacin
13 years ago

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

Is this is a regression?

#3 in reply to: ↑ 2 @solarissmoke
13 years ago

Replying to nacin:

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

But the error page only links to 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().

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

@solarissmoke
13 years ago

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

#4 in reply to: ↑ 2 @SergeyBiryukov
13 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.

#5 @nacin
13 years ago

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

#6 @nacin
13 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.

#7 @nacin
13 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().

#8 @solarissmoke
13 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.

I think we need to be using WP_Error::get_error_messages() to catch and display all bubbled errors. Knocking up a patch now.

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

#9 @solarissmoke
13 years ago

  • Keywords has-patch dev-feedback added

@solarissmoke
13 years ago

... and show more useful error information

#10 @solarissmoke
13 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() );

#11 follow-up: @dd32
13 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

#12 in reply to: ↑ 11 @westi
13 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.

#13 @solarissmoke
13 years ago

  • Keywords has-patch dev-feedback removed

Yep will do

Note: See TracTickets for help on using tickets.