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

IBX-354: Used thumbnail strategy for thumbnail generation #101

Merged
merged 2 commits into from
May 21, 2021

Conversation

ViniTou
Copy link
Contributor

@ViniTou ViniTou commented May 14, 2021

'uri' => $thumbnail->resource,
'width' => $thumbnail->width,
'height' => $thumbnail->height,
'mimeType' => $thumbnail->mimeType,
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing alternativeText in this array might cause issues. Is this intentional?

Copy link
Contributor Author

@ViniTou ViniTou May 14, 2021

Choose a reason for hiding this comment

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

intentional

yes. Our thumbnail object (\eZ\Publish\API\Repository\Values\Content\Thumbnail) does not have alternative text atm, it would have to be implemented for every \eZ\Publish\SPI\Repository\Strategy\ContentThumbnail\Field\ThumbnailStrategy we have.

On the other hand, you maybe right and till it will happen maybe we should just put empty string there.

Comment on lines +28 to +32
$thumbnail = $this->thumbnailStrategy->getThumbnail(
$content->getContentType(),
$content->getFields(),
$content->getVersionInfo()
);
Copy link
Member

Choose a reason for hiding this comment

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

hmmm I've got feeling that try ... catch statement should stay here (with proper error logging)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I see we are not catchin it inside \eZ\Publish\Core\Repository\Mapper\ContentDomainMapper::buildContentDomainObject and we did not had problems with it till now as exceptions are catched (without login tho) inside specific strategies.

Copy link
Member

Choose a reason for hiding this comment

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

OK

@ViniTou ViniTou requested a review from Steveb-p May 17, 2021 07:30
@tomaszszopinski tomaszszopinski self-assigned this May 19, 2021
Copy link

@tomaszszopinski tomaszszopinski left a comment

Choose a reason for hiding this comment

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

QA approved on Ibexa Exp 3.2.6 & 3.2.7-dev with patch & diff.

@lserwatka lserwatka merged commit 3f80847 into 2.2 May 21, 2021
@adamwojs adamwojs deleted the IBX-354-thumbnails branch May 21, 2021 09:19
@ViniTou
Copy link
Contributor Author

ViniTou commented May 25, 2021

merged up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants