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

[ZF3] [bc-break] remove ControllerLoader, use ControllerManager instead #6

Closed
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/DispatchListener.php
Original file line number Diff line number Diff line change
Expand Up @@ -65,15 +65,15 @@ public function onDispatch(MvcEvent $e)
$controllerName = $routeMatch->getParam('controller', 'not-found');
$application = $e->getApplication();
$events = $application->getEventManager();
$controllerLoader = $application->getServiceManager()->get('ControllerManager');
$controllerManager = $application->getServiceManager()->get('ControllerManager');

if (!$controllerLoader->has($controllerName)) {
if (!$controllerManager->has($controllerName)) {
$return = $this->marshalControllerNotFoundEvent($application::ERROR_CONTROLLER_NOT_FOUND, $controllerName, $e, $application);
return $this->complete($return, $e);
}

try {
$controller = $controllerLoader->get($controllerName);
$controller = $controllerManager->get($controllerName);
} catch (InvalidControllerException $exception) {
$return = $this->marshalControllerNotFoundEvent($application::ERROR_CONTROLLER_INVALID, $controllerName, $e, $application, $exception);
return $this->complete($return, $e);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
use Zend\ServiceManager\FactoryInterface;
use Zend\ServiceManager\ServiceLocatorInterface;

class ControllerLoaderFactory implements FactoryInterface
class ControllerManagerFactory implements FactoryInterface
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead create a new class and inherit the old one from the new one. By this way BC is preserved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as this is for ZF3, I thik it is ok to remove the old one /cc @weierophinney

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do it and this could be merged in 2.6

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, and ControllerLoaderFactory marked as @deprecated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as zendframework/zend-authentication#2 (comment) , ControllerLoader actually removed for 3.0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add description with the purpose of the class

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any suggestion description ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following the SRP of SOLID describe what is this class responsible

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

{
/**
* Create the controller loader service
Expand All @@ -33,16 +33,16 @@ class ControllerLoaderFactory implements FactoryInterface
*/
public function createService(ServiceLocatorInterface $serviceLocator)
{
$controllerLoader = new ControllerManager();
$controllerLoader->setServiceLocator($serviceLocator);
$controllerLoader->addPeeringServiceManager($serviceLocator);
$controllerManager = new ControllerManager();
$controllerManager->setServiceLocator($serviceLocator);
$controllerManager->addPeeringServiceManager($serviceLocator);

$config = $serviceLocator->get('Config');

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

return $controllerLoader;
return $controllerManager;
}
}
2 changes: 1 addition & 1 deletion src/Service/ModuleManagerFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ public function createService(ServiceLocatorInterface $serviceLocator)
'getServiceConfig'
);
$serviceListener->addServiceManager(
'ControllerLoader',
'ControllerManager',
'controllers',
'Zend\ModuleManager\Feature\ControllerProviderInterface',
'getControllerConfig'
Expand Down
4 changes: 1 addition & 3 deletions src/Service/ServiceListenerFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ class ServiceListenerFactory implements FactoryInterface
'factories' => [
'Application' => 'Zend\Mvc\Service\ApplicationFactory',
'Config' => 'Zend\Mvc\Service\ConfigFactory',
'ControllerLoader' => 'Zend\Mvc\Service\ControllerLoaderFactory',
'ControllerManager' => 'Zend\Mvc\Service\ControllerManagerFactory',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Old key must be preserved for bc

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment with @deprecated for to remove in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now it for 3.0, can be actually removed.

'ControllerPluginManager' => 'Zend\Mvc\Service\ControllerPluginManagerFactory',
'ConsoleAdapter' => 'Zend\Mvc\Service\ConsoleAdapterFactory',
'ConsoleRouter' => 'Zend\Mvc\Service\RouterFactory',
Expand Down Expand Up @@ -94,8 +94,6 @@ class ServiceListenerFactory implements FactoryInterface
'Zend\View\Resolver\TemplatePathStack' => 'ViewTemplatePathStack',
'Zend\View\Resolver\AggregateResolver' => 'ViewResolver',
'Zend\View\Resolver\ResolverInterface' => 'ViewResolver',
'ControllerManager' => 'ControllerLoader',
],
'abstract_factories' => [
'Zend\Form\FormAbstractServiceFactory',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, at ZF3, the old one ( ControllerLoader ) will no longer used. zendframework/zendframework#4962

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to add back closing "]", fixed now

],
Expand Down
24 changes: 12 additions & 12 deletions test/ApplicationTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -233,8 +233,8 @@ public function setupPathController($addService = true)
]);
$router->addRoute('path', $route);
if ($addService) {
$controllerLoader = $this->serviceManager->get('ControllerLoader');
$controllerLoader->setFactory('path', function () {
$controllerManager = $this->serviceManager->get('ControllerManager');
$controllerManager->setFactory('path', function () {
return new TestAsset\PathController;
});
}
Expand All @@ -256,8 +256,8 @@ public function setupActionController()
]);
$router->addRoute('sample', $route);

$controllerLoader = $this->serviceManager->get('ControllerLoader');
$controllerLoader->setFactory('sample', function () {
$controllerManager = $this->serviceManager->get('ControllerManager');
$controllerManager->setFactory('sample', function () {
return new Controller\TestAsset\SampleController;
});
$this->application->bootstrap();
Expand All @@ -279,8 +279,8 @@ public function setupBadController($addService = true)
$router->addRoute('bad', $route);

if ($addService) {
$controllerLoader = $this->serviceManager->get('ControllerLoader');
$controllerLoader->setFactory('bad', function () {
$controllerManager = $this->serviceManager->get('ControllerManager');
$controllerManager->setFactory('bad', function () {
return new Controller\TestAsset\BadController;
});
}
Expand Down Expand Up @@ -383,7 +383,7 @@ public function testExceptionsRaisedInDispatchableShouldRaiseDispatchErrorEvent(
public function testInabilityToRetrieveControllerShouldTriggerExceptionError()
{
$this->setupBadController(false);
$controllerLoader = $this->serviceManager->get('ControllerLoader');
$controllerManager = $this->serviceManager->get('ControllerManager');
$response = $this->application->getResponse();
$events = $this->application->getEventManager();
$events->attach(MvcEvent::EVENT_DISPATCH_ERROR, function ($e) use ($response) {
Expand Down Expand Up @@ -423,10 +423,10 @@ public function testInabilityToRetrieveControllerShouldTriggerDispatchError()
*/
public function testInvalidControllerTypeShouldTriggerDispatchError()
{
$this->serviceManager->get('ControllerLoader');
$this->serviceManager->get('ControllerManager');
$this->setupBadController(false);
$controllerLoader = $this->serviceManager->get('ControllerLoader');
$controllerLoader->setFactory('bad', function () {
$controllerManager = $this->serviceManager->get('ControllerManager');
$controllerManager->setFactory('bad', function () {
return new stdClass;
});
$response = $this->application->getResponse();
Expand Down Expand Up @@ -473,7 +473,7 @@ public function testRoutingFailureShouldTriggerDispatchError()
public function testLocatorExceptionShouldTriggerDispatchError()
{
$this->setupPathController(false);
$controllerLoader = $this->serviceManager->get('ControllerLoader');
$controllerManager = $this->serviceManager->get('ControllerManager');
$response = new Response();
$this->application->getEventManager()->attach(MvcEvent::EVENT_DISPATCH_ERROR, function ($e) use ($response) {
return $response;
Expand Down Expand Up @@ -570,7 +570,7 @@ public function testApplicationShouldBeEventTargetAtFinishEvent()
public function testOnDispatchErrorEventPassedToTriggersShouldBeTheOriginalOne()
{
$this->setupPathController(false);
$controllerLoader = $this->serviceManager->get('ControllerLoader');
$controllerManager = $this->serviceManager->get('ControllerManager');
$model = $this->getMock('Zend\View\Model\ViewModel');
$this->application->getEventManager()->attach(MvcEvent::EVENT_DISPATCH_ERROR, function ($e) use ($model) {
$e->setResult($model);
Expand Down
5 changes: 2 additions & 3 deletions test/Controller/Plugin/ForwardTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,10 @@ public function setUp()
return $controller;
});
$controllers->setServiceLocator($services);
$controllerLoader = function () use ($controllers) {
$controllerManager = function () use ($controllers) {
return $controllers;
};
$services->add('ControllerLoader', $controllerLoader);
$services->add('ControllerManager', $controllerLoader);
$services->add('ControllerManager', $controllerManager);
$services->add('ControllerPluginManager', function () use ($plugins) {
return $plugins;
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
use Zend\ServiceManager\AbstractFactoryInterface;
use Zend\ServiceManager\ServiceLocatorInterface;

class ControllerLoaderAbstractFactory implements AbstractFactoryInterface
class ControllerManagerAbstractFactory implements AbstractFactoryInterface
{
protected $classmap = array(
'path' => 'ZendTest\Mvc\TestAsset\PathController',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
use Zend\ServiceManager\AbstractFactoryInterface;
use Zend\ServiceManager\ServiceLocatorInterface;

class UnlocatableControllerLoaderAbstractFactory implements AbstractFactoryInterface
class UnlocatableControllerManagerAbstractFactory implements AbstractFactoryInterface
{
public function canCreateServiceWithName(ServiceLocatorInterface $sl, $cName, $rName)
{
Expand Down
12 changes: 6 additions & 6 deletions test/DispatchListenerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -79,12 +79,12 @@ public function setupPathController()
$this->application->bootstrap();
}

public function testControllerLoaderComposedOfAbstractFactory()
public function testControllerManagerComposedOfAbstractFactory()
{
$this->setupPathController();

$controllerLoader = $this->serviceManager->get('ControllerLoader');
$controllerLoader->addAbstractFactory('ZendTest\Mvc\Controller\TestAsset\ControllerLoaderAbstractFactory');
$controllerManager = $this->serviceManager->get('ControllerManager');
$controllerManager->addAbstractFactory('ZendTest\Mvc\Controller\TestAsset\ControllerManagerAbstractFactory');

$log = [];
$this->application->getEventManager()->attach(MvcEvent::EVENT_DISPATCH_ERROR, function ($e) use (&$log) {
Expand All @@ -102,12 +102,12 @@ public function testControllerLoaderComposedOfAbstractFactory()
$this->assertSame(200, $return->getStatusCode());
}

public function testUnlocatableControllerLoaderComposedOfAbstractFactory()
public function testUnlocatableControllerManagerComposedOfAbstractFactory()
{
$this->setupPathController();

$controllerLoader = $this->serviceManager->get('ControllerLoader');
$controllerLoader->addAbstractFactory('ZendTest\Mvc\Controller\TestAsset\UnlocatableControllerLoaderAbstractFactory');
$controllerManager = $this->serviceManager->get('ControllerManager');
$controllerManager->addAbstractFactory('ZendTest\Mvc\Controller\TestAsset\UnlocatableControllerManagerAbstractFactory');

$log = [];
$this->application->getEventManager()->attach(MvcEvent::EVENT_DISPATCH_ERROR, function ($e) use (&$log) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

use ArrayObject;
use PHPUnit_Framework_TestCase as TestCase;
use Zend\Mvc\Service\ControllerLoaderFactory;
use Zend\Mvc\Service\ControllerManagerFactory;
use Zend\Mvc\Service\ControllerPluginManagerFactory;
use Zend\Mvc\Service\DiFactory;
use Zend\Mvc\Service\DiStrictAbstractServiceFactoryFactory;
Expand All @@ -22,7 +22,7 @@
use Zend\ServiceManager\ServiceManager;
use Zend\Mvc\Exception;

class ControllerLoaderFactoryTest extends TestCase
class ControllerManagerFactoryTest extends TestCase
{
/**
* @var ServiceManager
Expand All @@ -32,15 +32,15 @@ class ControllerLoaderFactoryTest extends TestCase
/**
* @var \Zend\Mvc\Controller\ControllerManager
*/
protected $loader;
protected $manager;

public function setUp()
{
$loaderFactory = new ControllerLoaderFactory();
$config = new ArrayObject(['di' => []]);
$managerFactory = new ControllerManagerFactory();
$config = new ArrayObject(array('di' => []);
$this->services = new ServiceManager();
$this->services->setService('Zend\ServiceManager\ServiceLocatorInterface', $this->services);
$this->services->setFactory('ControllerLoader', $loaderFactory);
$this->services->setFactory('ControllerManager', $managerFactory);
$this->services->setService('Config', $config);
$this->services->setFactory('ControllerPluginManager', new ControllerPluginManagerFactory());
$this->services->setFactory('Di', new DiFactory());
Expand All @@ -53,13 +53,13 @@ public function setUp()

public function testCannotLoadInvalidDispatchable()
{
$this->loader = $this->services->get('ControllerLoader');
$this->manager = $this->services->get('ControllerManager');

// Ensure the class exists and can be autoloaded
$this->assertTrue(class_exists('ZendTest\Mvc\Service\TestAsset\InvalidDispatchableClass'));

try {
$this->loader->get('ZendTest\Mvc\Service\TestAsset\InvalidDispatchableClass');
$this->manager->get('ZendTest\Mvc\Service\TestAsset\InvalidDispatchableClass');
$this->fail('Retrieving the invalid dispatchable should fail');
} catch (\Exception $e) {
do {
Expand All @@ -70,25 +70,31 @@ public function testCannotLoadInvalidDispatchable()

public function testCannotLoadControllerFromPeer()
{
$this->loader = $this->services->get('ControllerLoader');
$this->manager = $this->services->get('ControllerManager');
$this->services->setService('foo', $this);

$this->setExpectedException('Zend\ServiceManager\Exception\ExceptionInterface');
$this->loader->get('foo');
$this->manager->get('foo');
}

public function testControllerLoadedCanBeInjectedWithValuesFromPeer()
{
<<<<<<< HEAD:test/Service/ControllerLoaderFactoryTest.php
$this->loader = $this->services->get('ControllerLoader');
$config = [
'invokables' => [
=======
$this->manager = $this->services->get('ControllerManager');
$config = array(
'invokables' => array(
>>>>>>> remove ControllerLoader, use ControllerManager instead:test/Service/ControllerManagerFactoryTest.php
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bad rebase

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.

'ZendTest\Dispatchable' => 'ZendTest\Mvc\Service\TestAsset\Dispatchable',
],
];
$config = new Config($config);
$config->configureServiceManager($this->loader);
$config->configureServiceManager($this->manager);

$controller = $this->loader->get('ZendTest\Dispatchable');
$controller = $this->manager->get('ZendTest\Dispatchable');
$this->assertInstanceOf('ZendTest\Mvc\Service\TestAsset\Dispatchable', $controller);
$this->assertSame($this->services, $controller->getServiceLocator());
$this->assertSame($this->services->get('EventManager'), $controller->getEventManager());
Expand All @@ -111,12 +117,12 @@ public function testWillInstantiateControllersFromDiAbstractFactoryWhenWhitelist
]);
$this->services->setAllowOverride(true);
$this->services->setService('Config', $config);
$this->loader = $this->services->get('ControllerLoader');
$this->manager = $this->services->get('ControllerManager');

$this->assertTrue($this->loader->has('my-controller'));
$this->assertTrue($this->manager->has('my-controller'));
// invalid controller exception (because we're getting an \stdClass after all)
$this->setExpectedException('Zend\Mvc\Exception\InvalidControllerException');
$this->loader->get('my-controller');
$this->manager->get('my-controller');
}

public function testWillNotInstantiateControllersFromDiAbstractFactoryWhenNotWhitelisted()
Expand All @@ -135,9 +141,9 @@ public function testWillNotInstantiateControllersFromDiAbstractFactoryWhenNotWhi
]);
$this->services->setAllowOverride(true);
$this->services->setService('Config', $config);
$this->loader = $this->services->get('ControllerLoader');
$this->manager = $this->services->get('ControllerManager');
$this->setExpectedException('Zend\ServiceManager\Exception\ServiceNotFoundException');
$this->loader->get('evil-controller');
$this->manager->get('evil-controller');
}

public function testWillFetchDiDependenciesFromControllerLoaderServiceManager()
Expand All @@ -160,12 +166,12 @@ public function testWillFetchDiDependenciesFromControllerLoaderServiceManager()
]);
$this->services->setAllowOverride(true);
$this->services->setService('Config', $config);
$this->loader = $this->services->get('ControllerLoader');
$this->manager = $this->services->get('ControllerManager');

$testService = new \stdClass();
$this->services->setService('stdClass', $testService);
// invalid controller exception (because we're not getting a \Zend\Stdlib\DispatchableInterface after all)
$controller = $this->loader->get($controllerName);
$controller = $this->manager->get($controllerName);
$this->assertSame($testService, $controller->injectedValue);
}

Expand Down