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.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | media_youtube-3198823-improve-exceptions-7.patch | 1.52 KB | jkingsnorth |
| #4 | interdiff-2-4.txt | 854 bytes | cboyden |
| #4 | media_youtube-3198823-validid-throws-unhandled-exception-4.patch | 741 bytes | cboyden |
| #2 | media_youtube-3198823-validid-throws-unhandled-exception.patch | 738 bytes | calebtr |
Issue fork media_youtube-3198823
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
Comment #2
calebtr commentedI 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.
Comment #3
cboyden commentedThis 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.
Comment #4
cboyden commentedUpdated 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.
Comment #5
calebtr commentedYes, +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.
Comment #6
jkingsnorth commentedThis 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
Comment #7
jkingsnorth commentedI always get the wrong comment number. And I've added a fix to remove unnecessary return statements off the latest dev.
Comment #9
joseph.olstadComment #10
joseph.olstadI tagged 3.11 which has this change,
Thanks everyone!