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.

Author: weierophinney

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;
     }
 

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 '

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);

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();

test/Application/ControllerIsDispatchedTest.php

@@ -9,20 +9,13 @@
 
 namespace ZendTest\Mvc\Application;
 
-use PHPUnit_Framework_Error_Deprecated;
 use PHPUnit_Framework_TestCase as TestCase;
 use Zend\Mvc\MvcEvent;
 
 class ControllerIsDispatchedTest extends TestCase
 {
     use PathControllerTrait;
 
-    public function setUp()
-    {
-        // Ignore deprecation errors
-        PHPUnit_Framework_Error_Deprecated::$enabled = false;
-    }
-
     public function testControllerIsDispatchedDuringRun()
     {
         $application = $this->prepareApplication();

test/Application/ExceptionsRaisedInDispatchableShouldRaiseDispatchErrorEventTest.php

@@ -9,20 +9,13 @@
 
 namespace ZendTest\Mvc\Application;
 
-use PHPUnit_Framework_Error_Deprecated;
 use PHPUnit_Framework_TestCase as TestCase;
 use Zend\Mvc\MvcEvent;
 
 class ExceptionsRaisedInDispatchableShouldRaiseDispatchErrorEventTest extends TestCase
 {
     use BadControllerTrait;
 
-    public function setUp()
-    {
-        // Ignore deprecation errors
-        PHPUnit_Framework_Error_Deprecated::$enabled = false;
-    }
-
     /**
      * @group error-handling
      */

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();
+    }
 }

test/Controller/ControllerManagerTest.php

@@ -9,26 +9,20 @@
 
 namespace ZendTest\Mvc\Controller;
 
-use PHPUnit_Framework_Error_Deprecated;
 use PHPUnit_Framework_TestCase as TestCase;
 use ReflectionClass;
 use Zend\EventManager\EventManager;
 use Zend\EventManager\SharedEventManager;
 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());
-    }
 }

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();

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));