Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
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
Labels
DependencyInjection

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
Contributor

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
Contributor

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)

@linaori linaori mentioned this issue May 22, 2015
Merged
fabpot added a commit that referenced this issue May 22, 2015
@fabpot
minor #14720 [2.7][DI] Definition deprecation notice includes the par…
3e7b19e 
…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 xabbuh mentioned this issue May 31, 2015
Merged
@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/) ?

@xabbuh xabbuh mentioned this issue May 31, 2015
Closed
@matejvelikonja matejvelikonja mentioned this issue Jun 2, 2015
Merged
@xabbuh xabbuh mentioned this issue Jun 3, 2015
Closed
fabpot added a commit that referenced this issue Jun 4, 2015
@fabpot
minor #14798 [DependencyInjection] provide better error message when …
e8d06b2 
…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
@xabbuh xabbuh mentioned this issue Jun 11, 2015
Merged
This was referenced Feb 21, 2016
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
DependencyInjection
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@fabpot @eXtreme @nicolas-grekas @gnutix @stof @Tobion @linaori @xabbuh