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.

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

[SeoBundle] Catch exception when OG image no longer exists #1324

Merged
merged 1 commit into from
Oct 13, 2016
Merged

[SeoBundle] Catch exception when OG image no longer exists #1324

merged 1 commit into from
Oct 13, 2016

Conversation

wesleylancel
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets n/a

When the selected ogImage is no longer present on disk, get_image_dimensions will throw an exception.

try {
list($width, $height) = getimagesize($src);
} catch (\Exception $e) {
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think return null is more convenient :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

} catch (\Exception $e) {
return null;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

why not do a check with file_exists ?

if (file_exists($src)) {
    list($width, $height) = getimagesize($src);

    return array('width' => $width, 'height' => $height);
}

return null;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bakie Sure, that's possible. Not sure about the exact performance of those two functions, but wouldn't that just hit the disk a second time for the same thing getimagesize would already do?

Copy link
Contributor

Choose a reason for hiding this comment

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

bare in mind that the file can be stored on a remote server. I turned this feature altogether since my media files are stored in a S3 bucket

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mlebkowski Yes, we sometimes have the same issue. Not sure it's worth the disk read when it's stored locally.

@jockri jockri merged commit 785dcf9 into Kunstmaan:master Oct 13, 2016
@jockri jockri added this to the 3.6.1 milestone Oct 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants