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

BUGFIX: Detect checked state for bound object collections #1230

Merged
merged 5 commits into from Apr 20, 2018

Conversation

kdambekalns
Copy link
Member

@kdambekalns kdambekalns commented Mar 7, 2018

When a collection property with objects is bound to a checkbox VH,
this change makes sure to check object identifiers against the passed
value when automatically determining the checked state.

Fixes #1229

When a collection property with obejcts is bound to a checkbox VH,
this change makes sure to check object identifiers against the passed
value when automatically determining the `checked` state.

Fixes neos#1229
@bwaidelich
Copy link
Member

as a bugfix shouldn't this target a lower branch?

@kdambekalns kdambekalns changed the base branch from master to 4.0 March 7, 2018 12:32
@kdambekalns
Copy link
Member Author

Absolutely, just didn't select the correct branch…

@kdambekalns kdambekalns closed this Mar 7, 2018
@kdambekalns kdambekalns reopened this Mar 7, 2018
@kdambekalns
Copy link
Member Author

Ah, there is Travis…

Copy link
Collaborator

@aertmann aertmann left a comment

Choose a reason for hiding this comment

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

makes sense, but a test should be added for it

@albe
Copy link
Member

albe commented Mar 13, 2018

Looks good, but yeah, a test would be cool

@bwaidelich
Copy link
Member

@kdambekalns the lowest supported branch is 3.3 - did you not target this on purpose?

@@ -89,7 +93,22 @@ public function render($checked = null, $multiple = null)
}
if (is_array($propertyValue)) {
if ($checked === null) {
$checked = in_array($valueAttribute, $propertyValue, true);
if (TypeHandling::isSimpleType(TypeHandling::getTypeForValue(current($propertyValue))) === false || Arrays::containsMultipleTypes($propertyValue)) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we can just change this to always walk through the array. Arrays::containsMultipleTypes() iterates through it anyways.

@bwaidelich
Copy link
Member

@kdambekalns sorry, I messed with Git..

@kdambekalns
Copy link
Member Author

Regarding 3.3… well, I was just too lazy, AFAIR

@kdambekalns kdambekalns force-pushed the bugfix/1229-checked-with-objects branch from d51f567 to 5f4d4ce Compare April 19, 2018 12:48
@bwaidelich
Copy link
Member

I think 4.0 as target is fine!

Copy link
Member

@albe albe left a comment

Choose a reason for hiding this comment

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

Looks good by reading and green. Fine for me too, to have it 4.0 only. It's a rather featurish bugfix after all :D

@kdambekalns kdambekalns merged commit 1c15f2c into neos:4.0 Apr 20, 2018
@kdambekalns kdambekalns deleted the bugfix/1229-checked-with-objects branch April 20, 2018 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants