Problem/Motivation

The media_internet module catches exceptions with the classes MediaInternetValidationException and MediaInternetNoHandlerException but not objects of the Exception class.

When an Exception is thrown, the Drupal request returns a 5xx error and a generic error page. The user has no idea what she did wrong, if anything.

Steps to reproduce

Create a video URL with extra characters after the valid video ID (e.g. https://www.youtube.com/watch?v=DA0xBTxqxdMdrupal) and try to embed it.

YouTube returns a 400 status code, throwing an Exception in MediaInternetYouTubeHandler::validId().

Proposed resolution

Use the MediaInternetValidationException class instead of Exception when throwing an exception from MediaInternetYouTubeHandler::validId().

This way, the user is redirected back to the form and a related error message can be displayed.

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

calebtr created an issue. See original summary.

calebtr’s picture

Status: Active » Needs review
StatusFileSize
new738 bytes

I made a patch. I did not find any related tests.

I tried to send the push through gitlab, but I can't authenticate. I'll work on that.

cboyden’s picture

This patch is working as expected. Would it be OK to make the error message more generic? There are more conditions than a bad URL that can cause the exception. One is if the video is set to disallow embedding. In that case, the URL is correct but the video settings need adjusting.

cboyden’s picture

Updated patch and interdiff attached. I'm not sure why videos not allowed for embedding aren't caught by the logic as updated in #1572550: The YouTube video ID is invalid or the video was deleted. but that's what we're experiencing.

calebtr’s picture

Yes, +1 for the message making sense.

Thinking about this now, I suggest we put the message through t() instead of format_string().

Drupal coding standards say not to translate exception messages (https://www.drupal.org/docs/develop/coding-standards/php-exceptions) but in this case, the message is user-facing.

Probably, the module could change the way it handles this error to not use an exception - one that would be more difficult to use and maintain. I think it is best to keep it simple and add translation here.

jkingsnorth’s picture

This is nice, but why not go a step further and give more details for the different error types.

Generally a 401 means the video has embedding disabled, and 403 means the video is 'private' or disabled. So shall we include those specific cases? The generic fallback message covers 400 bad request (video ID is too long or silly) and 404 (video not found).

I've also wrapped it in t() as suggested in #4

jkingsnorth’s picture

StatusFileSize
new1.52 KB

I always get the wrong comment number. And I've added a fix to remove unnecessary return statements off the latest dev.

joseph.olstad’s picture

Status: Needs review » Fixed
joseph.olstad’s picture

I tagged 3.11 which has this change,

Thanks everyone!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.