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-31449: Adapted TranslatablePropertyTransformer to properly handle nulls #338

Merged
merged 3 commits into from
Apr 17, 2020
Merged

EZP-31449: Adapted TranslatablePropertyTransformer to properly handle nulls #338

merged 3 commits into from
Apr 17, 2020

Conversation

barw4
Copy link
Member

@barw4 barw4 commented Apr 8, 2020 •
edited
Loading

Question Answer
JIRA issue EZP-31449
Bug yes
New feature no
BC breaks no
Tests pass yes
Doc needed no

Reverse transformation in TranslatablePropertyTransformer was adapted to properly handle ContentType and ContentType's fieldtype translatable fields when the empty value was provided in form.

Original kernel PR and discussion: ezsystems/ezpublish-kernel#2988 -> closed right now.

TODO:

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

@barw4 barw4 self-assigned this Apr 8, 2020
@barw4 barw4 requested review from alongosz and ViniTou April 8, 2020 16:19
@barw4 barw4 mentioned this pull request Apr 8, 2020
Closed
3 tasks
alongosz
alongosz reviewed Apr 9, 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.

Can we narrow it down to just Content Types? Or is this transformer used only there?

POV ping @webhdx

tests/RepositoryForms/Form/DataTransformer/TranslatablePropertyTransformerTest.php Outdated Show resolved Hide resolved
@barw4
Copy link
Member Author

barw4 commented Apr 10, 2020

Can we narrow it down to just Content Types? Or is this transformer used only there?

POV ping @webhdx

@alongosz Its also used in CT's fieldtypes and the bug there is exactly the same - so this fixes both of them - it's not used anywhere else.

ViniTou
ViniTou approved these changes Apr 10, 2020
View reviewed changes
Copy link
Contributor

@ViniTou ViniTou left a comment

Choose a reason for hiding this comment

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

I was afraid that it may have some side effects in translations of selection options, but it seems that is all alright!

@barw4 barw4 mentioned this pull request Apr 10, 2020
Merged
2 tasks
@lserwatka lserwatka merged commit 93b2d33 into 2.5 Apr 17, 2020
@lserwatka lserwatka deleted the EZP-31449 branch April 17, 2020 08:01
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

@webhdx webhdx webhdx approved these changes

@ViniTou ViniTou ViniTou approved these changes

@bogusez bogusez bogusez approved these changes

@adamwojs adamwojs Awaiting requested review from adamwojs

@michal-myszka michal-myszka Awaiting requested review from michal-myszka

@mikadamczyk mikadamczyk Awaiting requested review from mikadamczyk

@Nattfarinn Nattfarinn Awaiting requested review from Nattfarinn

Assignees

@barw4 barw4

Labels
Bug QA approved
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@barw4 @alongosz @webhdx @ViniTou @bogusez @lserwatka