[ZF3] [WIP] Refactor Filter component #4575

bakura10 · 2013-06-02T10:53:10Z

Hi,

As the discussion of ZF 3 is starting, I'm trying to refactor some components that need it. As @weierophinney and @Ocramius knows I'm starting to refactor InputFilter, but in order to make it properly, some changes needs to be done in the Filter and Validator components too.

I'm starting the Filter one as this is the most easy one. Here are current things I've done:

To be more homogeneous, each filter that needs options extend the AbstractOptions class. Now options must be specified using underscore_separated.
As a consequence, I've removed completely the fluent interface (as now most options will be specified through the constructor directly).
As ZF3 will be PHP 5.4, I've removed a lot of boilerplate code to take advantage of Callable typehint.
I've simplified drastically the FilterChain implementation. It was doing too much things (the setOptions acted as a factory to create filter using config, but it's not the task of the Filter chain). The creation should be hnadled by the Factory of the InputFilter class. Now, because the FilterChain does not create filters on itself, it does not have reference to the FilterPluginMnager. It will make things A LOT simpler for components that rely on Filter component like InputFilter. Injection will be simpler.
Each filters that will have dependencies will receive its own factory.
There is now an adapter plugin manager for compression filters.
There is now an adapter plugin manager for encryption filters.
Digits filter now uses filter_var internally.
In Null and Boolean filters : there was options as int and string. I removed the string as it added some overhead for little gain. People can use bit operator to combine multiple options, like in standard PHP.
Odd naming in HtmlEntities have been changed (from tagsAllowed to allowedTags).

Any thoughts?

Questions:

Should the StaticFilter be removed ? It makes things a bit harder, as it requires a FilterPluginManager but we cannot inject it easily because of the static methods.

TODO:

Remove the "extends AbstractOptions" from AbstractFilter.
Clean the Boolean, all File/* filters and Inflector filter
Properly write all factories for filters that have dependencies
Update tests
Full review of code
Update doc

marc-mabe · 2013-06-06T10:30:16Z

library/Zend/Filter/AbstractUnicode.php

-                ));
-            }
+        $encoding    = strtolower($encoding);
+        $mbEncodings = array_change_key_case(mb_list_encodings(), CASE_LOWER);


you are converting numeric keys to lower case here.

Oops... thx !

marc-mabe · 2013-06-06T12:03:27Z

library/Zend/Filter/HtmlEntities.php

            $enc      = $this->getEncoding();
-            $value    = iconv('', $this->getEncoding() . '//IGNORE', (string) $value);
-            $filtered = htmlentities($value, $this->getQuoteStyle(), $enc, $this->getDoubleQuote());
+            $value    = iconv('', $this->encoding . '//IGNORE', (string) $value);


Do you have a comment what this line is for?

bakura10 · 2013-06-06T15:17:18Z

@marc-mabe I changed the logic in AbstractUnicode, you were right. It's better now as it checks for the whole extension ;-).

bakura10 · 2013-08-13T20:26:08Z

Some questions that came up while studying again the code: should we move all the Encrypt filter to the Zend\Crypt namespace ? We need a consensus on that, because a lot of filters are already under the Zend\I18n namespace, for instance.

marc-mabe · 2013-08-14T03:41:14Z

@bakura10 I think it's a good idea having it under Zend\Crypt

bakura10 · 2013-09-02T13:44:53Z

I've made some progress on this one. The most importnat is the FilterChain: https://github.com/bakura10/zf2/blob/373c306119f4056ceed670c6ddfb116428e6c2dd/library/Zend/Filter/FilterChain.php#L39

It now has a hard dependency on teh filter plugin manager. The FilterChain has a built-in factory: https://github.com/bakura10/zf2/blob/373c306119f4056ceed670c6ddfb116428e6c2dd/library/Zend/Filter/Factory/FilterChainFactory.php

I'm not sure about the solution: some people may complaint it now needs a dependency to ServiceManager component. However, I think it's nice to enforce the dependency to the plugin manager in order to avoid the problem we had in ZF2, in forms for instance where the factory had a new filter plugin manager rather than the global one, hence people added some custom filters, but couldn't use them in the code because the plugin manager was a newly created one.

By enforcing this and using factories even for ZF code, this problem will be solved nicely.

marc-mabe · 2013-09-02T16:32:12Z

library/Zend/Filter/Callback.php

        array_unshift($params, $value);

-        return call_user_func_array($this->options['callback'], $params);
+        return $this->callback($params);


Are you sure this works? - I toughed it requires to define $this->callback as a local variable $cb = $this->callback; $cb();
You have to make sure setCallback() will convert Class::staticMethod like callbacks into array('Class', 'staticMethod').

I think it works in PHP 5.4 as soon as $this->callback is Callable (so anything like array('ClassName', 'methodName'), array($object, 'methodName'), Closure, object that implments __invoke...

Nope: http://3v4l.org/geAhT

array('Class', 'method') vs. Class::method : http://3v4l.org/tqrAS

So basically I just need to check if there are :: and separated this into array ?

You can't remove call_user_func_array because $params is an array that should be given as separated arguments.
But you can check if $params is empty:

if ($this->callbackParams) { $params = $this->callbackParams; array_unshift($params, $value); return call_user_func_array($this->callback, $params); } else { $callback = $this->callback; return $callback($value); }

... check about :: on setCallback() and split it into an array

bakura10 · 2013-09-06T13:40:05Z

library/Zend/Filter/Boolean.php

     */
-    protected $constants = array(


This boolean filter is honestly really big and I'm wondering if we need so much lines of code for such a simple thing. I'd be in favour of replacing it by the built-in PHP boolean filter:

filter_var($value, FILTER_VALIDATE_BOOLEAN)

We loose some features (like localization and chooisng to exclude some conditions), but for most cases, it works the same ("true" is evaluated to true, "false" to false, 0 to false, 1 to true...).

What do you think?

EDIT : I've tried all the use cases we have and it behaves exactly the same. The only thing we loose is the localization stuff.

In my opinion the localization stuff should be moved to Zend\I18n

I agree too, I've found it very strange to find this feature in Boolean filter.

Now the question is that this filter allows to tune very fine the behavior, so that you can configure it so that the string "true" return "false" when filtered. However I'm wondering if this is really a needed feature, and if we couldn't accept to loose some extreme edge-cases in favor of simpler implementation.

Why not simple make it a boolean casting BUT allow to define a list of possible true/false values. Such a list isn't localization because it's not required to translate something and you can define something like no/yes - true/false - on/off etc.
(An own filter for a simple cast is very heavy - not required)

Well, that's what does the built-in filter.

echo filter_var("true", FILTER_VALIDATE_BOOLEAN); // echo (bool) true
echo filter_var("foo", FILTER_VALIDATE_BOOLEAN); // echo (bool) false
echo filter_var(null, FILTER_VALIDATE_BOOLEAN); // echo (bool) false

I think this filter will handle 95% of use cases without the all overhead. People that want a very specific filtring can implement a callback filter.

sounds good to me

The problem is that we have these features there for a reason: developers
use them, and keep requesting them. I did the Boolean filter as a simple
one-off originally when refactoring for ZF2, and immediately had requests
to re-instate the original features.

That said, we may be able to have a Boolean filter and a "LocalizedBoolean"
filter in order to segregate the functionality.

On Fri, Sep 6, 2013 at 8:40 AM, Michaël Gallego notifications@github.comwrote:

In library/Zend/Filter/Boolean.php:

*/

protected $constants = array(

This boolean filter is honestly really big and I'm wondering if we need so
much lines of code for such a simple thing. I'd be in favour of replacing
it by the built-in PHP boolean filter:

filter_var($value, FILTER_VALIDATE_BOOLEAN)

We loose some features (like localization), but for most cases, it works
the same ("true" is evaluated to true, "false" to false, 0 to false, 1 to
true...).

What do you think?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/4575/files#r6207866
.

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

People were asking what? The ability to consider the string "true" as the boolean "false" ? Or only the localized feature? If so, we could keep as you said this simple boolean and a LocalizedBoolean in in I18n.

bakura10 · 2013-09-06T16:29:42Z

@danizord, Filters no longer extends AbstractOptions. Does it look better to you? ;-)

bakura10 · 2013-09-06T16:30:15Z

@marc-mabe , I gave the Boolean filter a try. Does it look good to you or should I revert back?

bakura10 · 2013-09-06T20:53:41Z

Soooo.... I've made a lot of progress today. I've continued to refactor a lot of filters, and move all crypt filters to the Zend\Crypt namespace. I've also made a lot of factories for each filter that needs dependency.

I hope someone will be able to help me to fix all the tests, it's gonna be long and booooring.... :/

bakura10 · 2013-09-09T19:02:21Z

Replaced by #5097

bakura10 added 7 commits June 2, 2013 12:35

First step to refactor filter component

002e5fe

Simplify filter chain

4e56248

Add compression adapter plugin manager

79aa2e5

Add plugin manager for encryp

068f7f2

Refactor Word filters

9dd1c87

Add factories

e7c9182

Inflector

6d6eb18

marc-mabe reviewed Jun 6, 2013
View reviewed changes

Revert back unicode

68262b2

marc-mabe reviewed Jun 6, 2013
View reviewed changes

Move exception support to constructor

b7f49ab

Add comment for iconv

f378ef5

marc-mabe mentioned this pull request Aug 9, 2013

Inherit correctly the class Zend\Filter\AbstractUnicode #4937

Closed

bakura10 added 3 commits August 12, 2013 16:04

Make sure to always call getEncoding

b72e033

Remove useless check

19f5357

Various cleanup

63a9b56

bakura10 added 5 commits August 20, 2013 21:32

Never mind, thats useful

f8f114a

Update for PHP 5.4 better performance

5a8349e

Properly inject filter chain

373c306

Fix english

205f479

Fix

20be921

marc-mabe reviewed Sep 2, 2013
View reviewed changes

Allow to remove filters from filter chain

895e024

bakura10 added 4 commits September 5, 2013 17:32

Fix logic in Callable filter

30aaba4

Remove useless method

9d29c68

Allow to add options

299c5e1

Start to write tests

94b090e

bakura10 reviewed Sep 6, 2013
View reviewed changes

bakura10 added 3 commits September 6, 2013 17:26

Remove dependency to compression

fe53180

Remove dependency to AbstractOptions

7331572

Cleaning

0d1d6bd

bakura10 added 4 commits September 6, 2013 19:17

Move crypt filters to Crypt component

8fee5c1

Rename lot of classes

cd33be2

BlockCipher

f80baf0

Remove useless filter

be8a6a9

bakura10 added 3 commits September 7, 2013 12:40

Foo

41af2ed

Allow to add filters to filter chain

a64fa7c

Fix some tests

877ac92

bakura10 closed this Sep 9, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ZF3] [WIP] Refactor Filter component #4575

[ZF3] [WIP] Refactor Filter component #4575

bakura10 commented Jun 2, 2013

marc-mabe Jun 6, 2013

bakura10 Jun 6, 2013

marc-mabe Jun 6, 2013

bakura10 commented Jun 6, 2013

bakura10 commented Aug 13, 2013

marc-mabe commented Aug 14, 2013

bakura10 commented Sep 2, 2013

marc-mabe Sep 2, 2013

bakura10 Sep 2, 2013

Ocramius Sep 2, 2013

marc-mabe Sep 3, 2013

bakura10 Sep 3, 2013

marc-mabe Sep 4, 2013

bakura10 Sep 6, 2013

marc-mabe Sep 6, 2013

bakura10 Sep 6, 2013

marc-mabe Sep 6, 2013

bakura10 Sep 6, 2013

marc-mabe Sep 6, 2013

weierophinney Sep 6, 2013

bakura10 Sep 6, 2013

bakura10 commented Sep 6, 2013

bakura10 commented Sep 6, 2013

bakura10 commented Sep 6, 2013

bakura10 commented Sep 9, 2013

[ZF3] [WIP] Refactor Filter component #4575

[ZF3] [WIP] Refactor Filter component #4575

Conversation

bakura10 commented Jun 2, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bakura10 commented Jun 6, 2013

bakura10 commented Aug 13, 2013

marc-mabe commented Aug 14, 2013

bakura10 commented Sep 2, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bakura10 commented Sep 6, 2013

bakura10 commented Sep 6, 2013

bakura10 commented Sep 6, 2013

bakura10 commented Sep 9, 2013