Make WordPress Core

Opened 10 years ago

Closed 4 years ago

#31502 closed enhancement (worksforme)

Replace embed URL with error message if oEmbed provider returns an error

Reported by: mbootsman's profile mbootsman Owned by: deremohan's profile deremohan
Milestone: Priority: normal
Severity: normal Version: 2.9
Component: Embeds Keywords: good-first-bug has-patch needs-testing has-screenshots
Focuses: Cc:

Description

If a youtube url in a post does not work when video is removed, or any other error occurs, the url of the video is shown. I tink it's better for UX to show an error message.

Attachments (7)

31502.diff (1000 bytes) - added by GaVrA 9 years ago.
Screen Shot 2015-09-06 at 2.07.14 PM.png (124.1 KB) - added by GaVrA 9 years ago.
31502.patch (799 bytes) - added by deremohan 9 years ago.
private-video-before-embed.png (32.4 KB) - added by desrosj 4 years ago.
on-load.png (884.0 KB) - added by desrosj 4 years ago.
How a previously embedded video that has since been deleted (though very recently) looks on load. It's possible the thumbnail is also eventually deleted.
play-clicked.png (61.7 KB) - added by desrosj 4 years ago.
Clicking play on a previously embedded video that has since been deleted
moved-private.png (50.3 KB) - added by desrosj 4 years ago.
Clicking play on a previously embedded video that has since been moved private

Download all attachments as: .zip

Change History (28)

#1 @SergeyBiryukov
10 years ago

  • Component changed from General to Embeds

#2 follow-up: @istein
10 years ago

Hey, I think that this is a good idea too. I'll try some things over the next few days and post what I come up with for people to discuss.

#3 in reply to: ↑ 2 @mbootsman
10 years ago

Replying to istein:

Hey, I think that this is a good idea too. I'll try some things over the next few days and post what I come up with for people to discuss.

Cool! Maybe obsolete, but please use a filter for this, so ppl can change the message if they want to.

#4 follow-up: @iseulde
10 years ago

  • Keywords reporter-feedback added

Do you mean in the editor, or the front-end? Maybe related: #29841. Links won't be converted to anything unless there's something from the provider to show. IMO it's up to them to show a message. If they do, we will show it.

#5 in reply to: ↑ 4 ; follow-up: @mbootsman
10 years ago

Replying to iseulde:

Do you mean in the editor, or the front-end? Maybe related: #29841. Links won't be converted to anything unless there's something from the provider to show. IMO it's up to them to show a message. If they do, we will show it.

I mean in the front-end. In the editor a embed image is shown. I think that if you try to embed something that does not exist, WordPress should tell that something went wrong, and have this message be filterable.

#6 in reply to: ↑ 5 ; follow-up: @jesin
10 years ago

Replying to mbootsman:

I mean in the front-end. In the editor a embed image is shown. I think that if you try to embed something that does not exist, WordPress should tell that something went wrong, and have this message be filterable.

I believe the embed_maybe_make_link can be used for this. It is present in tags/4.1/src/wp-includes/class-wp-embed.php#L332.

#7 follow-up: @iseulde
10 years ago

In the editor a embed image is shown.

Really? Can you give us an example video?

#8 in reply to: ↑ 7 @mbootsman
10 years ago

Replying to iseulde:

In the editor a embed image is shown.

Really? Can you give us an example video?

My bad, at first the embed image is hown, but when an invalid link is used, the url is shown. When this url is changed to a valid video, the video is shown.

#9 in reply to: ↑ 6 @mbootsman
10 years ago

Replying to jesin:

Replying to mbootsman:

I mean in the front-end. In the editor a embed image is shown. I think that if you try to embed something that does not exist, WordPress should tell that something went wrong, and have this message be filterable.

I believe the embed_maybe_make_link can be used for this. It is present in tags/4.1/src/wp-includes/class-wp-embed.php#L332.

Hmm, I missed that hook, seems usable! Thanks.

Version 0, edited 10 years ago by mbootsman (next)

#10 follow-up: @iseulde
10 years ago

Can you give us an example video, please?

#11 in reply to: ↑ 10 @jesin
10 years ago

Replying to iseulde:

Can you give us an example video, please?

Take any video URL and remove a character from it.

Valid video - https://www.youtube.com/watch?v=2mBG4vlhcCc

Video doesn't exist - https://www.youtube.com/watch?v=2mBG4vlhcC

#12 @johnbillion
9 years ago

  • Keywords needs-patch good-first-bug added; reporter-feedback removed
  • Version changed from 4.1.1 to 2.9

#13 @johnbillion
9 years ago

  • Summary changed from Replace Youtube embed URL with error message if no video found to Replace embed URL with error message if oEmbed provider returns an error

@GaVrA
9 years ago

#14 @GaVrA
9 years ago

I added 31502.diff patch that does the following:

  1. checks response.success and if that is true it shows embed html in response.data.body
  2. if response.success is false it runs this.renderFail()
  3. inside this.renderFail() I check for response.data.type and if it is equal to not-embeddable it shows an error and returns.

#15 @GaVrA
9 years ago

I'm not sure if this is enough, maybe input should be cleared?

$('#embed-url-field' ).val('');?

Also if embed fails, you can still press Insert into page button. I'm not sure how would you go about disabling that since I'm not all that familiar with backbone.

#16 follow-up: @jesin
9 years ago

Patch 31502.diff only checks when adding an embed URL. OP's problem is what happens when an existing video is removed by the video hosting service.

#17 in reply to: ↑ 16 @deremohan
9 years ago

Replying to jesin:

Patch 31502.diff only checks when adding an embed URL. OP's problem is what happens when an existing video is removed by the video hosting service.

Hi Jesin,
As looking ajax call inside ajax-actions.php file at line 2818:

if ( $url && ! $parsed ) {
 $parsed = $wp_embed->run_shortcode( $shortcode );
}

If existing video is removed from hosting, then video url is not get parsed which leads to 'not-embeddable' type error. i mean this case should be handle by WP_Embed class.

Also please have a look for patch which also handles "not-ssl" response from ajax.

@deremohan
9 years ago

#19 @swissspidy
9 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

#20 @DrewAPicture
8 years ago

  • Owner set to deremohan
  • Status changed from new to assigned

Assigning to mark the good-first-bug as "claimed".

@desrosj
4 years ago

How a previously embedded video that has since been deleted (though very recently) looks on load. It's possible the thumbnail is also eventually deleted.

@desrosj
4 years ago

Clicking play on a previously embedded video that has since been deleted

@desrosj
4 years ago

Clicking play on a previously embedded video that has since been moved private

#21 @desrosj
4 years ago

  • Keywords has-screenshots added
  • Resolution set to worksforme
  • Status changed from assigned to closed

I have not been able to reproduce the originally reported issue. I have documented the current behavior with screenshots. This is the behavior that happens so long as the cached embed response (stored in post meta) is not deleted. If that occurs (which does not happen in default WordPress), the URL will be displayed instead.

I am going to close this out as it seems to be working as it should.

Note: See TracTickets for help on using tickets.