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

Better ServiceLocatorAware deprecation #88

Merged

Conversation

weierophinney
Copy link
Member

This is a patch for fixing #87.

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.

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.
@weierophinney weierophinney mentioned this pull request Mar 2, 2016
@weierophinney weierophinney added this to the 2.7.1 milestone Mar 2, 2016
@weierophinney weierophinney self-assigned this Mar 2, 2016
@weierophinney weierophinney merged commit 0a531e2 into zendframework:master Mar 2, 2016
weierophinney added a commit that referenced this pull request Mar 2, 2016
weierophinney added a commit that referenced this pull request Mar 2, 2016
weierophinney added a commit that referenced this pull request Mar 2, 2016
@weierophinney weierophinney deleted the hotfix/deprecations branch March 2, 2016 15:09
@froschdesign
Copy link
Member

Better! 👍

@snapshotpl
Copy link
Contributor

Much better! 👍

@snapshotpl
Copy link
Contributor

I see one case which allow users to use service locator in controllers without warning - by serviceLocator property. Are we should care about this? @weierophinney

@weierophinney
Copy link
Member Author

@snapshotpl Too difficult to manage (we'd have to use property overloading, and that gets messy fast). Those users will feel the pain when they upgrade to zend-mvc v3 later, and it will be covered in the migration guide at that time.

@bgaillard
Copy link

@weierophinney Thanks, it works on our project now and we do not encounter the deprecated warning 👍

@kevin-olbrich
Copy link

I encountered a problem regarding this deprecation:

Sample Code:

        $em = $this->getServiceLocator()->get('doctrine.entitymanager.orm_default');

        echo "test1".PHP_EOL;

        $machines = $em->getRepository('Main\Entity\DataMachines')->findAll();

        echo "test2".PHP_EOL;
        die("die here...");

Output:

C:\secret>php public\index.php my-cli-job

Deprecated: You are retrieving the service locator from within the class CronJobs\Controller\SecretController. 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. in C:\secret\vendor\zendframework\zend-mvc\src\Controller\AbstractController.php on line 258
test1

C:\secret>

The job silently dies when I call a function of doctrine.

@kevin-olbrich
Copy link

Problem fixed. Doctrine-only problem.
doctrine/DoctrineORMModule#461

@chrisVdd
Copy link

chrisVdd commented Oct 5, 2016

Hi!

Could you please explain me how I can have this patch in my local project please ?
Thanks!

@froschdesign
Copy link
Member

@chrisVdd
Update the Zend\Mvc component per composer. For example:

  • Version 2.7.10 for ZF2
  • Version 3.0.3 for ZF3

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants