New Whitelist / Blacklist Filter

[gsomoza]

This new filter will behave either as a blacklist or a whitelist. If the value being filtered exists in the list of known values, the filter will return the value if its a whitelist or null if its a blacklist. The opposite will happen if value is not in the list.

Whitelist

$whitelist = new \Zend\Filter\Whitelist(array(
    'list' => array('allowed-1', 'allowed-2')
));
echo $whitelist->filter('allowed-2');   // => 'allowed-2'
echo $whitelist->filter('not-allowed'); // => null

Blacklist

$blacklist = new \Zend\Filter\Whitelist(array(
    'type' => 'blacklist', // optional, default is 'whitelist'
    'list' => array('not-allowed-1', 'not-allowed-2')
));
echo $blacklist->filter('allowed');        // => 'allowed'
echo $blacklist->filter('not-allowed-2');  // => null

Notes

1. Maybe calling the filter class Whitelist might be counter-intuitive if the desired behavior is that of a blacklist. But I couldn't think of a better way to name the filter, so I'm open to suggestions:
   • Something like "AllowedList" would just be a synonym of "whitelist".
   • "List" would just be too vague.
   • Separate filter classes seemed overkill.
2. I decided to allow only one type of configuration in the constructor (by array of options) just to keep it simple. If you'd like to allow for a more dynamic constructor (like a few other filters do) let me know.
3. Not sure what commit edbc12b is doing in this PR. Looks like its in master but not in develop.

See #6960 for more info.

[gsomoza]

Looks like that last commit generated a conflict on Zend\Filter\FilterPluginManager with code in develop. Would you like me to merge develop into this branch and fix the conflict? Or only the branch that introduced the conflict? Let me know.

[Ocramius]

Rebasing: merging back into dev branches is no-go :-)

[gsomoza]

Sure, I can rebase, but should I do it against develop? Rebasing against master would do nothing.

[Ocramius]

It has to happen against develop, yes. master is only for bugfixes

[Ocramius]

I don't think Whitelist should also support TYPE_BLACKLIST.

[gsomoza]

See my note about that in the PR description: should I make it a separate class? Or could you help me find a better name other than "Whitelist" that would encompass both a "whitelist" and a "blacklist"?

[Pittiplatsch]

I'd implement separate classes Whitelist and Blacklist who share their functionality in a common Zebra\AbstractList ;-) with filter() calling an abstract method which is implemented by Zebra\Whitelist and Zebra\Blacklist appropriately -> IOC.
I saw your PR description, but I think, a clean architectural approach should be favoured over one or two less classes.

[gsomoza]

Makes sense, I'll do that then.

[Ocramius]

AbstractList isn't even needed if there's not enough common logic. I suggest making them separate and that's it :-)

[ojhaujjwal]

I agree. Blacklist extending Whitelist is much better way, AbstractList is not needed. For example, Zend\Db\Sql\Predicate\NotIn extends Zend\Db\Sql\Predicate\In

[Ocramius]

I wouldn't use inheritance at all :P They are different (opposite) concept

[gsomoza]

Alright, no inheritance it is. Will have it ready and rebased tomorrow.

[Pittiplatsch]

Waiting for Copy / Paste detector to shout ;-)

[Ocramius]

Waiting for Copy / Paste detector to shout ;-)

I wouldn't worry too much about it, really. Duplication is better than a messed up inheritance for the sake of code reuse. Inheritance is not about code reuse, it's about meaning/type hierarchy.

[gsomoza]

Made two separate classes out of it and rebased.

To continue the discussion about inheritance: since inheritance is not a good fit, maybe in the future we could improve code re-use using Traits?

trait ListFilterTrait {
    protected $options = array(
        'list' => array(),
        'strict' => null,
    );

    public function setStrict($strict) { /* etc */ }
    public function getStrict() { /* etc */ }

    public function setList($list) { /* etc */ }
    public function getList() { /* etc */ }
}

class Whitelist extends AbstractFilter {
    use ListFilterTrait;

    public function filter($value) { /* etc */ }
}

class Blacklist extends AbstractFilter {
    use ListFilterTrait;

    public function filter($value) { /* etc */ }
}

Might be a good workaround to the inheritance problem, just brainstorming.

[Ocramius]

I would suggest coding the entire classes/tests first, and thinking about duplication later on (if it is a problem at all).

[gsomoza]

Yes, that's exactly what I meant. I already uploaded the new Blacklist and Whitelist classes - it should be ready for your review (unless you meant something else?)

[Ocramius]

Can this kind of setter be avoided? (storing strict in the $options array) What are the disadvantages if we had two private properties instead of $this->options['strict'] and $this->options['list']

[gsomoza]

Here we're re-using the AbstractFilter functionality, as it seems it was meant for doing it like this (see the constructor).

If I hadn't extended AbstractFilter I would have used properties instead.

[Ocramius]

AbstractFilter does not enforce this, so I would move to single properties if possible, reducing the coupling to the constructor logic only (not to the inherited property).

[gsomoza]

Ah, didn't realize that. Will make the change.

[gsomoza]

Latest changes are up for review.

[Ocramius]

I would avoid fluent interfaces unless really required. See this blogpost for my reasoning

[gsomoza]

Should I return nothing or the list?

[Ocramius]

@return void is preferable, yes

[akrabat]

Arguably, in this case, returning null is unexpected as I gave the filter a string, I would expect a string back. i.e. It's not clear why a string testing filter would change the type of the data.

[gsomoza]

So basically we're only making it final because its easy to go back to non-final. Honestly I was hoping for a better reason.

But I'm ok with the plan because it actually doesn't affect my project and its your repo after all.

[Ocramius]

So basically we're only making it final because its easy to go back to non-final.

No, it's mainly because I see a lot of the codebase (as of the last weeks of merges) where there is inheritance coupling that is hard to disentangle. If I can avoid it upfront, then I do. Before all the design/clean code discussions it's primarily a management question, as we are really free to redesign the API as we wish if it's final and something goes awry.

[akrabat]

May as well make every non-abstract class in ZF2 final if that's the reasoning :)

[gsomoza]

That's what I meant: if it helps you manage your codebase then I'm ok with it. Ultimately its your decision.

I just generally agree more with @akrabat. From a purely coding point of view I don't see the value of final. Not a big deal though.

[Ocramius]

May as well make every non-abstract class in ZF2 final if that's the reasoning :)

I'm in fact doing that for every new class being introduced

[gsomoza]

I'm in fact doing that for every new class being introduced

On one hand I'm glad to hear that. You're being consistent, not picky.

On the other hand I'd be worried about alienating developers: it doesn't seem to me that ZF should assume the role of enforcing proper use of inheritance, especially in matters of opinion.

More generally speaking: it seems very ambitious to create a framework that "cannot" be abused. You might end up alienating many developers, one edge-case at a time (e.g. returning empty string instead of null).

Planning for good BC is important, but if you make most classes final only because of THAT you will probably lose some of the "agility" that people are looking for in ZF2.

My word of caution is that if you force people to copy/paste when working with ZF2 by making most classes final (assuming BC is the only reason), they might soon start swarming to another framework..

[Ocramius]

it seems very ambitious to create a framework that "cannot" be abused.

I'm not aiming at that. I'm just realizing how many hours I wasted in solving bugs that are trivially solved by being more strict.

you will probably lose some of the "agility" that people are looking for in ZF2.

I'm actually doing that for the sake of agility, as we lost it (completely):

• https://github.com/zendframework/zf2/milestones/3.0.0
• Releases Plan? #6957
• http://www.michaelgallego.fr/blog/2014/10/04/status-of-zend-framework-3/

If we keep building software that allows to be misused in every possible way and allows every possible kind of mutable configuration and inheritance scheme we'll never get out of it, and we'll just make migration path for upgrades a big mess.

they might soon start swarming to another framework

I will actually say that I'm fine with that: trying to solve every use-case and support every kind of approach is wrong in first place. Let's solve few cases and let's do it correctly instead.

[gsomoza]

I see your point: you'll be more agile from a management point of view (e.g. being able to make releases more often), at the expense of being much more strict in what developers can or cannot do with ZF. Seems like a good compromise if you're not shooing for universal adoption.

[Martin-P]

@return bool is used in ZF2.

[weierophinney]

I've typically been against both final and private in the past, as I feel they stymie the ability to explore new use cases for a class (I even blogged about it).

However, over time, as I look at some of the maintenance issues that have arisen, I'm starting to appreciate the idea of private and final much more. As @Ocramius notes, we can always make something more public, or not final, and not break backwards compatibility, but this is not true in the other direction. Doing so, however, requires an onus on the person requesting it: demonstrating a valid, general use case that would require the change. (Reasons for that might include: logic is too long to duplicate in a decorator/proxy; demonstrable need to extend and/or proxy to a member; etc.)

The one reason I do not like final, however, is that "extension" of a class marked final is non-trivial, and requires a fair amount of education and experience in OOP to achieve. As an example, if a developer wants to alter how a method filter works on a class marked final, the only way to do so is via a proxy. Compare:

// Given:
final class Whitelist implements FilterInterface
{
    public function filter($value)
    {
        /* ... */
    }
}

class WhitelistExtension implements FilterInterface
{
    private $whitelist;

    public function __construct(Whitelist $whitelist)
    {
        $this->whitelist = $whitelist;
    }

    public function filter($value)
    {
        /* do something in here with $this->whitelist->filter(). Or not */
    }
}

with:

// Given:
class Whitelist implements FilterInterface
{
    public function filter($value)
    {
        /* ... */
    }
}

class WhitelistExtension extends Whitelist
{
    public function filter($value)
    {
        /* do something in here with parent::filter(). Or not */
    }
}

The point is that in the first case, you need to know what interface is implemented, and then inject an instance of the original into the new class, which requires understanding of dependency injection -- not to mention the various scoping issues you may have. The second case is quite easy for most junior developers to grasp, while the first requires at least a medior-level developer.

I have no problem with using private at this time, and actually encourage it. But I think usage of final is something we should wait until 3.0 for, as the education issues alone will create a barrier to entry for many users, and we need to discuss if that's the direction we should go.

[weierophinney]

I've just had an interesting discussion with @Ocramius and @akrabat in IRC. We have a balance to strike here between maintenance and extensibility. One initial reason for the popularity of ZF1 was the ability to extend anything. However, that ability came with a price: extensibility meant developers expected their extensions would become part of the framework eventually, leading to more code for the project to maintain, and changes to non-public members became an issue for developers who did extend (as we do not guarantee backwards compatibility on non-public members). The point is that marking classes as final and/or marking members as private alleviates many of these maintenance issues. Extension is still possible, just via different means (composition, decoration, proxying, etc.).

The flip side of a strict approach is that the framework becomes less usable for less-experienced developers. That said, not every class would need to follow these rules. The places where they do makes sense, however, are places where the expected input is well-known, and the logic will likely never need to change. Filters are actually a very good use case for both final and private.

At this stage, I'm going to agree with the recommendation from @Ocramius -- with the caveat that we should update the coding standards/guidelines to encourage this and indicate why the practices exist, and that we should add a chapter in the documentation about extension patterns for final classes and/or classes with private members.

[ojhaujjwal]

Finally I get it. +1 about using final and private where it makes sense. 👍

[gsomoza]

I can see the value of using final for ZF2 maintainers. @Ocramius thanks for the educational exchange of ideas.

👍

[samsonasik]

2015 ;)

[weierophinney]

Merged -- now I need @gabrielsomoza to write up docs on the new filter and send a PR to https://github.com/zendframework/zf2-documentation :)

[gsomoza]

Sure, will do - any chance you could give me a deadline? (its easier for me if I can schedule it :) )

[Ocramius]

@gabrielsomoza should be ~2 weeks