The change makes sense to me.

Isn't this a case where a configurator would make more sense? (though I agree with the fix)

It's really simple (property) "configuration" (only initializing) :) i'd prefer this approach working out-of-the-box instead of setting up a configurator class, etc.

Tests are broken (the dumped containers should be updated).

@fabpot green (ignore fabbot.io)

Now, the question is wether we need to merge this in 2.7 or 3.2. I would vote for 3.2.

To me, this looks like a bugfix and should be merged into 2.7. Merging this into 3.2 also has the drawback that the behaviour does change between supported versions which might be needless additional work for maintainers who need this feature for 2.x and 3.x.

Would it be possible to have a test case that shows why this ordering is important? The fixture update doesn't qualify as such to me (too easy to update them without considering the actual side effect).

Ping @symfony/deciders , we need to make a decision on this one before the end of the week for 3.2 to be ready (1/3).

👍 for merging this into 2.7 and if possible in any way with a test like suggested by@nicolas-grekas

Ill add the test somewhere tonight 👍 this is about asserting a property is initialized within a method call right?

@ro0NL yes

Test added.. however it does not involve thePhpDumper i realize now :) It covers onlyContainerBuilder 🤔 Should we add a similar test there as well?

Let's be safe :)

Speaking about.. what kind of case could we break in real life apps on 2.7? As it's only affected during serviceinitialization. Imo. it qualifies a bugfix.

Considering the impact of bug fixes like#20562, it might be safer to change the behavior only in 3.2. But I'm fine either way.

I have the same kind of thought than@Tobion . Yet,if this happens to break something, breaking 3.2 is not better than breaking 2.7. For this reason, and also because I fail to see where this can break anything reasonable, I'm 👍 for 2.7.

The only case we could break is when you have a method call changing behavior when the property is initialized or no, and initializing the property as a public property. This case looks quite insane though (and if the initialization relies on this to be working, the class is quite unusable directly).

So I'm also 👍 for 2.7 as it allows methods to rely on the property values.

Good catch, thanks@ro0NL.

That's yet again a very wrong argument: making such a change in 2.7.x is very different from making the change in 2.x. Also because we can document the change in the UPGRADE file. I would not have merged it in 2.7 for that reason. We are doing too much of these, we should stop. We had problems in the past and it looks like we are not learning from them.

May I add that it's not much better to break 2/3.x than it is to break 2.7.x. What I mean is than when we change a behavior, we must force ourselves to find a way to first deprecate the old one. Thus, this will naturally make such PRs move to master (or be rejected). Learning from past mistakes here IMHO means being stricter to what we consider a behavioral change and what we don't (not arguing about this specific PR thought.)

There is of course a big difference between "breaking" x.y.z and breaking x.y.

Imo. having different behavior cross-version should be avoided as much as possible. I still think the impact minimal, as@stof mentioned.. people are doing really weird things then (if they notice at all).

However, there are also things like EOM/EOL....

And the line is really thin here.. yes it's a behavior change, yet pragmatically, it qualifies a bugfix (imo) but technically it's a new feature for sure.