Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trigger a E_USER_DEPRECATED when I used a deprecated service #14307

Closed
jderusse opened this issue Apr 10, 2015 · 5 comments
Closed

Trigger a E_USER_DEPRECATED when I used a deprecated service #14307

jderusse opened this issue Apr 10, 2015 · 5 comments

Comments

@jderusse
Copy link
Member

My current problem is the following:
I upgrade a legacy application to 2.8. And I've now a E_USER_DEPRECATED: Symfony\Component\Form\Extension\Csrf\CsrfProvider\CsrfProviderAdapter class is deprecated since version 2.4.
I search a lot and found that the problem was the parameter csrf_provider in security.yml who's defined to the service form.csrf_provider. Finally this service use the deprecated class CsrfTokenManagerAdapter which let FormTypeCsrfExtension to instanciate a CsrfProviderAdapter... ouch.. hard to debug and find the source of the deprecation.

An idea to help is such case, is to trigger the E_USER_DEPRECATED earlier.
We (@nicolas-grekas and me) thinks that it could be ncie to trigger the error when the service is retreived from the container: A solution => When the container is compiled, we can Reflect the class defined in the service, and, automaticly add a trigger_error if the @deprecated annotation is found.

@nicolas-grekas
Copy link
Member

Rethinking about this, it would even be simpler and more reliable to just have a "deprecated" tag in the deprecated services definition. In debug mode, this should trigger deprecation notices.

@cordoval
Copy link
Contributor

@nicolas-grekas is there a way to avoid notices even in debug mode? sometimes even we are doing the right thing, e.g. using twig.form_themes: [...] instead of say twig.form.resources but because there is a setter $container->setParameter('twig.form.resources', $config['form_themes']); even on 2.8 https://github.com/symfony/symfony/blob/2.8/src/Symfony/Bundle/TwigBundle/DependencyInjection/TwigExtension.php#L58 we get the notice for no reason.

Should I send a PR to fix this or what is the purpose of this. Thanks in advance for the explanation. 👴

@nicolas-grekas
Copy link
Member

@cordoval see #14305. And yes, this needs a bug fix on 2.7

@wouterj
Copy link
Member

wouterj commented Apr 12, 2015

Checking for a @deprecated annotation on the class is not always enough (a service can also be renamed). So I'm +1 for a deprecated tag (which can be provided by the DebugBundle).

@hhamon
Copy link
Contributor

hhamon commented Apr 19, 2015

A deprecated tag seems to be a smart solution.

fabpot added a commit that referenced this issue Sep 25, 2015
This PR was merged into the 2.8 branch.

Discussion
----------

Add support for deprecated definitions

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #14307
| License       | MIT
| Doc PR        | symfony/symfony-docs#5689

This add a sort of marker in the Definition of a service that marks it as "deprecated". This is useful when we have a bunch of service and a bunch of where it is used, and we need to track if there are any uses before removing it (in a later version or right now). I was not sure if the `trigger_error` would be enough, or if I should log them instead.

I'm first gathering some feedback, and then I'll try to update the doc.

I was not sure if it should target 2.8 or master (3.0) though.

What's left ?
==========
- [x] Make a POC
- [x] Gather some feedbacks
- [x] Dump the tag in XML, YAML and PHP
- [x] Load the definition from XML, YAML and PHP
- [x] Fix some forgotten things such as the key existence check
- [x] Work on inline services in the php dumper
- [x] Handle deprecations for decorators
- ~~Possibility to overwrite the deprecated flag in the decorators in `XmlFileLoader` ?~~ Nope, and this behavior is also ported to the `YamlFileLoader`.

Commits
-------

83f4e9c [DI] Support deprecated definitions in decorators
0b3d0a0 [DI] Allow to change the deprecation message in Definition
954247d [DI] Trigger a deprecated error on the container builder
2f37cb1 [DI] Dump the deprecated status
8f6c21c [DI] Supports the deprecated tag in loaders
4b6fab0 [DI] Add a deprecated status to definitions
@fabpot fabpot closed this as completed Sep 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants