From 0a531e25bc8ec8e10b4730dfb609b6ff34f8b7b6 Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Wed, 2 Mar 2016 08:50:42 -0600 Subject: [PATCH 1/2] Better ServiceLocatorAware deprecation We were: - injecting any service that *was not* marked as `ServiceLocatorAware` but implemented `setServiceLocator()`, and - injecting any service that *was* marked `ServiceLocatorAware`, and - emitting a deprecation notice in both cases. The rationale was to allow backwards compatibility with existing implementations, while still messaging to users that they need to update their code not to rely on that. This created a few problems, though: - *Every* controller inheriting from `AbstractController` now emits deprecation notices on instantiation. - Developers who had implemented a `setServiceLocator()` method without implementing `ServiceLocatorAwareInterface` were now getting auto-injected with the service locator instance when they did not expect to. This patch attempts to address those issues as follows: - `ServiceManagerConfig` updates its service locator initializer to: - Inject non-plugin manager `ServiceLocatorAware` instances; in this situation, a deprecation notice *is* emitted. - For `ServiceLocatorAware` plugin managers, it checks to see if a service locator is already composed. If not, it will inject it, but then emit a deprecation notice indicating that the service locator should be injected as a constructor parameter. - `ControllerManager` updates its service locator initializer to: - For non-`ServiceLocatorAware` `AbstractController` implementations, it injects the service locator with no notices. - For all explicitly `ServiceLocatorAware` controllers, it injects the service locator *and* emits a deprecation notice. - It no longer injects non-`ServiceLocatorAware` controllers that implement `setServiceLocator()` unless they extend `AbstractController`. This is fully backwards compatible. - `AbstractController::getServiceLocator()` now emits a deprecation notice prior to returning the service locator instance. This is more appropriate, as we want to encourage users who rely on the service locator composition to upgrade. Interestingly, this allows us to revert a number of changes that were made to the tests to mask deprecation notices, which helps solidify the argument that the original changes were indeed a backwards compatibility break, and that the new changes will address that situation. --- src/Controller/AbstractController.php | 9 +++++++ src/Controller/ControllerManager.php | 14 +++++----- src/Service/ServiceManagerConfig.php | 20 ++++++++++---- .../AllowsReturningEarlyFromRoutingTest.php | 7 ----- .../ControllerIsDispatchedTest.php | 7 ----- ...hableShouldRaiseDispatchErrorEventTest.php | 7 ----- test/Controller/AbstractControllerTest.php | 12 +++++++++ test/Controller/ControllerManagerTest.php | 15 ----------- test/Controller/IntegrationTest.php | 4 --- test/Controller/Plugin/ForwardTest.php | 4 --- test/Service/ControllerManagerFactoryTest.php | 4 --- test/Service/ServiceManagerConfigTest.php | 16 ----------- .../DuckTypedServiceLocatorAware.php | 27 ------------------- ...DuckTypedServiceLocatorAwareController.php | 22 --------------- 14 files changed, 44 insertions(+), 124 deletions(-) delete mode 100644 test/Service/TestAsset/DuckTypedServiceLocatorAware.php delete mode 100644 test/Service/TestAsset/DuckTypedServiceLocatorAwareController.php diff --git a/src/Controller/AbstractController.php b/src/Controller/AbstractController.php index c546a4484..a3511151a 100644 --- a/src/Controller/AbstractController.php +++ b/src/Controller/AbstractController.php @@ -248,6 +248,15 @@ public function setServiceLocator(ServiceLocatorInterface $serviceLocator) */ public function getServiceLocator() { + trigger_error(sprintf( + 'You are retrieving the service locator from within the class %s. Please be aware that ' + . 'ServiceLocatorAwareInterface is deprecated and will be removed in version 3.0, along ' + . 'with the ServiceLocatorAwareInitializer. You will need to update your class to accept ' + . 'all dependencies at creation, either via constructor arguments or setters, and use ' + . 'a factory to perform the injections.', + get_class($this) + ), E_USER_DEPRECATED); + return $this->serviceLocator; } diff --git a/src/Controller/ControllerManager.php b/src/Controller/ControllerManager.php index 00eb1b2c0..8f4649466 100644 --- a/src/Controller/ControllerManager.php +++ b/src/Controller/ControllerManager.php @@ -220,18 +220,20 @@ public function injectServiceLocator($first, $second) $container = $container->getServiceLocator() ?: $container; } + // Inject AbstractController extensions that are not ServiceLocatorAware + // with the service manager, but do not emit a deprecation notice. We'll + // emit it from AbstractController::getServiceLocator() instead. if (! $controller instanceof ServiceLocatorAwareInterface + && $controller instanceof AbstractController && method_exists($controller, 'setServiceLocator') ) { - trigger_error(sprintf( - 'ServiceLocatorAwareInterface is deprecated and will be removed in version 3.0, along ' - . 'with the ServiceLocatorAwareInitializer. Please update your class %s to remove ' - . 'the implementation, and start injecting your dependencies via factory instead.', - get_class($controller) - ), E_USER_DEPRECATED); + // Do not emit deprecation notice in this case $controller->setServiceLocator($container); } + // If a controller implements ServiceLocatorAwareInterface explicitly, we + // inject, but emit a deprecation notice. Since AbstractController no longer + // explicitly does this, this will only affect userland controllers. if ($controller instanceof ServiceLocatorAwareInterface) { trigger_error(sprintf( 'ServiceLocatorAwareInterface is deprecated and will be removed in version 3.0, along ' diff --git a/src/Service/ServiceManagerConfig.php b/src/Service/ServiceManagerConfig.php index b20dc6aec..4248137b4 100644 --- a/src/Service/ServiceManagerConfig.php +++ b/src/Service/ServiceManagerConfig.php @@ -17,6 +17,7 @@ use Zend\EventManager\SharedEventManagerInterface; use Zend\ModuleManager\Listener\ServiceListener; use Zend\ModuleManager\ModuleManager; +use Zend\ServiceManager\AbstractPluginManager; use Zend\ServiceManager\Config; use Zend\ServiceManager\ServiceLocatorAwareInterface; use Zend\ServiceManager\ServiceManager; @@ -132,7 +133,12 @@ public function __construct(array $config = []) $instance = $first; } - if ($instance instanceof ServiceLocatorAwareInterface) { + // For service locator aware classes, inject the service + // locator, but emit a deprecation notice. Skip plugin manager + // implementations; they're dealt with later. + if ($instance instanceof ServiceLocatorAwareInterface + && ! $instance instanceof AbstractPluginManager + ) { trigger_error(sprintf( 'ServiceLocatorAwareInterface is deprecated and will be removed in version 3.0, along ' . 'with the ServiceLocatorAwareInitializer. Please update your class %s to remove ' @@ -142,13 +148,17 @@ public function __construct(array $config = []) $instance->setServiceLocator($container); } - if (! $instance instanceof ServiceLocatorAwareInterface - && method_exists($instance, 'setServiceLocator') + // For service locator aware plugin managers that do not have + // the service locator already injected, inject it, but emit a + // deprecation notice. + if ($instance instanceof ServiceLocatorAwareInterface + && $instance instanceof AbstractPluginManager + && ! $instance->getServiceLocator() ) { trigger_error(sprintf( 'ServiceLocatorAwareInterface is deprecated and will be removed in version 3.0, along ' - . 'with the ServiceLocatorAwareInitializer. Please update your class %s to remove ' - . 'the implementation, and start injecting your dependencies via factory instead.', + . 'with the ServiceLocatorAwareInitializer. Please update your %s plugin manager factory ' + . 'to inject the parent service locator via the constructor.', get_class($instance) ), E_USER_DEPRECATED); $instance->setServiceLocator($container); diff --git a/test/Application/AllowsReturningEarlyFromRoutingTest.php b/test/Application/AllowsReturningEarlyFromRoutingTest.php index 55f363150..47e731a7a 100644 --- a/test/Application/AllowsReturningEarlyFromRoutingTest.php +++ b/test/Application/AllowsReturningEarlyFromRoutingTest.php @@ -9,7 +9,6 @@ namespace ZendTest\Mvc\Application; -use PHPUnit_Framework_Error_Deprecated; use PHPUnit_Framework_TestCase as TestCase; use Zend\Http\PhpEnvironment\Response; use Zend\Mvc\MvcEvent; @@ -18,12 +17,6 @@ class AllowsReturningEarlyFromRoutingTest extends TestCase { use PathControllerTrait; - public function setUp() - { - // Ignore deprecation errors - PHPUnit_Framework_Error_Deprecated::$enabled = false; - } - public function testAllowsReturningEarlyFromRouting() { $application = $this->prepareApplication(); diff --git a/test/Application/ControllerIsDispatchedTest.php b/test/Application/ControllerIsDispatchedTest.php index 5398a3a1b..d780a6fa8 100644 --- a/test/Application/ControllerIsDispatchedTest.php +++ b/test/Application/ControllerIsDispatchedTest.php @@ -9,7 +9,6 @@ namespace ZendTest\Mvc\Application; -use PHPUnit_Framework_Error_Deprecated; use PHPUnit_Framework_TestCase as TestCase; use Zend\Mvc\MvcEvent; @@ -17,12 +16,6 @@ class ControllerIsDispatchedTest extends TestCase { use PathControllerTrait; - public function setUp() - { - // Ignore deprecation errors - PHPUnit_Framework_Error_Deprecated::$enabled = false; - } - public function testControllerIsDispatchedDuringRun() { $application = $this->prepareApplication(); diff --git a/test/Application/ExceptionsRaisedInDispatchableShouldRaiseDispatchErrorEventTest.php b/test/Application/ExceptionsRaisedInDispatchableShouldRaiseDispatchErrorEventTest.php index a5f7bc291..7defd5347 100644 --- a/test/Application/ExceptionsRaisedInDispatchableShouldRaiseDispatchErrorEventTest.php +++ b/test/Application/ExceptionsRaisedInDispatchableShouldRaiseDispatchErrorEventTest.php @@ -9,7 +9,6 @@ namespace ZendTest\Mvc\Application; -use PHPUnit_Framework_Error_Deprecated; use PHPUnit_Framework_TestCase as TestCase; use Zend\Mvc\MvcEvent; @@ -17,12 +16,6 @@ class ExceptionsRaisedInDispatchableShouldRaiseDispatchErrorEventTest extends Te { use BadControllerTrait; - public function setUp() - { - // Ignore deprecation errors - PHPUnit_Framework_Error_Deprecated::$enabled = false; - } - /** * @group error-handling */ diff --git a/test/Controller/AbstractControllerTest.php b/test/Controller/AbstractControllerTest.php index 4b18a4eeb..6a2d16500 100644 --- a/test/Controller/AbstractControllerTest.php +++ b/test/Controller/AbstractControllerTest.php @@ -11,6 +11,7 @@ use PHPUnit_Framework_TestCase as TestCase; use ReflectionProperty; +use Zend\ServiceManager\ServiceLocatorInterface; /** * @covers \Zend\Mvc\Controller\AbstractController @@ -104,4 +105,15 @@ public function testSetEventManagerWithDefaultIdentifiersIncludesImplementedInte $this->controller->setEventManager($eventManager); } + + public function testRetrievingServiceLocatorRaisesDeprecationNotice() + { + $services = $this->prophesize(ServiceLocatorInterface::class)->reveal(); + + $controller = new TestAsset\SampleController(); + $controller->setServiceLocator($services); + + $this->setExpectedException('PHPUnit_Framework_Error_Deprecated', 'retrieving the service locator'); + $controller->getServiceLocator(); + } } diff --git a/test/Controller/ControllerManagerTest.php b/test/Controller/ControllerManagerTest.php index ad57ae2ba..dbdf76bc3 100644 --- a/test/Controller/ControllerManagerTest.php +++ b/test/Controller/ControllerManagerTest.php @@ -9,7 +9,6 @@ namespace ZendTest\Mvc\Controller; -use PHPUnit_Framework_Error_Deprecated; use PHPUnit_Framework_TestCase as TestCase; use ReflectionClass; use Zend\EventManager\EventManager; @@ -17,18 +16,13 @@ use Zend\Mvc\Controller\ControllerManager; use Zend\Mvc\Controller\PluginManager as ControllerPluginManager; use Zend\ServiceManager\Config; -use Zend\ServiceManager\Factory\InvokableFactory; use Zend\ServiceManager\ServiceManager; use Zend\Console\Adapter\Virtual as ConsoleAdapter; -use ZendTest\Mvc\Service\TestAsset\DuckTypedServiceLocatorAwareController; class ControllerManagerTest extends TestCase { public function setUp() { - // Disable deprecation notices - PHPUnit_Framework_Error_Deprecated::$enabled = false; - $this->sharedEvents = new SharedEventManager; $this->events = $this->createEventManager($this->sharedEvents); $this->consoleAdapter = new ConsoleAdapter(); @@ -151,13 +145,4 @@ public function testDoNotUsePeeringServiceManagers() $this->setExpectedException('Zend\ServiceManager\Exception\ServiceNotFoundException'); $this->controllers->get('EventManager'); } - - public function testServiceLocatorAwareInitializerInjectsDuckTypedImplementations() - { - $this->controllers->setFactory(DuckTypedServiceLocatorAwareController::class, InvokableFactory::class); - - $controller = $this->controllers->get(DuckTypedServiceLocatorAwareController::class); - $this->assertInstanceOf(DuckTypedServiceLocatorAwareController::class, $controller); - $this->assertSame($this->services, $controller->getServiceLocator()); - } } diff --git a/test/Controller/IntegrationTest.php b/test/Controller/IntegrationTest.php index d750bcbd8..baa045149 100644 --- a/test/Controller/IntegrationTest.php +++ b/test/Controller/IntegrationTest.php @@ -9,7 +9,6 @@ namespace ZendTest\Mvc\Controller; -use PHPUnit_Framework_Error_Deprecated; use PHPUnit_Framework_TestCase as TestCase; use Zend\EventManager\EventManager; use Zend\EventManager\SharedEventManager; @@ -22,9 +21,6 @@ class IntegrationTest extends TestCase { public function setUp() { - // Ignore deprecation errors - PHPUnit_Framework_Error_Deprecated::$enabled = false; - $this->sharedEvents = new SharedEventManager(); $this->services = new ServiceManager(); diff --git a/test/Controller/Plugin/ForwardTest.php b/test/Controller/Plugin/ForwardTest.php index a24a4f17e..60a5a4610 100644 --- a/test/Controller/Plugin/ForwardTest.php +++ b/test/Controller/Plugin/ForwardTest.php @@ -9,7 +9,6 @@ namespace ZendTest\Mvc\Controller\Plugin; -use PHPUnit_Framework_Error_Deprecated; use PHPUnit_Framework_TestCase as TestCase; use ReflectionClass; use stdClass; @@ -52,9 +51,6 @@ class ForwardTest extends TestCase public function setUp() { - // Ignore deprecation errors - PHPUnit_Framework_Error_Deprecated::$enabled = false; - $eventManager = $this->createEventManager(new SharedEventManager()); $mockApplication = $this->getMock('Zend\Mvc\ApplicationInterface'); $mockApplication->expects($this->any())->method('getEventManager')->will($this->returnValue($eventManager)); diff --git a/test/Service/ControllerManagerFactoryTest.php b/test/Service/ControllerManagerFactoryTest.php index 6d1a7b461..5ffc53c58 100644 --- a/test/Service/ControllerManagerFactoryTest.php +++ b/test/Service/ControllerManagerFactoryTest.php @@ -9,7 +9,6 @@ namespace ZendTest\Mvc\Service; -use PHPUnit_Framework_Error_Deprecated; use PHPUnit_Framework_TestCase as TestCase; use Zend\EventManager\SharedEventManager; use Zend\Mvc\Service\ControllerManagerFactory; @@ -35,9 +34,6 @@ class ControllerManagerFactoryTest extends TestCase public function setUp() { - // Ignore deprecation errors - PHPUnit_Framework_Error_Deprecated::$enabled = false; - $loaderFactory = new ControllerManagerFactory(); $this->defaultServiceConfig = [ 'aliases' => [ diff --git a/test/Service/ServiceManagerConfigTest.php b/test/Service/ServiceManagerConfigTest.php index 13ea77059..938abed9b 100644 --- a/test/Service/ServiceManagerConfigTest.php +++ b/test/Service/ServiceManagerConfigTest.php @@ -9,7 +9,6 @@ namespace ZendTest\Mvc\Service; -use PHPUnit_Framework_Error_Deprecated; use PHPUnit_Framework_TestCase as TestCase; use ReflectionClass; use stdClass; @@ -38,9 +37,6 @@ class ServiceManagerConfigTest extends TestCase */ protected function setUp() { - // Disable deprecation notices - PHPUnit_Framework_Error_Deprecated::$enabled = false; - $this->config = new ServiceManagerConfig(); $this->services = new ServiceManager(); $this->config->configureServiceManager($this->services); @@ -216,16 +212,4 @@ public function testEventManagerInitializerCanBeReplaced() $serviceManager->get('EventManagerAware'); } - - public function testServiceLocatorAwareInitializerInjectsDuckTypedImplementations() - { - $serviceManager = new ServiceManager(); - (new ServiceManagerConfig(['factories' => [ - TestAsset\DuckTypedServiceLocatorAware::class => InvokableFactory::class, - ]]))->configureServiceManager($serviceManager); - - $instance = $serviceManager->get(TestAsset\DuckTypedServiceLocatorAware::class); - $this->assertInstanceOf(TestAsset\DuckTypedServiceLocatorAware::class, $instance); - $this->assertSame($serviceManager, $instance->getServiceLocator()); - } } diff --git a/test/Service/TestAsset/DuckTypedServiceLocatorAware.php b/test/Service/TestAsset/DuckTypedServiceLocatorAware.php deleted file mode 100644 index 480a464d8..000000000 --- a/test/Service/TestAsset/DuckTypedServiceLocatorAware.php +++ /dev/null @@ -1,27 +0,0 @@ -container = $container; - } - - public function getServiceLocator() - { - return $this->container; - } -} diff --git a/test/Service/TestAsset/DuckTypedServiceLocatorAwareController.php b/test/Service/TestAsset/DuckTypedServiceLocatorAwareController.php deleted file mode 100644 index 1f2da218a..000000000 --- a/test/Service/TestAsset/DuckTypedServiceLocatorAwareController.php +++ /dev/null @@ -1,22 +0,0 @@ - Date: Wed, 2 Mar 2016 09:07:44 -0600 Subject: [PATCH 2/2] Added CHANGELOG for #88 --- CHANGELOG.md | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d585618e9..698da3354 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ All notable changes to this project will be documented in this file, in reverse chronological order by release. -## 2.7.1 - TBD +## 2.7.1 - 2016-03-02 ### Added @@ -18,7 +18,25 @@ All notable changes to this project will be documented in this file, in reverse ### Fixed -- Nothing. +- [#88](https://github.com/zendframework/zend-mvc/pull/88) addresses backwards + compatibility concerns raised by users due to the new deprecation notices + emitted by `ServiceLocatorAware` initializers; in particular, all + `AbstractController` implementations were raising a deprecation wen first + pulled from the `ControllerManager`. + + At this time, notices are now only raised in the following conditions: + + - When a non-controller, non-plugin manager, `ServiceLocatorAware` instance + is detected. + - When a plugin manager instance is detected that is `ServiceLocatorAware` and + does not have a composed service locator. In this situation, the deprecation + notice indicates that the factory for the plugin manager should be updated + to inject the service locator via the constructor. + - For controllers that do not extend `AbstractController` but do implement + `ServiceLocatorAware`. + - When calling `getServiceLocator()` from within an `AbstractController` + extension; this properly calls out the practice that should be avoided and + which requires updates to the controller. ## 2.7.0 - 2016-03-01