Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

added "ControllerManager" Manager, and make "ControllerLoader" as alias of it #4962

Closed
wants to merge 1 commit into from

6 participants

@samsonasik

for consistency, "ControllerLoader" should be named "ControllerManager". It not BC break because of I made new Factory named ControllerManagerFactory and point ControllerLoader as alias of ControllerManager. It can be a preparation for ZF3. When ZF3 released, the "ControllerLoader" manager can be completely removed.

@Ocramius
Collaborator

@samsonasik :+1: , but you should probably also replace usages...

@samsonasik

@Ocramius I replaced usages of 'ControlleLoader' to 'ControllerManager' at library and tests folder, now it is safe to remove 'ControllerLoader' from ModuleManagerFactory because of already aliased. Let me know if something missing ;)

library/Zend/Mvc/Service/ControllerManagerFactory.php
@@ -14,10 +14,10 @@
use Zend\ServiceManager\FactoryInterface;
@weierophinney Owner

It might make sense to keep a ControllerLoaderFactory in place that simply extends the ControllerManagerFactory. We would not register the factory -- but that way, if somebody was extending it, it would continue to work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney

I'm still worried about BC implications of this.

If a developer has created a factory for ControllerLoader, the assumption is fetching the ControllerManager service will fetch that instance -- but with this change, it would not. I'm thinking we may need to bench this one until 3.0.

@samsonasik Can you post an email to the zf-contributors mailing list, pointing them to this PR and:

  • summarize what you are doing
  • explain the BC concerns
  • ask for feedback

Thanks!

@bakura10

For ZF3 I'm in favour of renaming all this mess to "ControllerPluginManager", "FilterPluginManager"... and so on =). But even, this is somewhere we're not consistent. I love using FQCN for keys, and we use it sometimes even inside the framework ('Zend\Authentication\AuthenticationService' for instance).

We need to have a consensus on that =).

...Zend/Mvc/Controller/Plugin/Service/ForwardFactory.php
@@ -32,14 +32,14 @@ public function createService(ServiceLocatorInterface $plugins)
));
}
- if (!$services->has('ControllerLoader')) {
+ if (!$services->has('ControllerManager')) {
@Ocramius Collaborator

Question: do we still need this check?

I think no need, but changing it can be "unrelated" commit for this PR ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Mvc/Service/ControllerLoaderFactory.php
@@ -9,41 +9,9 @@
namespace Zend\Mvc\Service;
-use Zend\Mvc\Controller\ControllerManager;
-use Zend\Mvc\Service\DiStrictAbstractServiceFactory;
-use Zend\ServiceManager\FactoryInterface;
-use Zend\ServiceManager\ServiceLocatorInterface;
-
-class ControllerLoaderFactory implements FactoryInterface
+/**
+ * @deprecated
@Ocramius Collaborator

Add note about future removal (2.4? 3.x?)

@Ocramius I have added note : "This will go away in 3.0" on the @deprecated ^^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...ZendTest/Mvc/Service/ControllerManagerFactoryTest.php
((10 lines not shown))
+namespace ZendTest\Mvc\Service;
+
+use ArrayObject;
+use PHPUnit_Framework_TestCase as TestCase;
+use Zend\Mvc\Service\ControllerManagerFactory;
+use Zend\Mvc\Service\ControllerPluginManagerFactory;
+use Zend\Mvc\Service\DiFactory;
+use Zend\Mvc\Service\DiStrictAbstractServiceFactoryFactory;
+use Zend\Mvc\Service\DiAbstractServiceFactoryFactory;
+use Zend\Mvc\Service\DiServiceInitializerFactory;
+use Zend\Mvc\Service\EventManagerFactory;
+use Zend\ServiceManager\Config;
+use Zend\ServiceManager\ServiceManager;
+use Zend\Mvc\Exception;
+
+class ControllerManagerFactoryTest extends TestCase
@Ocramius Collaborator

I think @covers annotations are missing here

I just make a copy of ControllerLoaderFactoryTest test :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@samsonasik

@bakura10 this PR can be a preparation for ZF3, I make an alias of ControllerManager pointed to ControlletLoader to make it's still compatible with current version 2. When ZF3 released, the "ControllerLoader" manager that deprecated can be completely removed.

@devosc

I'm not sure how only changing the name will benefit ZF3, when ideally its functionality should also change in order to make it an independent factory, below is along the lines of what I would like/expect to see in ZF3:

public function createService(ServiceLocatorInterface $serviceLocator)
{
    $config = $serviceLocator->get('Config');

    $controllerConfig = new ControllerManagerConfig($config['controllers']);

    $controllerManager = new ControllerManager();
    $controllerManager->setServiceLocator($serviceLocator);
    $controllerManager->addPeeringServiceManager($serviceLocator);

    $controllerConfig->configureServiceManager($controllerManager);

    if (isset($config['di']) && isset($config['di']['allowed_controllers']) && $serviceLocator->has('Di')) {
        $controllerManager->addAbstractFactory($serviceLocator->get('DiStrictAbstractServiceFactory'));
    }

    return $controllerManager;
}
@bakura10

@devosc : +100. I'm advocating using the SM config to create all plugin managers (that's what I'm doing in my ZF3 hydrator refactoring too). This will allow us to remove the infamous ModuleListener class. However we need to have a discussion about this with the team as we loose a feature (the ability to specify config in a Module.php class using the getControllerConfig method - I think it's not a big deal as people can still use the global getServiceConfig).

Changing names is just a coherency. ControllerLoader was confusing because most other plugin manager ends up with manager. At the same time we also have some plugin managers that ends with PluginManager (which is the suffix I prefer). It's all those small things that make the framework easier and more coherent to use, even if this is not a feature change.

@samsonasik

@devosc This PR is for preparation for ZF3 for more consistency, just to start consistency with naming. I think the changing code can be applied when starting ZF3

@EvanDotPro
Collaborator

The thread started via Nabble didn't come to my inbox, so I'll leave my comment here.

Personally, I'd rather see a ControllerManager alias added now rather than changing the class names and everything and aliasing the old to the new. Aliasing the future names to the existing names gives less risk of a BC break in 2.x. I agree with 3.x we should move toward more consistent naming, etc.

@weierophinney

@EvanDotPro can you clarify the specific changes you feel should be made for this PR to be ready to merge?

@weierophinney

@samsonasik what @EvanDotPro is indicating is:

  • Keep the original factories/services
  • Alias the future names (e.g., ControllerManager) to the present names (i.e. ControllerLoader) (I think this may be the current situation).

The rationale is it ensures BC is kept in 2.x. In 3.0, we would change the situation, and make the new name the actual service, and potentially have a "compatibility layer" that would alias the old service names to the new.

@samsonasik

@weierophinney done ;), I keep the original factories, and added alias ( ControlerManager ) for it. Please let me know if I missed something. Thank you.

@weierophinney weierophinney referenced this pull request from a commit
@weierophinney weierophinney [#4962] Updated code to use ControllerManager
- Original commit adds the alias for ControllerManager; this commit updates code
  to use that alias. Some tests needed to be updated as well.
7ab9530
@weierophinney weierophinney referenced this pull request from a commit
@weierophinney weierophinney [#4962] Update README
- while not technically a BC break, should be called out in release notes
9270ae9
@weierophinney

I updated code to reference the "ControllerManager" service, including tests, and noted the change in the README. Merged to develop for release with 2.3.0.

@samsonasik samsonasik referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@weierophinney weierophinney referenced this pull request from a commit in zendframework/zend-test
@weierophinney weierophinney [zendframework/zf2#4962] Updated code to use ControllerManager
- Original commit adds the alias for ControllerManager; this commit updates code
  to use that alias. Some tests needed to be updated as well.
41da475
@weierophinney weierophinney referenced this pull request from a commit in zendframework/zend-test
@weierophinney weierophinney Merge branch 'feature/4962' into develop c327a98
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 1 addition and 0 deletions.
  1. +1 −0  library/Zend/Mvc/Service/ServiceListenerFactory.php
View
1  library/Zend/Mvc/Service/ServiceListenerFactory.php
@@ -85,6 +85,7 @@ class ServiceListenerFactory implements FactoryInterface
'Zend\View\Resolver\TemplatePathStack' => 'ViewTemplatePathStack',
'Zend\View\Resolver\AggregateResolver' => 'ViewResolver',
'Zend\View\Resolver\ResolverInterface' => 'ViewResolver',
+ 'ControllerManager' => 'ControllerLoader'
),
'abstract_factories' => array(
'Zend\Form\FormAbstractServiceFactory',
Something went wrong with that request. Please try again.