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-31681: Fixed Image Value comparison via FT Comparable contract #3060

Merged
merged 19 commits into from
Nov 18, 2020
Merged

EZP-31681: Fixed Image Value comparison via FT Comparable contract #3060

merged 19 commits into from
Nov 18, 2020

Conversation

mateuszdebinski
Copy link
Contributor

@mateuszdebinski mateuszdebinski commented Aug 26, 2020 •
edited
Loading

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

TODO:

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

EZEE-3159: comparing fieldType's hash and additional verification for…
a8ed57a 
… fieldType's Image whether anything except imageId has changed
@mateuszdebinski mateuszdebinski changed the title [WIP]EZEE-3159: Changed comparing fieldType's hash and additional verification forfieldType's Image whether anything except imageId has changed [WIP]EZEE-31681: Changed comparing fieldType's hash and additional verification forfieldType's Image whether anything except imageId has changed Aug 26, 2020
@mateuszdebinski mateuszdebinski changed the title [WIP]EZEE-31681: Changed comparing fieldType's hash and additional verification forfieldType's Image whether anything except imageId has changed [WIP]EZEE-31681: Changed comparing fieldType's hash and additional verification for fieldType's Image whether anything except imageId has changed Aug 26, 2020
@mateuszdebinski mateuszdebinski changed the title [WIP]EZEE-31681: Changed comparing fieldType's hash and additional verification for fieldType's Image whether anything except imageId has changed EZEE-31681: Changed comparing fieldType's hash and additional verification for fieldType's Image whether anything except imageId has changed Aug 26, 2020
ViniTou
ViniTou requested changes Sep 7, 2020
View reviewed changes
Copy link
Contributor

@ViniTou ViniTou 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.

I think, that compareHash or areHashEquals or are haveEqualsValue (or however we gonna name it) should be a method in \eZ\Publish\SPI\FieldType\FieldType and ContentService should be not polluted with knowledge about comparing specifing fieldtypes hashes.

eZ/Publish/Core/Repository/ContentService.php Outdated Show resolved Hide resolved
eZ/Publish/Core/Repository/ContentService.php Outdated Show resolved Hide resolved
alongosz
alongosz requested changes Sep 7, 2020
View reviewed changes
eZ/Publish/Core/Repository/ContentService.php Outdated Show resolved Hide resolved
eZ/Publish/Core/Repository/ContentService.php Outdated Show resolved Hide resolved
ViniTou
ViniTou requested changes Sep 14, 2020
View reviewed changes
eZ/Publish/SPI/FieldType/Comparable.php Outdated Show resolved Hide resolved
alongosz
alongosz requested changes Sep 14, 2020
View reviewed changes
Copy link
Member

@alongosz alongosz 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.

We need to reassess architecture and decide what to do before processing my remarks (see the #3060 (comment) for details).

eZ/Publish/Core/FieldType/Image/Value.php Outdated Show resolved Hide resolved
eZ/Publish/Core/FieldType/Tests/Image/ValueTest.php Outdated Show resolved Hide resolved
eZ/Publish/Core/FieldType/Tests/Image/ValueTest.php Outdated Show resolved Hide resolved
eZ/Publish/Core/FieldType/Tests/Image/ValueTest.php Outdated Show resolved Hide resolved
eZ/Publish/Core/FieldType/Tests/Image/ValueTest.php Outdated Show resolved Hide resolved
eZ/Publish/Core/FieldType/Tests/Image/ValueTest.php Outdated Show resolved Hide resolved
eZ/Publish/Core/Repository/ContentService.php Outdated Show resolved Hide resolved
eZ/Publish/Core/FieldType/Image/Value.php Outdated Show resolved Hide resolved
@alongosz alongosz mentioned this pull request Sep 25, 2020
Closed
3 tasks
alongosz
alongosz requested changes Sep 25, 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.

My suggestion from the previous review: #3067
@ViniTou WDYT?

@alongosz alongosz changed the title EZP-31681: Changed comparing fieldType's hash and additional verification for fieldType's Image whether anything except imageId has changed EZP-31681: Fixed Image Value comparison via FT Comparable contract Oct 21, 2020
alongosz
alongosz approved these changes Nov 16, 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.

+1 if Travis agrees ;)

@micszo micszo self-assigned this Nov 17, 2020
micszo
micszo approved these changes Nov 18, 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.15 with diff.

@micszo micszo removed their assignment Nov 18, 2020
@lserwatka lserwatka merged commit 4b6717c into 7.5 Nov 18, 2020
@lserwatka lserwatka deleted the EZP-31681 branch November 18, 2020 13:35
@alongosz
Copy link
Member

Merged horizontally into ezplatform-kernel:1.1, 1.2, master as ezsystems/ezplatform-kernel@ac9a046, ezsystems/ezplatform-kernel@2bd637e, and ezsystems/ezplatform-kernel@976e52a respectively.

This was referenced Mar 11, 2021
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alongosz alongosz alongosz approved these changes

@micszo micszo micszo approved these changes

@ViniTou ViniTou ViniTou approved these changes

@konradoboza konradoboza konradoboza approved these changes

@mateuszbieniek mateuszbieniek mateuszbieniek approved these changes

@adamwojs adamwojs Awaiting requested review from adamwojs

Assignees
No one assigned
Labels
Bug QA approved
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@mateuszdebinski @alongosz @micszo @ViniTou @konradoboza @mateuszbieniek @lserwatka