Issues

ZF-9994: Zend_Navigation_Container improve filtering

Issue Type: Improvement Created: 2010-06-15T16:47:54.000+0000 Last Updated: 2013-01-22T08:32:59.000+0000 Status: Resolved Fix version(s): - 1.12.2 (25/Feb/13)

Reporter: Michiel Thalen (chielsen) Assignee: Frank Brückner (frosch) Tags: - Zend_Navigation

  • After1.12.0
  • state:patch-ready-for-review
  • zf-crteam-padraic
  • zf-crteam-priority
  • zf-crteam-review

Related issues: - ZF-10459

Attachments: - Container.php.patch

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.

<pre class="highlight">
    <?
    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:

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

Comments

Posted by Kai Uwe (kaiuwe) on 2011-06-09T07:59:42.000+0000

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

<pre class="highlight">
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;
            }
        }        
    }

    // …
}

Posted by Kai Uwe (kaiuwe) on 2011-06-09T08:09:16.000+0000

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

<pre class="highlight">
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;
}

Posted by Frank Brückner (frosch) on 2011-09-22T10:29:53.000+0000

Patch and unit tests added.

Posted by Frank Brückner (frosch) on 2011-09-22T11:16:34.000+0000

Sorry, here are the correct files!

Posted by Michiel Thalen (chielsen) on 2011-09-23T10:47:48.000+0000

What about my improvements?

Posted by Frank Brückner (frosch) on 2011-09-23T10:55:50.000+0000

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

Your example works:

<pre class="highlight">
$container->findAllByClass('my-class', false);

Posted by Frank Brückner (frosch) on 2011-09-23T11:04:14.000+0000

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

Posted by Michiel Thalen (chielsen) on 2011-09-23T11:08:25.000+0000

You must mean ```

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

Posted by Frank Brückner (frosch) on 2011-09-23T11:15:24.000+0000

{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! ;)

Posted by Michiel Thalen (chielsen) on 2011-09-23T11:27:38.000+0000

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.

Posted by Frank Brückner (frosch) on 2011-09-23T11:35:39.000+0000

Ideas and code examples are always welcome!

Thanks for your feedback.

Posted by Frank Brückner (frosch) on 2011-09-23T12:49:08.000+0000

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

Posted by Frank Brückner (frosch) on 2011-10-06T10:10:53.000+0000

I will replace strpos with preg_match.

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

Posted by Michiel Thalen (chielsen) on 2011-10-06T11:01:37.000+0000

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?

Posted by Frank Brückner (frosch) on 2011-10-06T11:06:47.000+0000

New patch and unit tests includes preg_match

Posted by Frank Brückner (frosch) on 2011-10-06T11:11:43.000+0000

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

Thanks.

Posted by Michiel Thalen (chielsen) on 2011-10-06T11:23:32.000+0000

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.

Posted by Frank Brückner (frosch) on 2011-10-06T11:47:44.000+0000

{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.

Posted by Michiel Thalen (chielsen) on 2011-10-06T11:52:14.000+0000

{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?

Posted by Frank Brückner (frosch) on 2011-10-06T12:01:10.000+0000

{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:

<pre class="highlight">
$container->findOneBy('label', 'foo');
$container->findOneBy('label', '/([o]{2})/', true);

With my code:

<pre class="highlight">
$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. :)

Posted by Michiel Thalen (chielsen) on 2011-10-06T12:17:13.000+0000

Ok it's slight shorter do to a search.

<pre class="highlight">
$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?

Posted by Frank Brückner (frosch) on 2011-10-06T12:29:21.000+0000

{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.

Posted by Frank Brückner (frosch) on 2011-10-06T13:05:57.000+0000

New round, new luck! ;)

Posted by Michiel Thalen (chielsen) on 2011-10-06T13:18:47.000+0000

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

Posted by Frank Brückner (frosch) on 2011-10-06T14:09:06.000+0000

@Michiel I had overlooked. Sorry!

Posted by Frank Brückner (frosch) on 2011-10-06T15:23:58.000+0000

Docblocks updated.

Posted by Frank Brückner (frosch) on 2012-03-22T12:30:47.000+0000

Docblocks updated.

Posted by Frank Brückner (frosch) on 2013-01-22T08:32:59.000+0000

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

Thanks to Michiel!

Have you found an issue?

See the Overview section for more details.

Copyright

© 2006-2016 by Zend, a Rogue Wave Company. Made with by awesome contributors.

This website is built using zend-expressive and it runs on PHP 7.

Contacts