[DI] Initialize properties before method calls #20566
Conversation
|
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 |
| @@ -798,6 +798,20 @@ public function testLazyLoadedService() | |||
|
|
|||
| $this->assertTrue($classInList); | |||
| } | |||
|
|
|||
| public function testInitialziePropertiesBeforeMethodCalls() | |||
There was a problem hiding this comment.
The approach is OK though?
|
Test added.. however it does not involve the |
|
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 service initialization. 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. |
This PR was squashed before being merged into the 2.7 branch (closes #20566). Discussion ---------- [DI] Initialize properties before method calls | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes-ish | New feature? | no | BC breaks? | not sure | Deprecations? | no | Tests pass? | not yet (only dumps seem to fail) | Fixed tickets | comma-separated list of tickets fixed by the PR, if any | License | MIT | Doc PR | reference to the documentation PR, if any Given ```yml services: handler: class: AppBundle\Handler properties: debug: '%kernel.debug%' calls: - [handle] ``` I totally expected `Handler::$debug` to be set before `Handler::handle` is called. It was not.. and it's really annoying :) Commits ------- 0af433b [DI] Initialize properties before method calls
|
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. |
|
To be fair, this sounds like a behavioural change more than a bug fix.
…On Fri, 25 Nov 2016, 23:01 Fabien Potencier, ***@***.***> wrote:
There is of course a big difference between "breaking" x.y.z and breaking
x.y.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#20566 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABrGNs_87LEdmzjHjx2A4uRga0N3Zb8Mks5rB1q8gaJpZM4K3Ozg>
.
|
|
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. |
Given
I totally expected
Handler::$debugto be set beforeHandler::handleis called. It was not.. and it's really annoying :)