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-31795: Added VersionInfo to getThumbnail method in order to utilise builtin caching mechanism #158

Merged
merged 3 commits into from
Jan 12, 2021
Merged

EZP-31795: Added VersionInfo to getThumbnail method in order to utilise builtin caching mechanism #158

merged 3 commits into from
Jan 12, 2021

Conversation

ViniTou
Copy link
Contributor

@ViniTou ViniTou commented Jan 11, 2021 •
edited
Loading

Question Answer
JIRA issue EZP-31795
Type feature
Target eZ Platform version v3.3
BC breaks yes
Doc needed yes

ezsystems/ezplatform-admin-ui#1690
https://github.com/ezsystems/ezplatform-connector-dam/pull/29

Checklist:

  • Provided PR description.
  • Tested the solution manually.
  • Provided automated test coverage.
  • Checked that target branch is set correctly (master for features, the oldest supported for bugs).
  • Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
  • Asked for a review (ping @ezsystems/php-dev-team).

@ViniTou ViniTou mentioned this pull request Jan 11, 2021
Merged
2 tasks
@alongosz
Copy link
Member

@ViniTou CI is failing. Some description is needed - why passing VersionInfo solves the issue?

@ViniTou
Copy link
Contributor Author

ViniTou commented Jan 12, 2021

@alongosz
VariationHandler needs proper VersionInfo in order to generate cache key, in our implementation dummy new VersionInfo() was provided.
\eZ\Bundle\EzPublishCoreBundle\Imagine\Cache\AliasGeneratorDecorator::getVariation

@lserwatka
Copy link
Member

@ViniTou Could you investigate CI?

@lserwatka
Copy link
Member

@ViniTou rebase is required

@ViniTou ViniTou force-pushed the ezp-31795-cache-thumbnails branch from 6d1e389 to cd37db6 Compare January 12, 2021 09:20
@ViniTou
Copy link
Contributor Author

ViniTou commented Jan 12, 2021

Fixed Unit & integration test, only behat is failing as the signature changes in adminUI implementation (done here ezsystems/ezplatform-admin-ui#1690)

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@micszo micszo self-assigned this Jan 12, 2021
micszo
micszo approved these changes Jan 12, 2021
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.

Verified with sanities on Ibexa Commerce 3.3.0-dev with diffs.

@micszo micszo removed their assignment Jan 12, 2021
@lserwatka lserwatka merged commit d0ce93f into master Jan 12, 2021
@lserwatka lserwatka deleted the ezp-31795-cache-thumbnails branch January 12, 2021 14:20
Steveb-p pushed a commit that referenced this pull request Feb 10, 2023
@alongosz @mnocon
Merged branch '1.3' of ezsystems/ezplatform-kernel into 4.2 (#158)
9aea579 
Merged up #336

Co-Authored-By: Marek Nocoń <mnocon@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lserwatka lserwatka lserwatka approved these changes

@alongosz alongosz alongosz approved these changes

@micszo micszo micszo approved these changes

@Nattfarinn Nattfarinn Awaiting requested review from Nattfarinn

@adamwojs adamwojs Awaiting requested review from adamwojs

@kaff kaff Awaiting requested review from kaff

@mikadamczyk mikadamczyk Awaiting requested review from mikadamczyk

@Steveb-p Steveb-p Awaiting requested review from Steveb-p

@webhdx webhdx Awaiting requested review from webhdx

Assignees
No one assigned
Labels
QA approved
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ViniTou @alongosz @lserwatka @micszo