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

Jump to bottom

EZP-31292: Fixed generating image variation using ez_image_alias #2917

Merged
merged 2 commits into from
Jan 21, 2020
Merged

EZP-31292: Fixed generating image variation using ez_image_alias #2917

merged 2 commits into from
Jan 21, 2020

Conversation

barw4
Copy link
Member

@barw4 barw4 commented Jan 15, 2020 •
edited
Loading

Question Answer
JIRA issue EZP-31292
Bug/Improvement bug
New feature no
Target version 7.5
BC breaks no
Tests pass yes
Doc needed no

There was an invalid property accessed in eZ\Bundle\EzPublishCoreBundle\Imagine\Variation\ImagineAwareAliasGenerator which resulted in 500 exception, as the $field object was not an Image\Value instance but an ImageAsset\Value instance which does not have the ID property. The Image\Value ID property has been added to variation in order to provide a valid variation file path.

TODO:

  • Fix a bug.
  • Implement tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

@barw4 barw4 self-assigned this Jan 15, 2020
@barw4 barw4 requested a review from kmadejski January 15, 2020 12:44
@adamwojs
Copy link
Member

adamwojs commented Jan 16, 2020 •
edited by andrerom
Loading

As we talked on Slack: please check if \eZ\Publish\SPI\Variation\VariationHandlerimplementations are executed in the correct order. \eZ\Bundle\EzPublishCoreBundle\Imagine\ImageAsset\AliasGenerator should be executed before \eZ\Bundle\EzPublishCoreBundle\Imagine\Variation\ImagineAwareAliasGenerator. This will guarante that ImagineAwareAliasGenerator::getVaration will work on ezimage field instread of ezimageasset.

@barw4
Copy link
Member Author

barw4 commented Jan 17, 2020 •
edited
Loading

As we talked on Skack: please check if \eZ\Publish\SPI\Variation\VariationHandlerimplementations are executed in the correct order. \eZ\Bundle\EzPublishCoreBundle\Imagine\ImageAsset\AliasGenerator should be executed before \eZ\Bundle\EzPublishCoreBundle\Imagine\Variation\ImagineAwareAliasGenerator. This will guarante that ImagineAwareAliasGenerator::getVaration will work on ezimage field instread of ezimageasset.

@adamwojs yeah, that definitely looks like a better solution, already committed b8c8cc3

alongosz
alongosz approved these changes Jan 17, 2020
View reviewed changes
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

I'm assuming that generating image variation for ezimage field still works as well ;)
cc @ezsystems/qa-team

@barw4
Copy link
Member Author

barw4 commented Jan 20, 2020

I'm assuming that generating image variation for ezimage field still works as well ;)
cc @ezsystems/qa-team

Yes, it seems to be working as well.

@micszo micszo self-assigned this Jan 20, 2020
micszo
micszo reviewed Jan 21, 2020
View reviewed changes
Copy link
Member

@micszo micszo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@barw4 exception does not occur but item preview on frontend is blank. Could you verify on your side? (both when using Upload image and Select from your repository) Resolved: issue with template.

micszo
micszo approved these changes Jan 21, 2020
View reviewed changes
Copy link
Member

@micszo micszo 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 eZ Platform EE v2.5.8 with diff.

@micszo micszo removed their assignment Jan 21, 2020
@adamwojs adamwojs merged commit bbb9c78 into ezsystems:7.5 Jan 21, 2020
@adamwojs
Copy link
Member

Could you merge it up @barw4?

@barw4 barw4 deleted the EZP-31292 branch January 21, 2020 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamwojs adamwojs adamwojs approved these changes

@alongosz alongosz alongosz approved these changes

@kmadejski kmadejski kmadejski approved these changes

@micszo micszo micszo approved these changes

@konradoboza konradoboza konradoboza approved these changes

Assignees

@barw4 barw4

Labels
Bug QA approved
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@barw4 @adamwojs @alongosz @kmadejski @micszo @konradoboza