Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

[Navigation] Introducing NavigationAbstractServiceFactory #7245

Conversation

sandrokeil
Copy link
Contributor

According to the feedback of #6717 I've created the Navigation Abstract Service Factory. If this abstract service factory is registered, a service with name Zend\Navigation\Special will use the special definition in configuration key navigation.

With one point I'm not satisfied. A service call must start with Zend\Navigation\ (e.g. $sl->get('Zend\Navigation\Special')). Maybe we could support navigation.special too?

I use the ConstructedNavigationFactory to create the Navigation Container. Is this ok or should I use the AbstractNavigationFactory (@weierophinney)? The problem here is that we have to implement the getName() and override the getPages() method. For the future (ZF 3.0) we could remove getName() and make getPages() abstract in AbstractNavigationFactory. This makes DefaultNavigationFactory and ConstructedNavigationFactory needless. But it's a BC break.

I will also update the documentation.

@weierophinney
Copy link
Member

With one point I'm not satisfied. A service call must start with Zend\Navigation\ (e.g. $sl->get('Zend\Navigation\Special')). Maybe we could support navigation.special too?

I think Zend\Navigation\ as a prefix is a reasonable default, and it's easier to support a single option than a configurable option. Developers can extend the factory if they want to change the prefix.

@weierophinney
Copy link
Member

I use the ConstructedNavigationFactory to create the Navigation Container. Is this ok or should I use the AbstractNavigationFactory (@weierophinney)?

ConstructedNavigationFactory looks like the appropriate one to use, as it allows you to get the specific configuration section you need. Internally, it's extending AbstractNavigationFactory anyways; the difference is how it gets the page configuration, and that functionality is why it exists.

@weierophinney
Copy link
Member

The problem here is that we have to implement the getName() and override the getPages() method. For the future (ZF 3.0) we could remove getName() and make getPages() abstract in AbstractNavigationFactory. This makes DefaultNavigationFactory and ConstructedNavigationFactory needless. But it's a BC break.

We can worry about that in a month or two. :)

@weierophinney weierophinney added this to the 2.4.0 milestone Feb 24, 2015
@weierophinney weierophinney merged commit af7ea80 into zendframework:develop Feb 24, 2015
weierophinney added a commit that referenced this pull request Feb 24, 2015
…factory

[Navigation] Introducing NavigationAbstractServiceFactory
weierophinney added a commit that referenced this pull request Feb 24, 2015
@froschdesign
Copy link
Member

@sandrokeil
Please add also a short example to docs. Thanks!

@sandrokeil
Copy link
Contributor Author

I will do it in the next few days.

@froschdesign
Copy link
Member

@sandrokeil
👍

weierophinney added a commit to zendframework/zend-navigation that referenced this pull request May 15, 2015
…avigation-abstract-service-factory

[Navigation] Introducing NavigationAbstractServiceFactory
weierophinney added a commit to zendframework/zend-navigation that referenced this pull request May 15, 2015
@razonklnbd
Copy link

Developer can't inherit due to its final behavior. Can anyone please explain why this factory using "final" keyword at class declaration?
Also constant "SERVICE_PREFIX" is not using anywhere, I guess it may use somewhere in Zend project!

@sandrokeil
Copy link
Contributor Author

@razonklnbd Why do you want to inherit from this class, it's not necessary. Please consider to create your own factory for your special behaviour.

A good explanation when to use final, can be found here. That's the case.

@razonklnbd
Copy link

@sandrokeil Thanks for your explanation.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants