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

Deprecation warning #87

Closed
metanav opened this issue Mar 2, 2016 · 40 comments
Closed

Deprecation warning #87

metanav opened this issue Mar 2, 2016 · 40 comments
Assignees
Labels

Comments

@metanav
Copy link

metanav commented Mar 2, 2016

I am getting following warning after upgrading zend-mvc:
ServiceLocatorAwareInterface is deprecated and will be removed in version 3.0, along with the ServiceLocatorAwareInitializer. Please update your class My\Controller\MyController to remove the implementation, and start injecting your dependencies via factory instead.

I have read the CHANGELOG and I understand why this warning is coming. But none of the controller classes is implementing ServiceLocatorAwareInterface in my application. I think due to extending AbstractController class which has setServiceLocator() method I am getting this warning. I know if I need I can inject it through factory. I would prefer to see this warning only if any classes is implementing ServiceLocatorAwareInterface, not for merely defining a method setServiceLocator() which can be there even if we inject it using setters in factory instead of constructor injection.

@froschdesign
Copy link
Member

@metanav
Copy link
Author

metanav commented Mar 2, 2016

That's my concern. Now there is no implementation of ServiceLocatorAwareInterface so why this warning?

@froschdesign
Copy link
Member

@metanav
I updated my previous comment. (Sorry, I was too slow!)

@froschdesign
Copy link
Member

I think, the warning is good, but message is wrong.

@metanav
Copy link
Author

metanav commented Mar 2, 2016

I agree with you regarding the message. But could you please explain the logic behind triggering this warning when you have a method setServiceLocator in your controller? Suppose I have controller and factory like this:
(Note: Neither I am extending AbstractController class or implementing ServiceLocatorAwareInterface)

class MyControllerFactory
 {
    public function __invoke($serviceLocator)
    {
        $myController = new MyController();
        $myController->setServiceLocator($serviceLocator);
    }
}

class MyController
{
    public function __constructor() {}
    public function setServiceLocator($serviceLocator) 
    {
         $this->serviceLocator = $serviceLocator;
    }
}

@froschdesign
Copy link
Member

But could you please explain the logic behind triggering this warning when you have a method setServiceLocator in your controller?

With a factory you do not need the Service Locator in your controller.

@bgaillard
Copy link

Hi, we also encounter this problem, sorry to complain again but this is the third problem we have in one week with the ZF 2 updates :(

Could this be fixed quickly ?

@wtabata
Copy link

wtabata commented Mar 2, 2016

In all my controllers appear as message: " Deprecated: ServiceLocatorAwareInterface is deprecated and will be removed in version 3.0 [...]"

:( and now!?

@someother1
Copy link

same here - i recognized multiple changes in the past weeks, which will break here or there some things (example: bjyoungblood/BjyAuthorize#304).

its like, working with an apple nowadays..

please provide some informations what to do, to clear the deprecated issue

@rajitha081189
Copy link

" Deprecated: ServiceLocatorAwareInterface is deprecated and will be removed in version 3.0, along with the ServiceLocatorAwareInitializer. " I am getting this error all over my application controller. Can anyone help?

@froschdesign
Copy link
Member

The message is good and important for all developers!

For you production system you should use a different error_reporting level.

@someother1
Copy link

Sorry, but this is, at least for me, inacceptable.
As i develop in my development system to see all errors, this deprectaed message is breaking my layout.
For sure i could supress by set another error_reporting, but i want to see all errors on development.
I dont use the "setServiceLocator" method in any class, and as "metanav" already wrote, its a default extend which have to be defined for every controller.
Please, modify the check, so that if the method is not used in our classes, dont let this deprecated message appear.
I will go to 2.6.3 back until there is a fix.

This will do the trick: in "composer.json"
"zendframework/zendframework": "2.*",
"zendframework/zend-mvc": "2.6.3",

@rajitha081189
Copy link

Still the issue exists. I had given error_reporting(0); in my controller function. But it is not working as i expected. Is it possible for me to change the version of Zend service Manager to Previous?

@froschdesign
Copy link
Member

@someother1

…but i want to see all errors on development.

This is correct. Therefore I wrote:

For you production system you should use a different…

@froschdesign
Copy link
Member

@rajitha081189

I had given error_reporting(0); in my controller function.

This is too late! error_reporting is a configuration option so use the config files of your application or the PHP configuration.

@bgaillard
Copy link

@froschdesign

The message is good and important for all developers!

IMO doing this triggers the warning.

class MyController extends AbstractActionController {}

So here using the API normally (i.e as explained in the ZF 2 docs) triggers a Warning.

So I do not agree with you because a warning should be displayed if an API is used with abnormal calls and manipulations (which is definitly not the case here).

@rajitha081189
Copy link

Ok thanks. Its work for me right now. When I put it on the global.php file in Config folder. But need a better solution.

@froschdesign
Copy link
Member

@bgaillard

So I do not agree with you because a warning should be displayed if an API is used with abnormal calls and manipulations (which is definitly not the case here).

This is only a warning for deprecation. Nothing more or less.
Trigger a E_USER_DEPRECATED message is the normal and right way.

@froschdesign
Copy link
Member

@rajitha081189

But need a better solution.

This is the normal way for E_USER_DEPRECATED. (On your production system you should use the PHP configuration.)

@metanav
Copy link
Author

metanav commented Mar 2, 2016

@froschdesign I understand what you mean and I don't have any issue with E_USER_DEPRECATED or any such kind of error reporting messages, they are really helpful. But they are useful only when they are meaningful and in appropriate places. The first case when a developer implements 'ServiceLocatorAwareInterface' it MUST be triggered but not for the other case when only 'setServiceLocator' is defined in the class or any parent class.

@froschdesign
Copy link
Member

@metanav

The first case when a developer implement 'ServiceLocatorAwareInterface' it MUST be triggered

Right!

but not for the other case when only setServiceLocator is defined in the class or any parent class.

The error message is wrong, but the hint for this "old" standard method is correct. In version 3.0 this messages will removed. It's only a small migration step in 2.7 to 3.0.

@metanav
Copy link
Author

metanav commented Mar 2, 2016

Do you mean we should never inject a service locator or have a setServiceLocator method in a controller class because it is an anti-pattern? If yes, I think this is fine and only thing is the message should be changed for the other case.

@froschdesign
Copy link
Member

…we should never inject a service locator or have a setServiceLocator method in a controller class because it is an anti-pattern?

You get it! 👍

@snapshotpl
Copy link
Contributor

Ok, but Zend\Mvc\Controller\AbstractController by default have still setServiceLocator method. It's any way to disable this warning? Maybe should we remove setServiceLocator method and implement it by __call?

@froschdesign
Copy link
Member

@snapshotpl

It's any way to disable this warning?

See error_reporting.

@snapshotpl
Copy link
Contributor

@froschdesign I know about it. But I think that should be other way to do that (refactor something). zend-mvc shouldn't trigger warnings by default, only my bad usage. __call will be good replacement

@metanav
Copy link
Author

metanav commented Mar 2, 2016

I think @snapshotpl has a good point. This way we can get rid of setServiceLocator() method and also it will be BC/FC.

@snapshotpl
Copy link
Contributor

Or add option to disable injectServiceLocator method

@snapshotpl
Copy link
Contributor

If $serviceLocator property in AbstractController was private then getServiceLocator usage would trigger warning. Any other ideas?

@dymen1
Copy link

dymen1 commented Mar 2, 2016

@froschdesign Why is it necessary to throw an error when the method setServiceLocator exists? I know we should use a factory to inject dependencies, but what if I want to use that method name for something else?

@froschdesign
Copy link
Member

@dymen1
It's only for migration from version 2 to 3. In future versions you can use setServiceLocator without deprecation warnings.

@froschdesign
Copy link
Member

@snapshotpl
ServiceLocatorAwareInterface is deprecated and also the included method setServiceLocator. Trigger an error with E_USER_DEPRECATED is the right way, because this code will not work in future versions.
It's a good practice to prepare the migration to a newer version with BC breaks.

froschdesign referenced this issue Mar 2, 2016
Do not check if ServiceLocatorAwareInterface exists, as that will skip
the initializer when it does, but the instance does not implement it and
*does* fit duck typing rules.
@dymen1
Copy link

dymen1 commented Mar 2, 2016

@froschdesign I understand that it's for easier migration, the only issue right now is that \Zend\Mvc\Controller\AbstractController has a setServiceLocator method. This results in most Controller classes throwing the deprecation error.

@froschdesign
Copy link
Member

@dymen1

This results in most Controller classes throwing the deprecation error.

Right! But:

  1. This should be not a problem in a production environment. (error_reporting)
  2. A deprecation warning in version 3 is too late. So it can be only the previous version.

@axelcho
Copy link

axelcho commented Mar 2, 2016

@froschdesign It is not a matter of right or wrong. By throwing a warning across the system, you are seriously interfering day to day operation of many developers. In real life I have to explain to other developers why we should use Zend Framework, and my best argument so far was enterprise friendly mature framework. The recent hiatus in Zend Development made me seriously doubt about that assessment.

Developers are seeing this warning NOT in the production system, but in their development system where they turned on all warnings. It is good to let developers know about coming changes, but it should not be by intentionally break develop apps. It is very bad idea to turn off any of warnings in develop machines, including deprecation warning. So your warning is doing much more harm than any good.

Please publish a fix as soon as possible.

@froschdesign
Copy link
Member

@axelcho

It is very bad idea to turn off any of warnings in develop machines, including deprecation warning.

If you want to use the latest version in your development, than you can turn off only the E_USER_DEPRECATED warnings or you need to think about porting your code. This is a harsh solution, but easier if there is no warning for deprecation and with version 3 nothing works anymore.

The recent hiatus in Zend Development made me seriously doubt about that assessment.

If you follow ZF on Github or Twitter you can jump in before these changes we merged into the master branch: https://twitter.com/zfdevteam/status/704448443519873024

Yes, help is needed!

@froschdesign
Copy link
Member

@ all
Thanks for the feedback. We will wait for @weierophinney the project maintainer.

@weierophinney
Copy link
Member

I'm just waking up now and seeing this. I have a few ideas on how to proceed, and will be implementing then as soon as I have a chance. In the meantime, I'll be locking the thread; we have more than enough information to proceed with at this time.

@zendframework zendframework locked and limited conversation to collaborators Mar 2, 2016
@weierophinney
Copy link
Member

A patch is ready in #88; I'm waiting for Travis to run before merging and tagging. If anybody wants to test, add the following to your composer.json:

"repositories": [
    {"type": "vcs", "url": "https://github.com/weierophinney/zend-mvc.git"}
],

and add a requirement for zend-mvc as follows:

"zendframework/zend-mvc": "dev-hotfix/deprecations as 2.7.1"

and then run composer update.

@froschdesign
Copy link
Member

Fixed with #88 and released as 2.7.1

Slamdunk pushed a commit to Slamdunk/zend-mvc that referenced this issue Jun 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

10 participants