ZF-11102: Zend_Form_Element issue in _loadFilter method when you use Zend_Filter with an "options" array

Description

considering this Zend_Form class:


class MyForm extends Zend_Form

public function __construct () { 

...

/* country is a Zend_Form_Element_Text object */
$this->country
     ->addFilter('AutocompleteHelper', array('minlength' => 2, 'url' => '/json/coutries'));

}

...

}

As you can see I add a filter to text field with options associative array. Zend_Form_Element class doesn't instance well the Zend_Filter object and during the initializing phase it loose "options" parameters.

To solve this I've change the way how Zend_Filter is initialized on line 1920 (_loadFilter method) of Zend_Form_Element class


            if ($r->hasMethod('__construct')) {
                $instance = new $name($filter['options']);
                //$instance = $r->newInstanceArgs($filter['options']);
            } else {
                $instance = new $name();
                //$instance = $r->newInstance();
            }

Regards

Comments

I decided to reopen this issue since it seems to me to be an inconsistent behaviour when compared to the addValidator() method which does not require an extra wrapping of options array. I have spent some time to make things work and I believe more users will have the same problem - since there is even no such an example in ZF Guide.

this->grade->addValidator('Float', false, array('locale' => 'en')); //english locale since value is already normalized this->grade->addFilter('LocalizedToNormalized', array(array('precision' => 2)));

Sorry for cloning the issue - is there a better way to reopening it?

Is this still an issue against current SVN trunk? This test passes against trunk for me:


Index: Zend/Form/ElementTest.php
===================================================================
--- Zend/Form/ElementTest.php   (revision 24672)
+++ Zend/Form/ElementTest.php   (working copy)
@@ -2189,6 +2189,22 @@
         $validator = $username->getValidator('regex');
         $this->assertTrue($validator->zfBreakChainOnFailure);
     }
+
+    /**
+     * @group ZF-11102
+     */
+    public function testAddFilterPassesOptionsOnToFilterObject()
+    {
+        $field = new Zend_Form_Element('foo');
+        $field->addFilter('Alpha', array(
+            'allowwhitespace' => true,
+        ));
+        $filters = $field->getFilters();
+        $this->assertArrayHasKey('Zend_Filter_Alpha', $filters);
+        $filter = $filters['Zend_Filter_Alpha'];
+        $this->assertType('Zend_Filter_Alpha', $filter);
+        $this->assertTrue($filter->getAllowWhitespace());
+    }
 }
 
 class Zend_Form_ElementTest_Decorator extends Zend_Form_Decorator_Abstract

Could you let me know if i've missed the point with the above test, or if the issue really is fixed in SVN trunk?

Apparently I did. This test fails against trunk:


Index: tests/Zend/Form/ElementTest.php
===================================================================
--- tests/Zend/Form/ElementTest.php (revision 24672)
+++ tests/Zend/Form/ElementTest.php (working copy)
@@ -2189,6 +2189,22 @@
         $validator = $username->getValidator('regex');
         $this->assertTrue($validator->zfBreakChainOnFailure);
     }
+
+    /**
+     * @group ZF-11102
+     */
+    public function testAddFilterPassesOptionsOnToFilterObjectLocalizedToNormalized()
+    {
+        $field = new Zend_Form_Element('foo');
+        $field->addFilter('LocalizedToNormalized', array(
+            'precision' => 2,
+        ));
+        $filters = $field->getFilters();
+        $this->assertArrayHasKey('Zend_Filter_LocalizedToNormalized', $filters);
+        $filter = $filters['Zend_Filter_LocalizedToNormalized'];
+        $this->assertType('Zend_Filter_LocalizedToNormalized', $filter);
+        $this->assertEquals(2, $filter->getPrecision());
+    }
 }
 
 class Zend_Form_ElementTest_Decorator extends Zend_Form_Decorator_Abstract

With this error:


1) Zend_Form_ElementTest::testAddFilterPassesOptionsOnToFilterObjectLocalizedToNormalized
Argument 1 passed to Zend_Filter_LocalizedToNormalized::setOptions() must be an array, integer given, called in Zend/Filter/LocalizedToNormalized.php on line 64 and defined

Zend/Filter/LocalizedToNormalized.php:84
Zend/Filter/LocalizedToNormalized.php:64

Could be a problem with how that particular filter implements it's option handling?

The problem is exactly as you point out. {{newInstanceArgs}} takes it's argument to be an array of constructor arguments to pass on the new instance, which mesh with the intended functionality: the array being passed as first argument. This change (which you also pointed out above) will fix the issue:



Index: library/Zend/Form/Element.php
===================================================================
--- library/Zend/Form/Element.php   (revision 24672)
+++ library/Zend/Form/Element.php   (working copy)
@@ -2083,7 +2083,7 @@
         } else {
             $r = new ReflectionClass($name);
             if ($r->hasMethod('__construct')) {
-                $instance = $r->newInstanceArgs((array) $filter['options']);
+                $instance = $r->newInstanceArgs(array((array) $filter['options']));
             } else {
                 $instance = $r->newInstance();
             }

Here's another example, this time of {{Zend_Filter_PregReplace}}:


Index: tests/Zend/Form/ElementTest.php
===================================================================
--- tests/Zend/Form/ElementTest.php (revision 24672)
+++ tests/Zend/Form/ElementTest.php (working copy)
@@ -2189,6 +2189,23 @@
         $validator = $username->getValidator('regex');
         $this->assertTrue($validator->zfBreakChainOnFailure);
     }
+
+    /**
+     * @group ZF-11102
+     */
+    public function testAddFilterPassesOptionsOnToFilterObjectPregReplace()
+    {
+        $field = new Zend_Form_Element('foo');
+        $field->addFilter('PregReplace', array(
+            'replace' => 'z',
+            'match' => '/[a-z]/i',
+        ));
+        $filters = $field->getFilters();
+        $this->assertArrayHasKey('Zend_Filter_PregReplace', $filters);
+        $filter = $filters['Zend_Filter_PregReplace'];
+        $this->assertType('Zend_Filter_PregReplace', $filter);
+        $this->assertEquals('z', $filter->getReplacement());
+    }
 }
 
 class Zend_Form_ElementTest_Decorator extends Zend_Form_Decorator_Abstract

And the result:


1) Zend_Form_ElementTest::testAddFilterPassesOptionsOnToFilterObjectPregReplace
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-z
+/[a-z]/i

Instead of calling constructor with single array argument, the array contents are used as N arguments to the constructor. Which works fine for some filters (like {{Zend_Filter_Alpha}}) which have only a single constructor argument anyway. In the second example above, I purposefully changed the order of the array (putting 'replace' first) and the resulting values get switched inside the filter (pattern comes out through {{getReplacement()}}).

So, what's the game plan? Zend_Form_Element->_loadFilter is obviously broken, almost to the point of unusability. But this change smells like a BC break to me.

This fix will result in another bug. I mean when $filter['options'] is a string, it will be cast to an unindexed array.

I explain what I mean :


->addFilter('null', 'integer');

This code will work with the current bug. But won't with that fix.

By changing your fix into :


-                $instance = $r->newInstanceArgs((array) $filter['options']);
+                $instance = $r->newInstanceArgs(array($filter['options']));

It will be backward compatible and the second argument of the Zend_Form_Element::addFilter() method will really be the same as the first argument of the filter's constructor