Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
deprecate get() for uncompiled container builders#18728
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
xabbuh commentedMay 9, 2016
| Q | A |
|---|---|
| Branch? | 3.2 |
| Bug fix? | no |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | yes |
| Tests pass? | yes |
| Fixed tickets | #18673 (comment) |
| License | MIT |
| Doc PR |
| } | ||
| /** | ||
| * @group legacy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
not all this test is legacy. It would be a great time to split the test into multiple smaller tests to mark only the relevant one as legacy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You are right. I split this into several small tests with the great effect that none of them needed to be marked as legacy.
javiereguiluz commentedMay 9, 2016
👍 I like this proposal a lot (I suffered some problems related to this recently) |
2d0faf7 to4adf349Comparexabbuh commentedMay 9, 2016
| { | ||
| if (!$this->compiled) { | ||
| @trigger_error(sprintf('Calling %s before compiling the container is deprecated since version 3.2 and will throw an exception in 4.0.',__METHOD__),E_USER_DEPRECATED); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We may want to think about moving this down after the call to the parent class'get() method (so you would be able to retrieve services that have been added usingset() before). Alternatively, deprecatingset() too might be another way to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
See#19192
This PR was merged into the 2.3 branch.Discussion----------call get() after the container was compiled| Q | A| ------------- | ---| Branch? | 2.3| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |This will prevent future issues when calling `ContainerBuilder::get()` before compiling the container will be deprecated (see#18728).Commits-------954126b call get() after the container was compiled
…ner service (xabbuh)This PR was merged into the 2.7 branch.Discussion----------[FrameworkBundle] prevent calling get() for service_container service| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |This change will simply fix the tests once#18728 gets merged. An alternative approach would be to compile the container so that the code would still work even for services that have been set directly using `set()`. However, compiling the container in a descriptor imo is an unexpected side effect which I tried to avoid here.Commits-------2d46bd4 prevent calling get() for service_container service
fabpot commentedMay 9, 2016
Both related PRs have been merged now. |
xabbuh commentedMay 9, 2016
@fabpot Thanks. This will need to be rebased then when lower branches are merged up. |
| publicfunctionget($id,$invalidBehavior = ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE) | ||
| { | ||
| if (!$this->compiled) { | ||
| @trigger_error(sprintf('Calling %s before compiling the container is deprecated since version 3.2 and will throw an exception in 4.0.',__METHOD__),E_USER_DEPRECATED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This error message will be displayed like this:
Calling get before compiling the container is ...get here may be confusing. What if we add the( and) to the method name?
Calling get() before compiling the container is ...Or wrap the method name with quotes:
Calling "get" before compiling the container is ...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
good catch, I added parentheses after the method name
42860eb to64f8a7eComparejaviereguiluz commentedMay 19, 2016
👍 |
| ------------------- | ||
| * Calling`get()` on a`ContainerBuilder` instance before compiling the | ||
| container is not supported anymore and will throw an exception. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
wdyt about replacingand will throw byand throws ?
sstok commentedJun 7, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Is there a better alternative for this? I'm actually using this. Note. I'm only doing this ease the parameters resolving, else I need to do this manually and this is easier. |
stof commentedJun 7, 2016
@sstok the issue is that you actually have no guarantee at all that your own code will work. The container is not yet ready to be used at this point, so your code works only if you are lucky (and any bundle could put the tag on a service requiring more configuration work done after your compiler pass and make your own compiler pass break badly, potentially with a fatal error) |
sstok commentedJun 7, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@stof OK, In my case it's only about the parameters so that's something I can change. |
fabpot commentedJun 8, 2016
Thank you@xabbuh. |
…buh)This PR was squashed before being merged into the 3.2-dev branch (closes#18728).Discussion----------deprecate get() for uncompiled container builders| Q | A| ------------- | ---| Branch? | 3.2| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets |#18673 (comment)| License | MIT| Doc PR |Commits-------f2e35cb deprecate get() for uncompiled container builders
…builders" (nicolas-grekas)This PR was merged into the 3.2-dev branch.Discussion----------[DI] Revert "deprecate get() for uncompiled container builders"| Q | A| ------------- | ---| Branch? | 3.2| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#18728| License | MIT| Doc PR | -ping@xabbuhThis reverts commit27f4680, reversingchanges made toe4177a0.While upgrading a few projects to 3.2, I found it impossible to remove this deprecation without duplicating "by hand" the instantiation logic of some service needed in a compiler pass.Seehttps://github.com/dustin10/VichUploaderBundle/blob/master/DependencyInjection/Compiler/RegisterPropelModelsPass.php#L30 for such an example into the wild.I think we should revert this deprecation. Using the container builder before it is compiled is tricky. Yet, bootstrapping in general is tricky and full of chicken-and-egg issues. In this case, using some services while bootstrapping the container works well. Deprecating would be fine if we were to provide some viable alternative. Duplicating the instantiation logic doesn't qualify as such to me, hence the proposed revert.@xabbuh, if you'd like to keep some test cases that may be unrelated to the revert, please add comments on the PR, or better: push on my fork, branch revert-get-deprec.Commits-------e449af0 Revert "feature#18728 deprecate get() for uncompiled container builders (xabbuh)"
…rs (xabbuh)This PR was squashed before being merged into the 3.2-dev branch (closessymfony#18728).Discussion----------deprecate get() for uncompiled container builders| Q | A| ------------- | ---| Branch? | 3.2| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets |symfony#18673 (comment)| License | MIT| Doc PR |Commits-------f2e35cb deprecate get() for uncompiled container builders