Navigation refactoring

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Navigation refactoring

dmitrybelyakov
Hi,

I'm, doing refactoring to Zend\Navigation component and would like some feedback on those changes. 
They are in my branch here: http://bit.ly/zkPeiI

So the changes I made so far:

The Navigation object now allows you to set urlHelper and routeMatch that would be injected into all subsequent 
pages. This will allow your navigation to render and calculate hrefs properly as currently it just thrown exceptions.

There are three use cases:

Case one:

    - get navigation from locator
    - set route match
    - set url helper (can be injected)
    - add pages

Example:

    $navigation = $this->locator->get('Zend\Navigation\Navigation');
    $navigation->setRouteMatch($routeMatch);
    $navigation->setPages($pages);


Case two:

- add everything at construction time

Example:

    $navigation = new \Zend\Navigation\Navigation(
        $pages, $urlHelper, $routeMatch
    );


Case three:

- create navigation (either from di or directly)
- add route match when you need to render

Example:

    $navigation = $this->locator->get('Zend\Navigation\Navigation');
    $navigation->setPages($pages);
    $navigation->setRouteMatch($routeMatch);


I would really love to get some feedback or suggestions on that.
Dmitry.
Reply | Threaded
Open this post in threaded view
|

Re: Navigation refactoring

Frank Brückner
 Hi Dmitry,
 I had already addressed the problem: hard dependency to Mvc\Router and
 View\Helper\Url! This makes it difficult to use Zend\Navigation as a
 stand-alone component. Only the MVC page type needs the routematch
 object and the url helper, but your solution adds this dependency to the
 whole Zend\Navigation component.


 By the way your code includes two errors:

 $page = Zend\Navigation\Page\AbstractPage::factory(array(
     'type'     => 'uri',
     'routeMatch' => '',
 ));

 Fatal error: Call to undefined method 'setRouteMatch'

 $page = Zend\Navigation\Page\AbstractPage::factory(array(
     'type'     => 'uri',
     'urlHelper'  => '',
 ));

 Fatal error: Call to undefined method 'setUrlHelper'

 https://github.com/dmitrybelyakov/zf2/blob/Feature/NavigationRefactoring/library/Zend/Navigation/Page/AbstractPage.php#L200


 Greetings
 Frank

 Am Freitag, den 16.03.2012, 13:27 +0100 schrieb Dmitry Belyakov
 <[hidden email]>:

> Hi,
>
> I'm, doing refactoring to ZendNavigation component and would like
> some
> feedback on those changes. 
> They are in my branch here: http://bit.ly/zkPeiI [1]
>
> So the changes I made so far:
>
> The Navigation object now allows you to set urlHelper and routeMatch
> that would be injected into all subsequent 
> pages. This will allow your navigation to render and calculate hrefs
> properly as currently it just thrown exceptions.
>
> There are three use cases:
>
> Case one:
>
>     - get navigation from locator
>     - set route match
>     - set url helper (can be injected)
>     - add pages
>
> Example:
>
>     $navigation = $this->locator->get('ZendNavigationNavigation');
>     $navigation->setRouteMatch($routeMatch);
>     $navigation->setPages($pages);
>
> Case two:
>
>  - add everything at construction time
>
> Example:
>
>     $navigation = new ZendNavigationNavigation(
>         $pages, $urlHelper, $routeMatch
>     );
>
> Case three:
>
>  - create navigation (either from di or directly)
>  - add route match when you need to render
>
> Example:
>
>     $navigation = $this->locator->get('ZendNavigationNavigation');
>     $navigation->setPages($pages);
>     $navigation->setRouteMatch($routeMatch);
>
> I would really love to get some feedback or suggestions on that.
>  Dmitry.
>
> Links:
> ------
> [1] http://bit.ly/zkPeiI

Reply | Threaded
Open this post in threaded view
|

Re: Navigation refactoring

dmitrybelyakov
This post has NOT been accepted by the mailing list yet.
Hi Frank.

I will be glad to hear any suggestions regarding the router dependency.

An on your error cases: I'm not sure what you refer to here. There's no such thing any more as AbstractPage::factory() as in your examples. This static functionality was moved to Container that now does that pages instantiation in a non-static way:https://github.com/dmitrybelyakov/zf2/blob/Feature/NavigationRefactoring/library/Zend/Navigation/Container.php#L284

Would you please give me an update on the errors you get?

Dmitry.
Reply | Threaded
Open this post in threaded view
|

Re: Navigation refactoring

Frank Brückner
In reply to this post by Frank Brückner
 Hi Dmitry,
 I have seen your changes and your are right, my two code examples can
 not work.

 But why can I set a routematch object to an uri page?

 $container = new Zend\Navigation\Navigation();

 $page = $container->constructPage(array(
     'uri' => '#',
 ));

 $page->setRouteMatch(
     new Zend\Mvc\Router\Http\RouteMatch(array())
 );

 Does this make sense? I think no!
 We need a solution which only creates dependencies to Zend\Mvc\Router
 when the mvc page type is used.

 I have an idea for an implementation and I will check this with some
 example code.


 Greetings
 Frank

 Am Montag, den 19.03.2012, 10:10 +0100 schrieb Dmitry Belyakov
 <[hidden email]>:

> Hi Frank.
>
> I will be glad to hear any suggestions regarding the router
> dependency.
>
> An on your error cases: I'm not sure what you refer to here. There's
> no such thing any more as AbstractPage::factory() as in your
> examples.
> This static functionality was moved to Container that now does that
> pages instantiation in a non-static
>
> way:https://github.com/dmitrybelyakov/zf2/blob/Feature/NavigationRefactoring/library/Zend/Navigation/Container.php#L284
> [1]
>
> Would you please give me an update on the errors you get?
>
> Dmitry.

Reply | Threaded
Open this post in threaded view
|

Re: Navigation refactoring

Dmitry Belyakov

Hi Frank!

RouteMatch is not a hard dependency. It will be ignored unless you provide it. Same is true for UrlHelper.
So you should be fine without it.

The reason you can set it to UriPage is that all pages extend base container class that has those 
properties to be able to inject them into subsequent pages. The idea behind that was to be able to configure
navigation container with di configuration.

Still I would love to see your implementation so ping me when it's ready.

Dmitry.




вторник, 20 марта 2012 г. в 12:28, Frank Brückner написал:

Hi Dmitry,
I have seen your changes and your are right, my two code examples can
not work.

But why can I set a routematch object to an uri page?

$container = new Zend\Navigation\Navigation();

$page = $container->constructPage(array(
'uri' => '#',
));

$page->setRouteMatch(
new Zend\Mvc\Router\Http\RouteMatch(array())
);

Does this make sense? I think no!
We need a solution which only creates dependencies to Zend\Mvc\Router
when the mvc page type is used.

I have an idea for an implementation and I will check this with some
example code.


Greetings
Frank

Am Montag, den 19.03.2012, 10:10 +0100 schrieb Dmitry Belyakov
<[hidden email]>:
Hi Frank.

I will be glad to hear any suggestions regarding the router
dependency.

An on your error cases: I'm not sure what you refer to here. There's
no such thing any more as AbstractPage::factory() as in your
examples.
This static functionality was moved to Container that now does that
pages instantiation in a non-static

way:https://github.com/dmitrybelyakov/zf2/blob/Feature/NavigationRefactoring/library/Zend/Navigation/Container.php#L284
[1]

Would you please give me an update on the errors you get?

Dmitry.

Reply | Threaded
Open this post in threaded view
|

Re: Navigation refactoring

Pádraic Brady
I'm sure, all of us are saying the same thing: "Is it done yet?!" a
thousand times a day ;). Let us know how much changes are needed, and
don't be afraid to put up a wee RFC to explain them (makes it easier
for everyone to review and discuss on one of the weekly meetings!).
Note also - add it for discussion to a meeting agenda when you're
ready.

Now, back to work. No slacking.

Paddy

On Tue, Mar 20, 2012 at 9:48 AM, Dmitry Belyakov <[hidden email]> wrote:

>
> Hi Frank!
>
> RouteMatch is not a hard dependency. It will be ignored unless you provide
> it. Same is true for UrlHelper.
> So you should be fine without it.
>
> The reason you can set it to UriPage is that all pages extend base container
> class that has those
> properties to be able to inject them into subsequent pages. The idea behind
> that was to be able to configure
> navigation container with di configuration.
>
> Still I would love to see your implementation so ping me when it's ready.
>
> Dmitry.
>
>
>
>
> вторник, 20 марта 2012 г. в 12:28, Frank Brückner написал:
>
> Hi Dmitry,
> I have seen your changes and your are right, my two code examples can
> not work.
>
> But why can I set a routematch object to an uri page?
>
> $container = new Zend\Navigation\Navigation();
>
> $page = $container->constructPage(array(
> 'uri' => '#',
> ));
>
> $page->setRouteMatch(
> new Zend\Mvc\Router\Http\RouteMatch(array())
> );
>
> Does this make sense? I think no!
> We need a solution which only creates dependencies to Zend\Mvc\Router
> when the mvc page type is used.
>
> I have an idea for an implementation and I will check this with some
> example code.
>
>
> Greetings
> Frank
>
> Am Montag, den 19.03.2012, 10:10 +0100 schrieb Dmitry Belyakov
> <[hidden email]>:
>
> Hi Frank.
>
> I will be glad to hear any suggestions regarding the router
> dependency.
>
> An on your error cases: I'm not sure what you refer to here. There's
> no such thing any more as AbstractPage::factory() as in your
> examples.
> This static functionality was moved to Container that now does that
> pages instantiation in a non-static
>
> way:https://github.com/dmitrybelyakov/zf2/blob/Feature/NavigationRefactoring/library/Zend/Navigation/Container.php#L284
> [1]
>
> Would you please give me an update on the errors you get?
>
> Dmitry.
>
>



--
Pádraic Brady

http://blog.astrumfutura.com
http://www.survivethedeepend.com
Zend Framework Community Review Team
Reply | Threaded
Open this post in threaded view
|

Re: Navigation refactoring

dmitrybelyakov

Hi Pádraic .

I decided to put it on the list first to get some feedback and figure out if it's worth an RFC since it's just 
a tiny refactoring with a couple of additional methods.

Still I would be happy if you could find a minute to try it and tell me what you think.

Dmitry.


среда, 21 марта 2012 г. в 19:21, Pádraic Brady написал:

I'm sure, all of us are saying the same thing: "Is it done yet?!" a
thousand times a day ;). Let us know how much changes are needed, and
don't be afraid to put up a wee RFC to explain them (makes it easier
for everyone to review and discuss on one of the weekly meetings!).
Note also - add it for discussion to a meeting agenda when you're
ready.

Now, back to work. No slacking.

Paddy

On Tue, Mar 20, 2012 at 9:48 AM, Dmitry Belyakov <[hidden email]> wrote:

Hi Frank!

RouteMatch is not a hard dependency. It will be ignored unless you provide
it. Same is true for UrlHelper.
So you should be fine without it.

The reason you can set it to UriPage is that all pages extend base container
class that has those
properties to be able to inject them into subsequent pages. The idea behind
that was to be able to configure
navigation container with di configuration.

Still I would love to see your implementation so ping me when it's ready.

Dmitry.




вторник, 20 марта 2012 г. в 12:28, Frank Brückner написал:

Hi Dmitry,
I have seen your changes and your are right, my two code examples can
not work.

But why can I set a routematch object to an uri page?

$container = new Zend\Navigation\Navigation();

$page = $container->constructPage(array(
'uri' => '#',
));

$page->setRouteMatch(
new Zend\Mvc\Router\Http\RouteMatch(array())
);

Does this make sense? I think no!
We need a solution which only creates dependencies to Zend\Mvc\Router
when the mvc page type is used.

I have an idea for an implementation and I will check this with some
example code.


Greetings
Frank

Am Montag, den 19.03.2012, 10:10 +0100 schrieb Dmitry Belyakov
<[hidden email]>:

Hi Frank.

I will be glad to hear any suggestions regarding the router
dependency.

An on your error cases: I'm not sure what you refer to here. There's
no such thing any more as AbstractPage::factory() as in your
examples.
This static functionality was moved to Container that now does that
pages instantiation in a non-static

way:https://github.com/dmitrybelyakov/zf2/blob/Feature/NavigationRefactoring/library/Zend/Navigation/Container.php#L284
[1]

Would you please give me an update on the errors you get?

Dmitry.



--
Pádraic Brady

http://blog.astrumfutura.com
http://www.survivethedeepend.com
Zend Framework Community Review Team

Reply | Threaded
Open this post in threaded view
|

Re: Navigation refactoring

Frank Brückner
 Hi Dmitry,
 look here: https://github.com/zendframework/zf2/pull/952

 Frank

 Am Mittwoch, den 21.03.2012, 16:49 +0100 schrieb Dmitry Belyakov
 <[hidden email]>:

> Hi Pádraic .
>
> I decided to put it on the list first to get some feedback and figure
> out if it's worth an RFC since it's just
> a tiny refactoring with a couple of additional methods.
>
> Still I would be happy if you could find a minute to try it and tell
> me what you think.
> You can find it in my branch here:
>
> https://github.com/dmitrybelyakov/zf2/tree/Feature/NavigationRefactoring
> [1]
>
> Dmitry.