Collection Input Filter fix messages #5968

Merged
merged 13 commits into from Apr 14, 2014

Conversation

Projects
None yet
6 participants
Contributor

fabiocarneiro commented Mar 13, 2014

All CollectionInputFilter messages are returning as a empty arrays instead of an array of messages.

Also modified the class behavior. Previously it was copying the inputs from its Inputfilter and validating the InputFilter in CollectionInputFilter, which is not necessary since it can already be done in the InputFilter itself.

To make sure this is necessary, I updated testGetDefaultInputFilter, so it can fail when there is a error in the nested CollectionInputFilter. It is failing in current master, but it will work with this hotfix.

The CollectionInputFilter should only get each element data and setting it to the InputFilter it contains, get back the validation and messages and return in its own methods.

Special thanks to @danizord, who is always helping me with those issues.

Member

Ocramius commented Mar 13, 2014

Hey @fabiocarneiro, can you provide tests that back this new logic? I also expect failures from the current test suite, right?

Contributor

fabiocarneiro commented Mar 13, 2014

@Ocramius I'll work on that. The only problem is that the messages were not working. But since i had to change the behavior of that, i went further.

@danizord danizord commented on the diff Mar 13, 2014

library/Zend/InputFilter/CollectionInputFilter.php
@@ -80,7 +80,6 @@ public function setInputFilter($inputFilter)
}
$this->inputFilter = $inputFilter;
- $this->inputs = $inputFilter->getInputs();
@danizord

danizord Mar 13, 2014

Contributor

This change breaks line 172?

@weierophinney

weierophinney Apr 14, 2014

Owner

@fabiocarneiro Can you comment on this? I think @danizord raises a valid point...

@fabiocarneiro

fabiocarneiro Apr 14, 2014

Contributor

@weierophinney, He was talking about the old line 172, already removed. but since this line 83 was not modified, his note wasn't outdated.

@danizord danizord commented on an outdated diff Mar 14, 2014

library/Zend/InputFilter/CollectionInputFilter.php
@@ -170,41 +169,23 @@ public function isValid()
$valid = false;
}
- $inputs = $this->validationGroup ?: array_keys($this->inputs);
+ $inputs = $this->validationGroup ? : array_keys($this->inputFilter->inputs);
@danizord

danizord Mar 14, 2014

Contributor

$this->inputFilter->inputs is a protected property. Use $this->inputFilter->getValidInput() instead.

@danizord danizord commented on an outdated diff Mar 14, 2014

library/Zend/InputFilter/CollectionInputFilter.php
$valid = false;
+ $this->collectionMessages[$key] = $this->inputFilter->getMessages();
+ $this->collectionInvalidInputs[$key] = $this->inputFilter->invalidInputs;
@danizord

danizord Mar 14, 2014

Contributor

$this->inputFilter->getInvalidInput();

@danizord danizord and 1 other commented on an outdated diff Mar 14, 2014

library/Zend/InputFilter/CollectionInputFilter.php
- if ($input instanceof InputFilterInterface) {
- $values[$name] = $input->getValues();
- $rawValues[$name] = $input->getRawValues();
- continue;
- }
- $values[$name] = $input->getValue($this->data);
- $rawValues[$name] = $input->getRawValue();
- $tmpMessages = $input->getMessages();
- if (!empty($tmpMessages)) {
- $messages[$name] = $tmpMessages;
- }
- }
- $this->collectionValues[$key] = $values;
- $this->collectionRawValues[$key] = $rawValues;
+ $this->collectionValues[$key] = $this->inputFilter->getValues();
+ $this->collectionRawValues[$key] = $this->inputFilter->getRawValues();
if (!empty($messages)) {
@danizord

danizord Mar 14, 2014

Contributor

This now becomes dead code.

@fabiocarneiro

fabiocarneiro Mar 14, 2014

Contributor

Didn't get this one, if you didnt notice, there is a $key param there, so it will copy the inputfilter values to the current iteration index, which is still necessary, otherwise you can't build the value tree and data set to the form will be different from data returned from it, tests will fail.

@danizord

danizord Mar 14, 2014

Contributor

Since you removed line 203, now $message will always be "empty". So, you can remove that dead code.

@danizord danizord and 1 other commented on an outdated diff Mar 14, 2014

library/Zend/InputFilter/CollectionInputFilter.php
$this->validationGroup = null;
return $this;
}
- if (is_array($name)) {
- // Best effort check if the validation group was set by a form for BC
- if (count($name) == count($this->collectionData) && is_array(reset($name))) {
- return parent::setValidationGroup(reset($name));
- }
- return parent::setValidationGroup($name);
+ if (is_array($groups)) {
+ foreach($groups as $group) {
+ $this->inputFilter->setValidationGroup($group);
@danizord

danizord Mar 14, 2014

Contributor

It will not always overwrite the previous one?

@fabiocarneiro

fabiocarneiro Mar 14, 2014

Contributor

That is the point. You must overwrite the previous one, since each iteration is dealing with a "new" InputFilter.

@danizord danizord and 1 other commented on an outdated diff Mar 14, 2014

library/Zend/InputFilter/CollectionInputFilter.php
$this->validationGroup = null;
return $this;
}
- if (is_array($name)) {
- // Best effort check if the validation group was set by a form for BC
@danizord

danizord Mar 14, 2014

Contributor

No more BC? Not sure what's going on this commit, it needs a test to be more clear.

@fabiocarneiro

fabiocarneiro Mar 14, 2014

Contributor

It was there because the CollectionInputFilter was dealing with its InputFilter inputs, so now it only gets each validation group it receives and set to the current InputFilter (which is always the same instance btw).

Contributor

danizord commented Mar 14, 2014

@fabiocarneiro It seems that you're introducing a minor behavior change. So, you should probably reopen this PR against develop.

Contributor

danizord commented Mar 14, 2014

Also, @fabiocarneiro. I realised that you moved a lot of logic from CollectionInputFilter to the child InputFilter. It seems very reasonable.

Having said that, I think you can also remove the $this->collection* properties and its setters/getters in order to use the default properties and getters/setters inherited from BaseInputFilter.

fabiocarneiro reopened this Mar 14, 2014

@Maks3w Maks3w commented on the diff Mar 17, 2014

tests/ZendTest/InputFilter/CollectionInputFilterTest.php
@@ -109,17 +109,6 @@ public function testSetInputFilter()
$this->assertInstanceOf('Zend\InputFilter\BaseInputFilter', $this->filter->getInputFilter());
@Maks3w

Maks3w Mar 17, 2014

Member

Please restore the chmod of this file

@fabiocarneiro

fabiocarneiro Mar 17, 2014

Contributor

Restored

Maks3w added the InputFilter label Mar 17, 2014

@Maks3w Maks3w commented on an outdated diff Mar 17, 2014

library/Zend/InputFilter/CollectionInputFilter.php
@@ -223,22 +201,18 @@ public function isValid()
/**
* {@inheritdoc}
*/
- public function setValidationGroup($name)
+ public function setValidationGroup($groups)
@Maks3w

Maks3w Mar 17, 2014

Member

singular

@Maks3w

Maks3w Mar 17, 2014

Member

Please revert the change. It's better to have the same name on parent and childs

@Maks3w Maks3w commented on the diff Mar 17, 2014

tests/ZendTest/InputFilter/CollectionInputFilterTest.php
@@ -109,17 +109,6 @@ public function testSetInputFilter()
$this->assertInstanceOf('Zend\InputFilter\BaseInputFilter', $this->filter->getInputFilter());
}
- public function testInputFilterInputsAppliedToCollection()
- {
- if (!extension_loaded('intl')) {
- $this->markTestSkipped('ext/intl not enabled');
- }
-
- $this->filter->setInputFilter($this->getBaseInputFilter());
-
- $this->assertCount(4, $this->filter->getInputs());
@Maks3w

Maks3w Mar 17, 2014

Member

It's weird to have a collection and don't count the number of elements in the collection

@fabiocarneiro

fabiocarneiro Mar 17, 2014

Contributor

That's not about counting elements in collection. That is about setting the InputFilter inputs to the collection, and checking if they're there. Since that is exactly the behavior i modified in that class, and InputFilter Inputs are no longer assigned and validated in Collections, there is no reason for that test.

That test is already done in InputFilter, so we are good.

@Maks3w Maks3w commented on an outdated diff Mar 17, 2014

library/Zend/InputFilter/CollectionInputFilter.php
@@ -176,45 +175,24 @@ public function isValid()
return $valid;
}
- $inputs = $this->validationGroup ?: array_keys($this->inputs);
+ $inputs = $this->validationGroup ?: array_keys($this->inputFilter->getInputs());
@Maks3w

Maks3w Mar 17, 2014

Member

This variable is unused

@danizord danizord commented on an outdated diff Mar 18, 2014

library/Zend/InputFilter/CollectionInputFilter.php
@@ -13,20 +13,6 @@
class CollectionInputFilter extends InputFilter
{
- /*
- * @var array
- */
- protected $collectionData;
-
- /*
- * @var array
- */
- protected $collectionValidInputs;
-
- /*
- * @var array
- */
- protected $collectionInvalidInputs;
@danizord

danizord Mar 18, 2014

Contributor

Remove this line.

@danizord danizord commented on an outdated diff Mar 18, 2014

library/Zend/InputFilter/CollectionInputFilter.php
if (!is_array($data)) {
$data = array();
}
- $this->data = $data;
- $this->populate();
+ $this->inputFilter->setData($data);
@danizord

danizord Mar 18, 2014

Contributor

$this->inputFilter can be null, use $this->getInputFilter() instead.

@danizord danizord commented on an outdated diff Mar 18, 2014

library/Zend/InputFilter/CollectionInputFilter.php
- if ($this->validateInputs($inputs, $data)) {
- $this->collectionValidInputs[$key] = $this->validInputs;
+ if ($this->inputFilter->isValid()) {
@danizord

danizord Mar 18, 2014

Contributor

Same as above.

@danizord danizord commented on an outdated diff Mar 18, 2014

library/Zend/InputFilter/CollectionInputFilter.php
- if ($this->validateInputs($inputs, $data)) {
- $this->collectionValidInputs[$key] = $this->validInputs;
+ if ($this->inputFilter->isValid()) {
+ $this->validInputs[$key] = $this->inputFilter->getValidInput();
@danizord

danizord Mar 18, 2014

Contributor

Same.

@danizord danizord commented on an outdated diff Mar 18, 2014

library/Zend/InputFilter/CollectionInputFilter.php
$valid = false;
+ $this->collectionMessages[$key] = $this->inputFilter->getMessages();
+ $this->invalidInputs[$key] = $this->inputFilter->getInvalidInput();
@danizord

danizord Mar 18, 2014

Contributor

😄

@danizord danizord commented on an outdated diff Mar 18, 2014

library/Zend/InputFilter/CollectionInputFilter.php
- if ($input instanceof InputFilterInterface) {
- $values[$name] = $input->getValues();
- $rawValues[$name] = $input->getRawValues();
- continue;
- }
- $values[$name] = $input->getValue($this->data);
- $rawValues[$name] = $input->getRawValue();
- $tmpMessages = $input->getMessages();
- if (!empty($tmpMessages)) {
- $messages[$name] = $tmpMessages;
- }
- }
- $this->collectionValues[$key] = $values;
- $this->collectionRawValues[$key] = $rawValues;
+ $this->collectionValues[$key] = $this->inputFilter->getValues();
+ $this->collectionRawValues[$key] = $this->inputFilter->getRawValues();
@danizord

danizord Mar 18, 2014

Contributor

Same as above

@danizord danizord commented on an outdated diff Mar 18, 2014

library/Zend/InputFilter/CollectionInputFilter.php
@@ -231,30 +194,10 @@ public function setValidationGroup($name)
}
if (is_array($name)) {
- // Best effort check if the validation group was set by a form for BC
- if (count($name) == count($this->collectionData) && is_array(reset($name))) {
- return parent::setValidationGroup(reset($name));
+ foreach($name as $group) {
+ $this->inputFilter->setValidationGroup($group);
@danizord

danizord Mar 18, 2014

Contributor

ok, last one :)

@danizord danizord commented on an outdated diff Mar 18, 2014

library/Zend/InputFilter/CollectionInputFilter.php
@@ -300,4 +250,5 @@ public function getMessages()
{
return $this->collectionMessages;
}
+
@danizord

danizord Mar 18, 2014

Contributor

Remove this line.

@danizord danizord commented on an outdated diff Mar 18, 2014

library/Zend/InputFilter/CollectionInputFilter.php
@@ -1,4 +1,5 @@
<?php
+
@danizord

danizord Mar 18, 2014

Contributor

Remove this line.

Contributor

fabiocarneiro commented Mar 19, 2014

@Maks3w Can you review this one? Tell me what must be done to get it merged.

Ocramius added the bug label Apr 3, 2014

Will someone review this ? I need it in my project.

Owner

weierophinney commented Apr 14, 2014

@fabiocarneiro I'll review after you answer the open question above.

weierophinney added this to the 2.3.1 milestone Apr 14, 2014

weierophinney self-assigned this Apr 14, 2014

@weierophinney weierophinney added a commit that referenced this pull request Apr 14, 2014

@weierophinney weierophinney Merge branch 'hotfix/5968' into develop
Forward port #5968
b6a7d4e

weierophinney merged commit d314e45 into zendframework:master Apr 14, 2014

1 check passed

default The Travis CI build passed
Details

@weierophinney weierophinney added a commit that referenced this pull request Apr 14, 2014

@weierophinney weierophinney Merge branch 'hotfix/5968'
Close #5968
df97b23

@weierophinney weierophinney added a commit to zendframework/zend-inputfilter that referenced this pull request May 15, 2015

@weierophinney weierophinney Merge pull request zendframework/zendframework#5968 from fabiocarneir…
…o/hotfix-collection-input-filter-messages

Collection Input Filter fix messages
5050f73

@weierophinney weierophinney added a commit to zendframework/zend-inputfilter that referenced this pull request May 15, 2015

@weierophinney weierophinney Merge branch 'hotfix/5968' 1821648

@weierophinney weierophinney added a commit to zendframework/zend-inputfilter that referenced this pull request May 15, 2015

@weierophinney weierophinney Merge branch 'hotfix/5968' into develop 0cd91e3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment