Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Try Catch for embeded media #9424

Merged
merged 7 commits into from
Jul 3, 2020
+20 −1
Merged

Try Catch for embeded media #9424

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d1e3dfe
Try Catch for embeded media
dataqube Mar 4, 2020
f88dbe6
added missing namespaces, translatable message INVALID_URL
dataqube Mar 10, 2020
94b73b4
generate tag only once
dataqube Mar 10, 2020
006151c
catch after closing bracket
dataqube Mar 10, 2020
d6e3465
space after comma
dataqube Mar 11, 2020
d8415ee
Update src/View/Shortcodes/EmbedShortcodeProvider.php
emteknetnz Jul 2, 2020
1edc15e
Linting
emteknetnz Jul 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion src/View/Shortcodes/EmbedShortcodeProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,24 @@ public static function handle_shortcode($arguments, $content, $parser, $shortcod
}

// Process embed
$embed = $embed->getEmbed();
try {
$embed = $embed->getEmbed();
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

$attr = array(
'class' => 'ss-media-exception embed'
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$attr = array(
'class' => 'ss-media-exception embed'
);
$attr = [
'class' => 'ss-media-exception embed'
];

if (Director::isDev()) {
$result = HTML::createTag('div', $attr, $e->getMessage());
return $result;
}
$result = HTML::createTag(
'div',
$attr,
HTML::createTag('p', array(), 'There was a problem loading the media.')
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.')

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EmbedShortcodeProvider.INVALID_URL works for me : )

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

);
emteknetnz marked this conversation as resolved.
Show resolved Hide resolved
return $result;
}

// Convert embed object into HTML
if ($embed && $embed instanceof Adapter) {
Expand Down