Try Catch for embeded media #9424

rdigitalg · 2020-03-04T14:09:07Z

BUG Embedded media throws E_EMERGENCY error if url returns status codes other than 200.

If a media that is already embedded on the page from a url is removed from the server/url it throws an E_EMERGENCY error that breaks the whole page. Added a try / catch block to EmbedShortcodeProvider.php in function handle_shortcode() that will prevent this and just give a message based on the environment.
Dev environment will display the response message from the url.
Live environment will just display the text 'There was a problem loading the media.'

The text is wrapped in a div that has the class ss-media-exception so it can be styled by admin.

ScopeyNZ · 2020-03-05T02:55:09Z

src/View/Shortcodes/EmbedShortcodeProvider.php

+            $attr = array(
+                'class' => 'ss-media-exception embed'
+            );


Suggested change

$attr = array(

'class' => 'ss-media-exception embed'

);

$attr = [

'class' => 'ss-media-exception embed'

];

src/View/Shortcodes/EmbedShortcodeProvider.php

asecondwill · 2020-03-05T10:57:36Z

src/View/Shortcodes/EmbedShortcodeProvider.php

+            $result = HTML::createTag(
+                'div',
+                $attr,
+                HTML::createTag('p', array(), 'There was a problem loading the media.')


If changing to the [] format from array() in @ScopeyNZ s suggestion, then probably should do the same here.

Suggested change

HTML::createTag('p', array(), 'There was a problem loading the media.')

HTML::createTag('p', [], 'There was a problem loading the media.')

Should the text be translateable?
https://docs.silverstripe.org/en/4/developer_guides/i18n/

Suggested change

HTML::createTag('p', array(), 'There was a problem loading the media.')

HTML::createTag('p', [], _t("Namespace.Entity", "There was a problem loading the media");

@ScopeyNZ - can you advise what the namespace & entity should be?

Good catch. Normally the namespace is related to the class (and namespace) - the "entity" indicates the content of the message. Maybe something like EmbedShortcodeProvider.INVALID_URL?

EmbedShortcodeProvider.INVALID_URL works for me : )

It should be _t(__CLASS__ . '.INVALID_URL', 'There was a problem loading the media.') so we get the fully qualified class name, that’s the convention across core code 😄

asecondwill · 2020-03-06T14:03:51Z

src/View/Shortcodes/EmbedShortcodeProvider.php

-        $embed = $embed->getEmbed();
+        try {
+            $embed = $embed->getEmbed();
+        }


travis is failing because:
94 | ERROR | [x] Expected 1 space after closing brace; newline found

think the style is supposed to be:

Suggested change

}

try {

$embed = $embed->getEmbed();

} catch(\Embed\Exceptions\InvalidUrlException $e) {

https://www.php-fig.org/psr/psr-2/#56-try-catch

asecondwill · 2020-03-06T14:04:46Z

src/View/Shortcodes/EmbedShortcodeProvider.php

+        try {
+            $embed = $embed->getEmbed();
+        }
+        catch(\Embed\Exceptions\InvalidUrlException $e) {


95 | ERROR | [x] Expected 1 space(s) after CATCH keyword; 0 found

Suggested change

catch(\Embed\Exceptions\InvalidUrlException $e) {

catch (\Embed\Exceptions\InvalidUrlException $e) {

Could you change this to
use \Embed\Exceptions\InvalidUrlException; (top of the php file)
catch (InvalidUrlException $e) {

asecondwill · 2020-03-06T14:19:41Z

scrutinizer is flagging a decrease in quality, complaining that this can't be found:

SilverStripe\View\Shortcodes\Director

for php 7.1.

Am I reading that right? it has a tick, but an F sounds pretty bad. @rdigitalg have you tested against php 7.1 ?

kinglozzer · 2020-03-06T14:21:06Z

@asecondwill I think it’s because it’s currently missing the namespace import

rdigitalg · 2020-03-10T08:50:58Z

scrutinizer is flagging a decrease in quality, complaining that this can't be found:

SilverStripe\View\Shortcodes\Director

for php 7.1.

Am I reading that right? it has a tick, but an F sounds pretty bad. @rdigitalg have you tested against php 7.1 ?

only tested on php 7.0, ill run tests on other versions and report back

asecondwill · 2020-03-11T10:16:28Z

Thanks for changes @rdigitalg ,

@ScopeyNZ @kinglozzer @emteknetnz :

How is this looking now?

asecondwill · 2020-03-17T13:09:31Z

@kinglozzer - is this likely to get merged do you think?

kinglozzer

I’m fine with merging this. I know that adding caching is mentioned in #9073, but no one has actually submitted a PR for that and this certainly doesn’t block that work

src/View/Shortcodes/EmbedShortcodeProvider.php

Cheddam · 2020-04-20T07:51:34Z

Hey @asecondwill, sorry this is taking a while to get across the line! If you could squash your commits and give them a message matching our standard format - something like “FIX Handle failures in embedded media gracefully” - I’d be happy to merge this for you 🙂

quamsta · 2020-06-17T15:32:44Z

Would be great if this could get merged in. Spent the morning fixing older blog posts containing broken GIFs that were bringing down a site's blog entirely.

Co-authored-by: Loz Calver <[email protected]>

emteknetnz · 2020-07-03T00:11:10Z

Tested locally on a site using silverstripe-themes/simple:

SS_ENVIRONMENT_TYPE="dev"

SS_ENVIRONMENT_TYPE="live"

Youtube domain correct, invalid urlsegment (no preview, links to "video unavailable" page on youtube)

Valid embeds still function correctly on frontend. CMS still functions correctly.

Looks good to me

Thanks for your contribution

src/View/Shortcodes/EmbedShortcodeProvider.php

brynwhyman · 2020-07-17T05:18:11Z

Hi, what are the chances of cherry picking this fix back to the 4.6 branch?

* Try Catch for embeded media * added missing namespaces, translatable message INVALID_URL * generate tag only once * catch after closing bracket * space after comma * Update src/View/Shortcodes/EmbedShortcodeProvider.php * Linting Co-authored-by: [email protected] <[email protected]> Co-authored-by: Steve Boyd <[email protected]> Co-authored-by: Loz Calver <[email protected]>

emteknetnz · 2020-07-17T07:47:14Z

@brynwhyman
Issue: #9601
PR: #9600

asecondwill · 2020-07-20T10:00:18Z

Thanks for merging this in, Ill test it this week on our site.

quamsta · 2020-07-20T18:39:49Z

Same, thank you all!! <3

Try Catch for embeded media

d1e3dfe

brettt89 mentioned this pull request Mar 5, 2020

Embedded content throws 500 during connection issues. #9073

Closed

6 tasks

Cheddam linked an issue Mar 5, 2020 that may be closed by this pull request

Embedded content throws 500 during connection issues. #9073

Closed

6 tasks

ScopeyNZ reviewed Mar 5, 2020

View reviewed changes

asecondwill reviewed Mar 5, 2020

View reviewed changes

asecondwill reviewed Mar 6, 2020

View reviewed changes

dataqube added 4 commits March 10, 2020 11:30

added missing namespaces, translatable message INVALID_URL

f88dbe6

generate tag only once

94b73b4

catch after closing bracket

006151c

space after comma

d6e3465

kinglozzer approved these changes Mar 18, 2020

View reviewed changes

src/View/Shortcodes/EmbedShortcodeProvider.php Outdated Show resolved Hide resolved

Update src/View/Shortcodes/EmbedShortcodeProvider.php

d8415ee

Co-authored-by: Loz Calver <[email protected]>

emteknetnz reviewed Jul 3, 2020

View reviewed changes

src/View/Shortcodes/EmbedShortcodeProvider.php Outdated Show resolved Hide resolved

Linting

1edc15e

emteknetnz merged commit 3bf89b2 into silverstripe:4 Jul 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try Catch for embeded media #9424

Try Catch for embeded media #9424

rdigitalg commented Mar 4, 2020

ScopeyNZ Mar 5, 2020

asecondwill Mar 5, 2020

asecondwill Mar 5, 2020

ScopeyNZ Mar 5, 2020

asecondwill Mar 6, 2020

kinglozzer Mar 6, 2020

asecondwill Mar 6, 2020

asecondwill Mar 6, 2020

asecondwill Mar 6, 2020

emteknetnz Mar 9, 2020

asecondwill commented Mar 6, 2020

kinglozzer commented Mar 6, 2020

rdigitalg commented Mar 10, 2020

asecondwill commented Mar 11, 2020

asecondwill commented Mar 17, 2020

kinglozzer left a comment

Cheddam commented Apr 20, 2020

quamsta commented Jun 17, 2020

emteknetnz commented Jul 3, 2020

brynwhyman commented Jul 17, 2020

emteknetnz commented Jul 17, 2020 •

edited

Loading

asecondwill commented Jul 20, 2020

quamsta commented Jul 20, 2020

	HTML::createTag('p', array(), 'There was a problem loading the media.')
	HTML::createTag('p', [], 'There was a problem loading the media.')

	HTML::createTag('p', array(), 'There was a problem loading the media.')
	HTML::createTag('p', [], _t("Namespace.Entity", "There was a problem loading the media");

	catch(\Embed\Exceptions\InvalidUrlException $e) {
	catch (\Embed\Exceptions\InvalidUrlException $e) {

Try Catch for embeded media #9424

Try Catch for embeded media #9424

Conversation

rdigitalg commented Mar 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asecondwill commented Mar 6, 2020

kinglozzer commented Mar 6, 2020

rdigitalg commented Mar 10, 2020

asecondwill commented Mar 11, 2020

asecondwill commented Mar 17, 2020

kinglozzer left a comment

Choose a reason for hiding this comment

Cheddam commented Apr 20, 2020

quamsta commented Jun 17, 2020

emteknetnz commented Jul 3, 2020

brynwhyman commented Jul 17, 2020

emteknetnz commented Jul 17, 2020 • edited Loading

asecondwill commented Jul 20, 2020

quamsta commented Jul 20, 2020

emteknetnz commented Jul 17, 2020 •

edited

Loading