Allow to pull route manager from sl #3519

Merged
merged 4 commits into from Jan 25, 2013

Projects

None yet

8 participants

@bakura10
Contributor

This PR addresses the following issue : #3516 (comment)

It was not easy because the Router uses static factories too much and doing it the clean way (by using invokables set in the plugin manager and properly pull everything from the service locator) would without any doubt bring some BC...

So I just did it the easy way by not calling the factory of default Http and Console routers and copy/pasting the code from the static function factory to the factory class itself. Not really that beautiful but well... I think this is a BC we should do as it makes the code quite hard to extend.

Anyway, this PR adds a new ModuleManager feature that allow to add routes either from a getRouteConfig in your Module.php, or using the routes key in service manager config:

return array(
    'routes' => array(
        'invokables' => array(
            'Rest' => 'ZfrRest\Mvc\Router\Route\Rest'
        )
    )
);

And then use it in the router:

return array(
    'router' => array(
        'routes' => array(
            'dummy' => array(
                   'type' => 'Rest'
            )
        )
    )
);

EDIT : @weierophinney , regarding this file: https://github.com/zendframework/zf2/blob/develop/library/Zend/Mvc/Service/ModuleManagerFactory.php#L82

It's just growing and growing. Don't you think we could allow each plugin manager to register itself to the ServiceListener. It makes things easier to discover (jsut open the plugin maanger and you immediately know what is the config key and so...). I never remember this file...

@Ocramius Ocramius commented on the diff Jan 22, 2013
tests/ZendTest/Mvc/Router/RoutePluginManagerTest.php
@@ -27,4 +27,13 @@ public function testLoadNonExistentRoute()
$this->setExpectedException('Zend\ServiceManager\Exception\ServiceNotFoundException');
$routes->get('foo');
}
+
+ public function testCanLoadAnyRoute()
@Ocramius
Ocramius Jan 22, 2013 Member

Add @covers \Zend\Mvc\Router\RoutePluginManager

@bakura10
bakura10 Jan 22, 2013 Contributor

Hu ? What's that ?

@Ocramius
Ocramius Jan 24, 2013 Member

Simply:

/**
 * @covers \Zend\Mvc\Router\RoutePluginManager
 */

:)

@bakura10
bakura10 Jan 24, 2013 Contributor

But it's already in the file RoutePluginManagerTest 🍭

@Ocramius
Ocramius Jan 24, 2013 Member

Aha! Didn't notice :)

@Ocramius
Member

so any type here will look in the plugin manager? awesome!

@Ocramius
Member

👍

@bakura10
Contributor

Yes. I hope @weierophinney will allow me some BC for ZF 2.2 so that we can make this a lot cleaner though.

@prolic prolic commented on an outdated diff Jan 22, 2013
library/Zend/Mvc/Router/SimpleRouteStack.php
{
- $this->routes = new PriorityList();
- $this->routePluginManager = new RoutePluginManager();
+ $this->routes = new PriorityList();
+
+ if ($routePluginManager === null) {
@prolic
prolic Jan 22, 2013 Contributor

Please use joda conditions.

@turrsis
Contributor
turrsis commented Jan 23, 2013

To my mind, SimpleRouteStack::factory should be as follows

if (isset($options['route_plugins'])) {
        if (is_array($options['route_plugins'])) {
            $config = new PluginManagerConfig($options['route_plugins']);
            $config->configureServiceManager($instance->getRoutePluginManager());
        } elseif($options['route_plugins'] instanceof RoutePluginManager) {
            $instance->setRoutePluginManager($options['route_plugins']);
        } else {
            throw new InvalidArgumentException(__METHOD__ . ' expects an route_plugins as Array or    RoutePluginManager');
        }
    }
@devosc
Contributor
devosc commented Jan 23, 2013

@turrsis, would that mean in setInvokableClass, that allowOverride would need to be false? The TreeStack's init method which is called within the constructor automatically defines its own invokables?

@turrsis
Contributor
turrsis commented Jan 23, 2013

it depends on the desired result.
'route_plugins' section is analog of the 'service_manager' section

@bakura10
Contributor

Mmmhhhh turrsis I'm not really ok for this feature because, basically, you're duplicating the new feature that this PR provides (bring the route plugin manager as any other plugin manager). I think that the router was done before all this plugin manager stuff and does not really use it 100%.

If I bring your PR this means people will have two ways to do the same thing, I think we should encourage only the best way to do it.

@Ocramius
Member

Agreed.
This has to be cleaned up, and possibly a BC should be done for 2.2. The
plugin manager idea that @bakura10 implemented is actually in line with all
the helpers/controllers/services/whatever logic we use in other components.

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

On 23 January 2013 11:15, Michaël Gallego notifications@github.com wrote:

Mmmhhhh turrsis I'm not really ok for this feature because, basically,
you're duplicating the new feature that this PR provides (bring the route
plugin manager as any other plugin manager). I think that the router was
done before all this plugin manager stuff and does not really use it 100%.

If I bring your PR this means people will have two ways to do the same
thing, I think we should encourage only the best way to do it.


Reply to this email directly or view it on GitHubhttps://github.com/zendframework/zf2/pull/3519#issuecomment-12589115.

@turrsis
Contributor
turrsis commented Jan 23, 2013

Yes @bakura10 idea is a good solution.
But $options['route_plugins'] should be array OR RoutePluginManager. Because using instances in config files is a bad idea

@bakura10
Contributor

The thing is that we don't want people to use it this way. So we won't add features here too :). for people wanting to add routes through an array config, they should use the recommended method. You don't add features to something you want to deprecate ;-).

@radnan radnan and 3 others commented on an outdated diff Jan 24, 2013
library/Zend/Mvc/Service/ModuleManagerFactory.php
@@ -85,6 +85,12 @@ public function createService(ServiceLocatorInterface $serviceLocator)
'Zend\ModuleManager\Feature\FormElementProviderInterface',
'getFormElementConfig'
);
+ $serviceListener->addServiceManager(
+ 'RoutePluginManager',
+ 'routes',
@radnan
radnan Jan 24, 2013 Contributor

I'd vote for calling this 'route_plugins'

@bakura10
bakura10 Jan 24, 2013 Contributor

It's just to make it more consistent. Other keys are: validators, filters, controllers, form_elements... so it's more logical to call it simply "routes".

@Ocramius
Ocramius Jan 24, 2013 Member

We already have a config key, and it is router ( see https://github.com/zendframework/ZendSkeletonApplication/blob/master/module/Application/config/module.config.php#L11 )

It would be very confusing IMO to have config like:

return array(
    'router' => array(
        'routes' => array(...),
    ),
    'routes' => array(...),
);

Should probably continue to be router

@radnan
radnan Jan 24, 2013 Contributor

@bakura10 There's also 'controller_plugins' :)

@Ocramius I want to agree (makes things easier for end users to have one key) but it breaks naming convention for other service-managers (ControllerManager -> 'controllers', ControllerPluginManager -> 'controller_plugins' in ModuleManagerFactory) and services (Router -> 'router', ViewManager -> 'view_manager').

@bakura10
bakura10 Jan 24, 2013 Contributor

I don't see why you all want to have strange syntax :'(.

There is some incoherencies on that radian. All those things extends from AbstractPluginManager. In fact RoutePluginManager could be called RouteManager...

@weierophinney
weierophinney Jan 25, 2013 Member

Since this is configuring the router itself -- not the routes -- and in order to reduce confusion, I think "route_manager" makes sense here, and would provide reasonable symmetry with existing keys like "service_manager" and "view_manager".

@bakura10
bakura10 Jan 25, 2013 Contributor

I prefer route_manager. There are some inconsistencies on that across the framework :'(. I'm going to change that.

@weierophinney weierophinney added a commit that referenced this pull request Jan 25, 2013
@weierophinney weierophinney Merge branch 'feature/3519' into develop
Close #3519
8f6c68b
@weierophinney weierophinney merged commit 9f032e8 into zendframework:develop Jan 25, 2013
@richardjennings
Contributor

@bakura10 Should RouteProviderInterface be called RoutePluginProviderInterface such that RouteProviderInterface can be used to provide route definitions as per ControllerPluginProviderInterface and ControllerProviderInterface ?

Member

@richardjennings A bit late to ask that question, as this functionality is already released. :)

@weierophinney weierophinney added a commit to zendframework/zend-modulemanager that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge branch 'feature/3519' into develop b4afa2a
@weierophinney weierophinney added a commit to zendframework/zend-view that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge branch 'feature/3519' into develop 169e75b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment