Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

#4996 broke File filters management #5050

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

Slamdunk commented Aug 30, 2013

I've found PR #4996 broke how custom file input filter specification is handled; both 2.2.3 and 2.2.4 releases are affected.

I've tried hard to figure out why, but I'm sorry I was not able to 😕

Here the tests: note that both assertions fail, not only the first.

@samsonasik samsonasik commented on an outdated diff Aug 30, 2013

...st/Form/TestAsset/FileInputFilterProviderFieldset.php
@@ -0,0 +1,48 @@
+<?php
+/**
+ * Zend Framework (http://framework.zend.com/)
+ *
+ * @link http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2013 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license http://framework.zend.com/license/new-bsd New BSD License
+ * @package Zend_Form

@samsonasik samsonasik commented on an outdated diff Aug 30, 2013

...ndTest/Form/TestAsset/FileInputFilterProviderForm.php
@@ -0,0 +1,27 @@
+<?php
+/**
+ * Zend Framework (http://framework.zend.com/)
+ *
+ * @link http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2013 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license http://framework.zend.com/license/new-bsd New BSD License
+ * @package Zend_Form

@weierophinney weierophinney commented on the diff Sep 3, 2013

...st/Form/TestAsset/FileInputFilterProviderFieldset.php
+ parent::__construct($name, $options);
+
+ $this->add(array(
+ 'type' => 'file',
+ 'name' => 'file_field',
+ 'options' => array(
+ 'label' => 'File Label',
+ ),
+ ));
+
+ }
+
+ public function getInputFilterSpecification()
+ {
+ return array(
+ 'file_field' => array(
@weierophinney

weierophinney Sep 3, 2013

Owner

There's a flaw in the test here. You need to specify an input type in order for the merge order to work correctly; otherwise, a normal Input will be created, and the FileInput settings merged to it, which will result in the wrong context.

That said, even with 'type' => 'Zend\InputFilter\FileInput' in your configuration, you still get an error -- just a different one (duplicated filters).

weierophinney added a commit that referenced this pull request Sep 3, 2013

weierophinney added a commit that referenced this pull request Sep 3, 2013

[#5050] Fix for break in file input specifications
- File inputs inside fieldsets were causing issues.
- Issue was due to removal of a `$fieldset === $this` condition when
  determining whether or not to attach the input filter specification
  for a fieldset.
- Tests had an issue as well: the input specification needed to indicate
  the input type in order to ensure the correct `FileInput` type is
  created on the initial seeding of the input filter.

weierophinney added a commit that referenced this pull request Sep 3, 2013

@ghost ghost assigned weierophinney Sep 3, 2013

Contributor

Slamdunk commented Sep 4, 2013

@weierophinney so from release 2.2.3, the default behaviour is changed: till 2.2.2 https://github.com/zendframework/zf2/blob/release-2.2.2/library/Zend/Form/Element/File.php#L46-L53 was enough and it didn't need to be specified a second time in a custom getInputFilterSpecification.

This is a compatibility break:

  1. If it was intentional, please document it on changelogs (I can't see it in http://framework.zend.com/blog/zend-framework-2-2-3-released.html)
  2. If it was not intentional, I think the 2.2.2 default behaviour should be restored
Owner

weierophinney commented Sep 4, 2013

The bug fix for 2.2.3 was intentional, and documented. You were relying on
behavior that was neither documented nor tested - in other words, you were
relying on the bug itself. As such, we did not cover it in the changelog,
as we did not know developers were utilizing it.

This fix I introduce here covers a new issue introduced with #4996, which
was the duplicate application of input filter specifications, and which was
uncovered by your test case once I updated it to include the input type.

When we release 2.2.5, I will note the change, and also point out the
changes users should make to their input filter specifications.

On Wednesday, September 4, 2013, Filippo Tessarotto wrote:

@weierophinney https://github.com/weierophinney so from release 2.2.3,
the default behaviour is changed: till 2.2.2
https://github.com/zendframework/zf2/blob/release-2.2.4/library/Zend/Form/Element/File.php#L46-L53was enough and it didn't need to be specified a second time in a custom
getInputFilterSpecification.

This is a compatibility break:

  1. If it was intentional, please document it on changelogs (I can't
    see it in
    http://framework.zend.com/blog/zend-framework-2-2-3-released.html)
  2. If it was not intentional, I think the 2.2.2 default behaviour
    should be restored


Reply to this email directly or view it on GitHubhttps://github.com/zendframework/zf2/pull/5050#issuecomment-23768758
.

Matthew Weier O'Phinney
matthew@weierophinney.net
http://mwop.net/

Contributor

Slamdunk commented Sep 4, 2013

Ok, I understand.
I hope you'll realize that the following paragraphs are not polemic, but just a way to understand if ZF2 users can get it.

If type key is not mandatory, a new developer would think it is inherited.
Otherwise a weird situation could happens:

  1. Write down a new My\Form, tests pass
  2. Write a custom getInputFilterSpecification, tests fail
  3. Why tests fail? Oh, I didn't write the type key
  4. I need to look at each original Zend\Form\Element i'm using to see which type it needs

This is strange, isn't it?

If it sounds strange for you too, either type must be mandatory or must be inherited.

If this is ok for you, at least must be well documented on http://zf2.readthedocs.org/en/latest/modules/zend.form.quick-start.html#hinting-to-the-input-filter to avoid debugging headaches.

Owner

weierophinney commented Sep 4, 2013

@Slamdunk We have a default type, Zend\InputFilter\Input. This is reasonable for 99% of use cases. The only time it is not is when you have specialized input types, such as the FileInput.

It does not make sense for the most common use case to require that people add the type. However, it's not unreasonable to require it for those cases where a specialized type is being used.

As such, the exceptional case needs to be documented. Would you be willing to do a PR against the documentation for that?

Contributor

Slamdunk commented Sep 5, 2013

@weierophinney what about something like this #5079?

I will also PR against the doc.

Slamdunk added a commit to Slamdunk/zf2-documentation that referenced this pull request Sep 6, 2013

weierophinney added a commit to zendframework/zf2-documentation that referenced this pull request Sep 6, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment