Skip to content
This repository has been archived by the owner on Jan 30, 2020. It is now read-only.

it's impossible to remove all elements from collection #14

Merged
merged 3 commits into from
Sep 23, 2015
Merged

it's impossible to remove all elements from collection #14

merged 3 commits into from
Sep 23, 2015

Conversation

svycka
Copy link
Contributor

@svycka svycka commented Sep 11, 2015

Fixed incorrect test to show that.

@svycka
Copy link
Contributor Author

svycka commented Sep 11, 2015

Fixed 2 invalid tests and bug

@svycka
Copy link
Contributor Author

svycka commented Sep 16, 2015

@Maks3w @weierophinney @bakura10 any feedback?

@weierophinney
Copy link
Member

@svycka Can you write a new unit test that demonstrates the scenario, instead of altering an existing test, please? Right now, I cannot tell for sure from the tests what you're validating, as it's mixed in with other testing scenarios.

weierophinney
weierophinney reviewed Sep 22, 2015
View reviewed changes
test/Element/CollectionTest.php
$data = [
'colors' => [
'#ffffff',
],
];
$form->setData($data);
$object = new \ArrayObject();
$form->bind($object);
Copy link
Member

Choose a reason for hiding this comment

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

What do these test changes have to do with the change you're introducing? If they're unrelated, please revert them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@weierophinney it is related only because it was not failing just because collection can't remove all elements that's what this PR fixes so it needs to be fixed also.

@svycka
Copy link
Contributor Author

svycka commented Sep 22, 2015

@weierophinney the testCanRemoveAllElementsIfAllowRemoveIsTrue test I altered was incorrect and checked that empty collection is empty and nothing about removing elements. Does this make sense? I could leave old one but it was incorrect and doesn't do what it should.

and with testCanHydrateObject test it is also relies on this bug and is incorrect. It sets colors then binds empty object so it resets collection to empty end expects it to be not empty. Does it make sense?

@weierophinney weierophinney added this to the 2.6.1 milestone Sep 23, 2015
@weierophinney weierophinney self-assigned this Sep 23, 2015
@weierophinney weierophinney merged commit 93796c0 into zendframework:master Sep 23, 2015
weierophinney added a commit that referenced this pull request Sep 23, 2015
@weierophinney
Merge pull request #14 from svycka/hotfix/collection-remove-all-elements
b990e9d 
it's impossible to remove all elements from collection
weierophinney added a commit that referenced this pull request Sep 23, 2015
@weierophinney
Added CHANGELOG for #14
733f413
weierophinney added a commit that referenced this pull request Sep 23, 2015
@weierophinney
Merge branch 'hotfix/14'
767ed40 
Close #14
weierophinney added a commit that referenced this pull request Sep 23, 2015
@weierophinney
Merge branch 'hotfix/14' into develop
aa6de61 
Forward port #14
This was referenced Sep 23, 2015
Closed
Closed
Closed
@svycka svycka deleted the hotfix/collection-remove-all-elements branch September 23, 2015 13:50
@svycka svycka mentioned this pull request Oct 14, 2015
Merged
@adamlundrigan adamlundrigan mentioned this pull request Nov 6, 2015
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@weierophinney weierophinney

Labels
bug
Projects
None yet
Milestone
2.6.1
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@svycka @weierophinney