[DI] Deprecate Container::isFrozen and introduce isCompiled #19673

ro0NL · 2016-08-19T12:33:21Z

Q	A
Branch?	"master"
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	comma-separated list of tickets fixed by the PR, if any
License	MIT
Doc PR	reference to the documentation PR, if any

This deprecates the concept of freezing a container, implied by Container::isFrozen. However, freezing happens due compilation (Container::compile). So having just isCompiled instead seems more intuitive, and plays along well with ContainerBuilder.

Before/After;

Container::isFrozen
- Checks if the parameter bag is frozen, but is deprecated in 3.2
- In 4.0 this methods does not exists and can be replaced with getParameterBag() instanceof FrozenParameterBag or isCompiled(). Depending on what you want (to clarify; the behavior is different when passing a frozen bag to the constructor)
Container::isCompiled
- Truly checks if compile() has ran, and is a new feature
ContainerBuilder::merge etc.
- Now uses isCompiled instead of isFrozen, ie. we allow for it till compilation regarding the state of the paramater bag

ro0NL · 2016-08-19T13:05:50Z

In some ways it was actually compiling (resolving the parameters), as this is also done in a compiler pass. So we can also just go with compile() + isCompiled() on both sides. And deprecate the concept of freezing, which breaks;

Still allowing to compile a already frozen container builder.

stof · 2016-08-19T13:26:16Z

-1 for renaming compile to freeze. This method is not only about freezing. It is about performing all the compilation steps of the container building (everything done in compiler passes for instance).

ro0NL · 2016-08-19T13:57:54Z

@stof different approach. WDYT?

fabpot · 2016-08-19T16:01:26Z

I don't understand the problem you are trying to fix here. Can you elaborate? Deprecating something should be taken lightly, so we need to have a very good reason to rename something.

ro0NL · 2016-08-19T16:14:18Z

It's confusing to mix&match freeze/is-frozen and compile/is-compiled. isFrozen is currently enabled due compilation (ie. we practically have compile/is-frozen). Imo, the parameter bag is frozen; the container compiled.

This exposes a minor code smell in ContainerBuilder where it's additionally tracking state (ContainerBuilder::$compiled) where this is actually the same as checking if it's frozen (ie. compiled). I guess the difference was unclear back then, and you get "better be safe then sorry" changes.

ro0NL · 2016-08-19T17:37:52Z

src/Symfony/Component/DependencyInjection/Container.php

-     * @return bool true if the container parameter bag are frozen, false otherwise
+     * @return bool
+     */
+    final public function isCompiled()


This is now the ought to use method. If you truly want to check for a frozen parameter bag use
getParameterBag() instanceof FrozenParameterBag instead (hence the deprecation).

Besides, this method now guarantees state of compile() as we dont depend on a frozen parameterbag for that anymore (which we can pass to the constructor ;-)).

Now.., i assume the calls to isFrozen in ContainerBuilder (or practically anywhere) are intended as isCompilated. Which is my true point :)

Why making it final? That would be the first method in the class.

GuilhemN · 2016-08-19T21:11:09Z

src/Symfony/Component/DependencyInjection/ContainerBuilder.php

-        if ($this->isFrozen()) {
-            throw new BadMethodCallException('Cannot load from an extension on a frozen container.');
+        if ($this->isCompiled()) {
+            throw new BadMethodCallException('Cannot load from an extension on a conpiled container.');


nicolas-grekas · 2016-08-22T15:16:20Z

I tend to be sympathetic to deprecating the "frozen" vocabulary on containers. It's been looking strange to me also and the difference is unclear with the "compile" concept. isCompiled looks better.
That may enhance DX, that would be the argument supporting this change.

ro0NL · 2016-08-22T16:59:10Z

Updated the PhpDumper as well.. it makes sense, but can be unpredictable when you have
new Container<Builder>(new FrozenParameterBag())

What do we expect when dumping this uncompiled (ie. we dont call $container->compile())? Currently the behavior is the same when doning new Container<Builder>(new ParamaterBag())

ogizanagi · 2016-08-22T17:19:48Z

@ro0NL : As we do not rely anymore on the $this->parameterBag instanceof FrozenParameterBag, I'd say it doesn't matter anymore... But...you'll probably have to check if the parameterBag is frozen in the ContainerBuilder::merge() method before trying to add parameters.

nicolas-grekas · 2016-08-23T07:04:15Z

src/Symfony/Component/DependencyInjection/Container.php

        $this->parameterBag->resolve();

-        $this->parameterBag = new FrozenParameterBag($this->parameterBag->all());
+        if (!$this->parameterBag instanceof FrozenParameterBag) {


This should be removed: changing the object instance of the bag can be seen as a wanted side effect of compiling. Not wrapping a FrozenParameterBag in a FrozenParameterBag looks like an unnecessary optimization to me also.

ro0NL · 2016-08-23T08:11:14Z

@ogizanagi merging is only allowed before compiling. If you passed a FrozenParameterBag at that point i tend to let it crash (from the paramereter bag side that is).

Not sure what's best for DX here.. crashing or ignoring silently...

@nicolas-grekas any thoughts?

Other then that.. this is ready.

ro0NL · 2016-08-23T08:14:45Z

Oh and btw... just to be sure; the YAML+XML dumper really depend on a frozen bag right? Due escaping.

nicolas-grekas · 2016-08-23T13:41:47Z

Rebase needed to take #19704 into account.

allflame · 2016-08-24T08:53:21Z

@ro0NL as far as i can see isCompiled will return false for dumped container whether or not container was compiled beforehand

nicolas-grekas · 2016-08-24T09:06:23Z

I really don't know if this has a chance to be accepted by the core team, but let's say yes, I think we should go one step further and try deprecating dumping non-compiled containers.
WDYT?

ro0NL · 2016-08-24T15:20:52Z

@allflame isCompiled depends on compile(). The PHP dumper doesnt compile your container, it''s more or less designed to cover both cases, ie.

dump a compiled container(builder)
compile a dumped container(builder)

allflame · 2016-08-24T16:36:47Z

@ro0NL I'm aware of that fact. Let me explain using your own statements:
Since PhpDumper has nothing to do with the compiled status, it shouldn't change it, right?
What I'm trying to say is that if you take your code, compile the container, you will get isCompiled resolving to true (correct), but if you dump it to the file and then create the container from the dumped file, you will get isCompiled resolving to false (incorrect), which is "strange" since you were dumping compiled container.
@nicolas-grekas tbh I'm not using the whole Symfony ecosystem so I cant think of implication this change can have (potential BC issues), but what I'm pretty sure of is that if you don't compile your container the "original" and "dumped" version won't necessarily be the same.

ro0NL · 2016-08-24T17:11:19Z

@allflame sorry, you're right #19704 applies to isCompiled as well, i should fix that 👍

About BC, i truly consider this a design flaw and ~~think~~hope the impact is minimal.

ro0NL · 2016-12-20T19:23:19Z

Rebased. Still all good :)

ro0NL · 2016-12-29T19:33:12Z

I just realized i made a mistake with the latest deprecation: ed5b1d8#diff-f7b23d463cba27ac5e4cb677f2be7623R76

Im talking uncompiled container here, looking at the class it should have been not-frozen container. I really dont mind this tiny semantic. Then again, if we dont do the PR it might be worth updating that instead... sorry :)

nicolas-grekas · 2017-01-02T18:29:12Z

src/Symfony/Component/DependencyInjection/Container.php

        $this->parameterBag = new FrozenParameterBag($this->parameterBag->all());
+
+        $this->compiled = true;


I'd keep only this line a revert the previous changes in this method, that would prevent any potential BC-related side effect of the change.

nicolas-grekas · 2017-01-02T18:29:49Z

src/Symfony/Component/DependencyInjection/Container.php

     * @return bool true if the container parameter bag are frozen, false otherwise
     */
    public function isFrozen()
    {
+        @trigger_error(sprintf('The %s() method is deprecated as of 3.3 and will be removed in 4.0. Use the isCompiled() method instead.', __METHOD__), E_USER_DEPRECATED);


we use "since version 3.3" here usually I think

nicolas-grekas · 2017-01-02T18:33:13Z

src/Symfony/Component/DependencyInjection/ContainerBuilder.php

@@ -554,6 +554,10 @@ public function prependExtensionConfig($name, array $config)
  
F438
   */
    public function compile()
    {
+        if ($this->isCompiled()) {
+            return;


let's remove this change, it just opens a potential BC break to me

nicolas-grekas · 2017-01-02T18:34:03Z

👍 with suggested changes

ro0NL · 2017-01-03T16:07:06Z

Updated, rebased & waiting for travis.

Cant wait for 4.0 guys 👍

xabbuh · 2017-01-11T13:11:13Z

@ro0NL Looks like tests fail.

ro0NL · 2017-01-11T13:19:46Z

Hm.. looks fixture related, maybe something added in the meanwhile. Ill have a look tonight.

Note to self;

Symfony\Component\DependencyInjection\Tests\ContainerTest::testIsFrozen
Symfony\Component\DependencyInjection\Tests\Dumper\PhpDumperTest::testClosureProxy

nicolas-grekas · 2017-02-14T14:05:14Z

ping @ro0NL

ro0NL · 2017-02-14T14:54:57Z

Yes.. ill check it tonight. Focussed on #19668 first.. but that's more or less dealt with now.

ro0NL · 2017-02-14T19:36:04Z

All good @nicolas-grekas

fabpot · 2017-03-22T17:29:33Z

LGMT @ro0NL Can you rebase one last time? Thanks.

ro0NL · 2017-03-22T18:59:03Z

@fabpot all good.

fabpot · 2017-03-22T19:02:23Z

Thank you @ro0NL.

…piled (ro0NL) This PR was merged into the 3.3-dev branch. Discussion ---------- [DI] Deprecate Container::isFrozen and introduce isCompiled | Q | A | | --- | --- | | Branch? | "master" | | Bug fix? | no | | New feature? | yes | | BC breaks? | no | | Deprecations? | yes | | Tests pass? | yes | | Fixed tickets | comma-separated list of tickets fixed by the PR, if any | | License | MIT | | Doc PR | reference to the documentation PR, if any | This deprecates the concept of freezing a container, implied by `Container::isFrozen`. However, freezing happens due compilation (`Container::compile`). So having just `isCompiled` instead seems more intuitive, and plays along well with `ContainerBuilder`. Before/After; - `Container::isFrozen` - Checks if the parameter bag is frozen, but is deprecated in 3.2 - In 4.0 this methods does not exists and can be replaced with `getParameterBag() instanceof FrozenParameterBag` _or_ `isCompiled()`. Depending on what you want (to clarify; the behavior is different when passing a frozen bag to the constructor) - `Container::isCompiled` - Truly checks if `compile()` has ran, and is a new feature - `ContainerBuilder::merge` etc. - Now uses `isCompiled` instead of `isFrozen`, ie. we allow for it till compilation regarding the state of the paramater bag Commits ------- 6abd312 [DI] Deprecate Container::isFrozen and introduce isCompiled

Taluu · 2017-03-22T20:53:03Z

src/Symfony/Component/DependencyInjection/ContainerBuilder.php

     */
    public function set($id, $service)
    {
        $id = $this->normalizeId($id);

-        if ($this->isFrozen() && (isset($this->definitions[$id]) && !$this->definitions[$id]->isSynthetic())) {
+        if ($this->isCompiled() && (isset($this->definitions[$id]) && !$this->definitions[$id]->isSynthetic())) {
            // setting a synthetic service on a frozen container is alright
            throw new BadMethodCallException(sprintf('Setting service "%s" for an unknown or non-synthetic service definition on a frozen container is not allowed.', $id));


sprintf('Setting service "%s" for an unknown or non-synthetic service definition on a compiled container is not allowed.', $id)

basically, s/frozen/compiled, no ?

Fixed with #22112

This PR was squashed before being merged into the 4.0-dev branch (closes #22763). Discussion ---------- [DI] Remove deprecated isFrozen() | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | yes | Deprecations? | no | Tests pass? | should be | Fixed tickets | #...  | License | MIT | Doc PR | symfony/symfony-docs#...  See #19673 Test failure seems unrelated. Commits ------- 04b39a3 [DI] Remove deprecated isFrozen()

carsonbot added Status: Needs Review Feature Deprecation labels Aug 19, 2016

ro0NL changed the title ~~[DI] Deprecate Container::compile~~ [DI] Deprecate Container::isFrozen in favor of isCompiled Aug 19, 2016

ro0NL reviewed Aug 19, 2016
View reviewed changes

ro0NL changed the title ~~[DI] Deprecate Container::isFrozen in favor of isCompiled~~ [DI] Deprecate Container::isFrozen and introduce isCompiled Aug 19, 2016

ro0NL mentioned this pull request Aug 19, 2016

[DI] Deprecate Container::set for nulls, initialized and alias services #19668

Closed

GuilhemN reviewed Aug 19, 2016
View reviewed changes

nicolas-grekas reviewed Aug 23, 2016
View reviewed changes

ro0NL mentioned this pull request Aug 23, 2016

[DI] Allow getting synthetic before compilation #19715

Closed

nicolas-grekas reviewed Jan 2, 2017

View reviewed changes

xabbuh added Status: Needs Work and removed Status: Needs Review labels Jan 11, 2017

[DI] Deprecate Container::isFrozen and introduce isCompiled

6abd312

fabpot merged commit 6abd312 into symfony:master Mar 22, 2017

ro0NL deleted the di/separate-compilation branch March 22, 2017 19:03

Taluu reviewed Mar 22, 2017

View reviewed changes

nicolas-grekas modified the milestones: 3.x, 3.3 Mar 24, 2017

javiereguiluz mentioned this pull request Mar 26, 2017

[DI] Deprecate Container::isFrozen and introduce isCompiled symfony/symfony-docs#7698

Closed

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

ro0NL mentioned this pull request May 19, 2017

[DI] Remove deprecated isFrozen() #22763

Closed

		$this->parameterBag = new FrozenParameterBag($this->parameterBag->all());

		$this->compiled = true;

Uh oh!

[DI] Deprecate Container::isFrozen and introduce isCompiled #19673

[DI] Deprecate Container::isFrozen and introduce isCompiled #19673

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!