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

[2.8] Deprecation notice when using factory_service/factory_method #14687

Closed
linaori opened this issue May 19, 2015 · 10 comments
Closed

[2.8] Deprecation notice when using factory_service/factory_method #14687

linaori opened this issue May 19, 2015 · 10 comments

Comments

@linaori
Copy link
Contributor

linaori commented May 19, 2015

Starting on a PR regarding another issue and I hooked up on of my applications to 2.8 instead of 2.6. For me this was 2 seconds to figure out it was caused by the old way of defining a factory service + method, but the error message might cause a lot of confusion for others. The message is thrown in the Definition class rather than the place where you expect it (Yaml Loader in this case).

The fix for this is to change your service definition:

    # old
    app.layout.menu.beta:
        class: Knp\Menu\MenuItem
        factory_service: app.layout.menu_builder
        factory_method: createBetaMenu
        tags:
            - { name: knp_menu.menu, alias: beta }

    # new
    app.layout.menu.beta:
        class: Knp\Menu\MenuItem
        factory: [@app.layout.menu_builder, createBetaMenu]
        tags:
            - { name: knp_menu.menu, alias: beta }

The thing is that the error message below does not clearly reflect that. I don't think it's desired to put this information in the message below, but maybe someone has a nice idea where to put it instead.

Deprecated: The Symfony\Component\DependencyInjection\Definition::setFactoryMethod method is deprecated since version 2.6 and will be removed in 3.0. Use Definition::setFactory() instead. in /home/ivanderberg/projects/symfony/src/Symfony/Component/DependencyInjection/Definition.php on line 137
@xabbuh
Copy link
Member

xabbuh commented May 19, 2015

To get a more sensible deprecation message, we will have to trigger them in the loader classes. However, this would mean that one one get two messages, one from the loader and one from the Definition class.

I see two solutions to get rid of the double message:

  • Add an argument to optionally silence the deprecated method (we did that in some other places where we didn't have another option for internal code).
  • Stop the loaders from using setFactoryClass(), setFactoryService() and setFactoryMethod() and let them call setFactory() instead regardless of how the user configured the factory. Will that solution have any downsides?

@Tobion
Copy link
Member

Tobion commented May 19, 2015

Just add to the deprecation message in the Definition that it could also be caused by using factory_service and factory_method in configuration file.

@linaori
Copy link
Contributor Author

linaori commented May 19, 2015

Another question related to this though, I have fixed all my application usages, but composer is still complaining. I suspect this is coming from a third-party bundle, but I am not sure which or where just yet.

The problem is that I can't suppress those warnings in composer and it will cause a deployment failure on the cache:warmup. Any tips on that issue?

@xabbuh
Copy link
Member

xabbuh commented May 19, 2015

@Tobion Though you would still not know which file actually causes the issue. Given that the warning can also be triggered by a third-party bundle, this could become hard to debug imho.

@Tobion
Copy link
Member

Tobion commented May 19, 2015

@xabbuh true

@stof
Copy link
Member

stof commented May 20, 2015

@xabbuh calling setFactory is not an option, because it would force to have a service definition defining the whole factory at once, while the old syntax allows that (think about a service overwriting only part of the factory compared to the parent)

fabpot added a commit that referenced this issue May 22, 2015
…ameters given (iltar)

This PR was merged into the 2.7 branch.

Discussion
----------

[2.7][DI] Definition deprecation notice includes the parameters given

| Q             | A
| ------------- | ---
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #14687
| License       | MIT

This PR won't fix the issue at hand, but should make it a bit easier to debug. Because this is used by the configuration loader, it's hard to trace back where and how it's actually used.

Commits
-------

b322d46 Definition deprecation notice includes the parameters given
@eXtreme
Copy link
Contributor

eXtreme commented May 24, 2015

I've had similar problems as @iltar with composer/warmup spamming with deprecations - afair it was caused by sonata admin dependency and as a quick fix I placed error_reporting(E_ALL & ~E_USER_DEPRECATED & ~E_DEPRECATED); in the constructor of my kernel.

@nicolas-grekas
Copy link
Member

Having better error messages would be great, even in 2.7.x releases IMO. I wouldn’t mind the duplicate too much.

@xabbuh
Copy link
Member

xabbuh commented May 31, 2015

see #14798

@gnutix
Copy link
Contributor

gnutix commented May 31, 2015

Is there a way to filter the deprecation messages that comes from the application code (app/ & src/) and not 3rd-party code (vendor/) ?

fabpot added a commit that referenced this issue Jun 4, 2015
…using deprecated configuration options (xabbuh)

This PR was merged into the 2.7 branch.

Discussion
----------

[DependencyInjection] provide better error message when using deprecated configuration options

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #14687
| License       | MIT
| Doc PR        |

Commits
-------

e1e7440 [DependencyInjection] provide better error message when using deprecated configuration options
@fabpot fabpot closed this as completed Jun 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants