deprecate get() for uncompiled container builders #18728
Conversation
| 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 |
| @@ -102,6 +105,9 @@ public function testHas() | |||
| $this->assertTrue($builder->has('bar'), '->has() returns true if a service exists'); | |||
| } | |||
|
|
|||
| /** | |||
| * @group legacy | |||
There was a problem hiding this comment.
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.
You are right. I split this into several small tests with the great effect that none of them needed to be marked as legacy.
|
👍 I like this proposal a lot (I suffered some problems related to this recently) |
2d0faf7 to
4adf349
Compare
4adf349 to
42860eb
Compare
| @@ -409,6 +411,10 @@ public function has($id) | |||
| */ | |||
| public function get($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.
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 using set() before). Alternatively, deprecating set() too might be another way to go.
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
|
Both related PRs have been merged now. |
|
@fabpot Thanks. This will need to be rebased then when lower branches are merged up. |
| @@ -409,6 +411,10 @@ public function has($id) | |||
| */ | |||
| public function get($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.
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.
good catch, I added parentheses after the method name
42860eb to
64f8a7e
Compare
|
👍 |
| @@ -4,6 +4,9 @@ UPGRADE FROM 3.x to 4.0 | |||
| DependencyInjection | |||
| ------------------- | |||
|
|
|||
| * 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.
wdyt about replacing and will throw by and throws ?
|
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. |
|
@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) |
|
@stof OK, In my case it's only about the parameters so that's something I can change. |
|
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 @xabbuh This reverts commit 27f4680, reversing changes made to e4177a0. 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. See https://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 (closes symfony#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