8000 Service defaults not working for factories · Issue #22143 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Service defaults not working for factories #22143

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
curry684 opened this issue Mar 24, 2017 · 3 comments
Closed

Service defaults not working for factories #22143

curry684 opened this issue Mar 24, 2017 · 3 comments
Labels
Bug DependencyInjection Status: Needs Review

Comments

@curry684
Copy link
Contributor
Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no
Symfony version dev-master

Given the new features with anonymous services and service defaults, I was assuming the following syntax would work:

services:
    _defaults:
        factory:        [ "@doctrine", getRepository ]

    MyProject\UserRepository:
        arguments:      [ "AppBundle:User" ]

This errors with: "Please add the class to service "_defaults" even if it is constructed by a factory since we might need to add method calls based on compile-time checks."

Changing the defaults to:

    _defaults:
        class:          Doctrine\ORM\EntityRepository
        factory:        [ "@doctrine", getRepository ]

Appears to work at first, but doesn't, as this actually causes a service called _defaults to be created, and the other services in the file to ignore the defaults altogether.
@curry684 curry684 mentioned this issue Mar 24, 2017
Closed
@curry684
Copy link
Contributor Author

As a sidenote: shouldn't we completely be disallowing creation of a service called _defaults or _instanceof whatsoever? Or do we consider that BC breakage which it highly theoretically is?

@ro0NL
Copy link
Contributor
ro0NL commented Mar 27, 2017 •
edited
Loading

Or do we consider that BC breakage which it highly theoretically is?

Yes 😞
https://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php#L328

The whole _defaults, _instanceof etc. services are ignored if we assume it's a regular service;
https://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php#L296

As of 4.0 this could be possible, and with _instanceof it will be possible. It should work with XML currently though...

I like your usecase.. i would have tried exactly that.

@nicolas-grekas nicolas-grekas mentioned this issue Mar 27, 2017
Merged
@curry684 curry684 mentioned this issue Mar 27, 2017
Closed
nicolas-grekas added a commit that referenced this issue Mar 28, 2017
@nicolas-grekas
feature #22185 [DI] Enhance DX by throwing instead of triggering a de…
05d15cf 
…precation notice (nicolas-grekas)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[DI] Enhance DX by throwing instead of triggering a deprecation notice

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | yes - at the config file level, for edge cases
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #22143
| License       | MIT
| Doc PR        | -

Looking at the linked issue, I'm reconsidering our decision to trigger a deprecation notice when one uses `_instanceof` or `_defaults` as a service name. While on the BC side, this is strict - on the DX side, it looks like this opens a trap where people fill fall into.

The same occurs to me with named args: instead of silently accepting invalid args as was the case before, let's throw to help DX when people do mistakes.

Last change in this PR: the complex logic required to force strings to be given as `$id` args into `Reference` or `Alias` makes no sense to me, especially considering that a `string` type hint on PHP7 will *do* a string cast.

Commits
-------

b07da3d [DI] Enhance DX by throwing instead of triggering a deprecation notice
@curry684
Copy link
Contributor Author

Error part closed by #22185, feature part superseded by #22189

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug DependencyInjection Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@ro0NL @curry684 @xabbuh @carsonbot
0