Issues

ZF-9994: Zend_Navigation_Container improve filtering

Description

The filters now is a bit basic. For instance if you add multiple classes to a page "one two" and you will search for findByClass('one'), it will return empty.

I propose to alter the findOneBy / findAllBy /findBy functions to add smarter filtering with regex.


    <?
    public function findOneBy($property, $value, $regex = false)
    {
        $iterator = new RecursiveIteratorIterator($this,
                            RecursiveIteratorIterator::SELF_FIRST);
        
        foreach ($iterator as $page) {
            if ($regex ? preg_match($value, $page->get($property)) : $page->get($property) == $value) {
                return $page;
            }
        }
        return null;
    }
    
    public function findAllBy($property, $value, $regex = false)
    {
        $found = array();

        $iterator = new RecursiveIteratorIterator($this,
                            RecursiveIteratorIterator::SELF_FIRST);     
        
        foreach ($iterator as $page) {
            if ($regex ? preg_match($value, $page->get($property)) : $page->get($property) == $value) {
                $found[] = $page;
            }
        }
        return $found;
    }
    
    public function findBy($property, $value, $all = false, $regex = false)
    {
        if ($all) {
            return $this->findAllBy($property, $value, $regex);
        } else {
            return $this->findOneBy($property, $value, $regex);
        }
    }
    
    public function __call($method, $arguments)
    {
        if (@preg_match('/(find(?:One|All)?By)(.+)/', $method, $match)) {
            return $this->{$match[1]}($match[2], $arguments[0], !empty($arguments[1]));
        }

        require_once 'Zend/Navigation/Exception.php';
        throw new Zend_Navigation_Exception(sprintf(
                'Bad method call: Unknown method %s::%s',
                get_class($this),
                $method));
    }

You can then still use it as it works now, but also:


$container->findAllByClass('/\s?my-class\s?/', true);
// It will also find this your-class my-class

Comments

My suggestion: the use of the method as simple as possible!


public function findOneBy($property, $value, $strict = true)
{
    // …
    
    if (false === $strict) {
        foreach ($iterator as $page) {
            if (false !== strpos($page->get($property), $value)) {
                return $page;
            }
        }
    } else {
        foreach ($iterator as $page) {
            if ($page->get($property) == $value) {
                return $page;
            }
        }        
    }

    // …
}

Another idea: Respect the page properties with arrays ("rel" and "rev")


public function findOneBy($property, $value)
{
    $iterator = new RecursiveIteratorIterator($this,
                        RecursiveIteratorIterator::SELF_FIRST);

    foreach ($iterator as $page) {
        $pageProperty = $page->get($property);
        
        if (is_array($pageProperty)) {
            foreach ($pageProperty as $item) {
                if (is_array($item)) {
                    if (in_array($value, $item)) {
                        return $page;
                    }
                } else {
                    if ($item == $value) {
                        return $page;
                    }
                }
            }                
            
            continue;
        }
        
        if ($pageProperty == $value) {
            return $page;
        }
    }

    return null;
}

Patch and unit tests added.

Sorry, here are the correct files!

What about my improvements?

Hi Michiel, basically: keep the use as easy as possible.

Your example works:


$container->findAllByClass('my-class', false);

Sorry, the previous patch did not include the method "findBy"!

You must mean ```

But then you could also have "my-class2" and that will be found as well.

{quote}You must mean…{quote} Yes, you are right! {quote}But then you could also have "my-class2" and that will be found as well.{quote} Regex vs. easy to use! ;)

Well it stays easy to use if you don't want smart filtering. If you do, you will always end up with some kind of advanced syntax to let know what you want (and what not). With this change you will only end to finding too much. I agree that you shouldn't make filtering with regex a default, because of complexity and speed. However for usability a regEx is so much more convenient. I always thought that ZF should be as flexible as possible.

Btw i could easily refactor my code to make it smaller.

Ideas and code examples are always welcome!

Thanks for your feedback.

Okay, I have another idea. I will check this with some unit tests.

I will replace {{strpos}} with {{preg_match}}.


if (false === $strict && false === @preg_match($value, '')) {
    $value = sprintf('%2$s%s%2$s', $value, $delimiter); // Example: '~my-class~'
}

You lost me here. Where are you going to put this? What are you going to do with value? Where is the delimiter coming from?

Also you don't want to test for regex and see if an error occurs and then silence it. You just have to assume it's a regex and when it fails you will get an error. What if want to use a regex but made a typo?

All in all, what is wrong with my solution?

New patch and unit tests includes preg_match

Hi Michiel, I was too slow! ;) I have attached a new patch. Please look at this and give me a feedback.

Thanks.

Well, regex is not so fast already. If you choose to use it, please do not add extra overhead by checking if it's really a regex. That's why i would call the param regex in stead of strict.

If you look at your documentation you actually suggesting it: {quote} if true property and value must equal. if false PHP's preg_match is used. Default is true. {/quote}

Your has alot of duplication as well. Check out my code for instance. Also you are forgetting to change the __call method.

But what i wrong with my code?? I'm using this for a year already and it's fine. I can add the rev / rel stuff if you want.

{quote}Well, regex is not so fast already. If you choose to use it, please do not add extra overhead by checking if it's really a regex.{quote} You are right, a performance check is needed. I will use Xdebug and my test container with 10.000 pages.

{quote}Also you are forgetting to change the __call method.{quote} Ahh, I've forgotten! I will update the patch.

{quote}But what i wrong with my code?? I'm using this for a year already and it's fine.{quote} Nobody said that your code is wrong! I just wanted a simple use and support for rel and rev - no more.

{quote}I can add the rev / rel stuff if you want.{quote} That would be nice. (You can send me an e-mail or use Pastebin/Gist/…)

Thanks for your fast feedback.

{quote} Nobody said that your code is wrong! I just wanted a simple use and support for rel and rev - no more. {quote}

Well the use is exactly the same. The only thing what is missing is the rev/rel support. So you didn't say it was wrong, but why are you writing your own code that is heavier and does the same then?

{quote}Well the use is exactly the same. The only thing what is missing is the rev/rel support. So you didn't say it was wrong, but why are you writing your own code that is heavier and does the same then?{quote} With your code:


$container->findOneBy('label', 'foo');
$container->findOneBy('label', '/([o]{2})/', true);

With my code:


$container->findOneBy('label', 'foo'); // finds only "foo"
$container->findOneBy('label', 'foo', false); // finds also "foo foo" <--- Very easy to use!
$container->findOneBy('label', '/([o]{2})/', false);

$container->findOneBy('rel', 'Example.org');
$container->findOneBy('rel', 'Example', false);
$container->findOneBy('rel', '?Ex([a-z]*)?', false);

The only heavy thing is the support for rel and rev. :)

Ok it's slight shorter do to a search.


$container->findOneBy('label', 'foo', false); // yours
$container->findOneBy('label', '/foo/', false); // mine

But this is magic that is too powerfull. What if i later on alter my navigation and add a label 'foobar'. I might have forgotten about this find all thingy. So i don't think you want to use this very often. And if you really want to, it's only 2 extra chars. But the major downside is you get a performance penalty every request (it's hard to cache). Also it's more obvious to a programmer that he is doing a fullsearch.

And your documentation suggests that i have to put in a regex, but i don't really have to?

{quote}But the major downside is you get a performance penalty every request (it's hard to cache).{quote} My performance test is still open!

By the way it is a small fix to change from {{$strict = true}} to {{$regex = false}}.

New round, new luck! ;)

Looks more like it. Why did you change my __call? Now you cannot pass for instance 0 or 1.

@Michiel I had overlooked. Sorry!

Docblocks updated.

Docblocks updated.

Fixed on trunk (25236) and release-1.12 (25237)

Thanks to Michiel!