[DI] Initialize properties before method calls #20566

ro0NL · 2016-11-19T11:30:30Z

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

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 :)

Tobion · 2016-11-19T12:49:34Z

The change makes sense to me.

linaori · 2016-11-19T17:32:17Z

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

ro0NL · 2016-11-19T17:45:17Z

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.

fabpot · 2016-11-22T00:05:53Z

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

ro0NL · 2016-11-22T20:19:55Z

@fabpot green (ignore fabbot.io)

fabpot · 2016-11-24T01:10:00Z

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

xabbuh · 2016-11-24T08:00:58Z

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.

nicolas-grekas · 2016-11-24T08:05:48Z

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).

nicolas-grekas · 2016-11-24T13:57:04Z

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).

xabbuh · 2016-11-24T14:25:55Z

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

ro0NL · 2016-11-24T14:27:20Z

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

stof · 2016-11-24T14:29:37Z

@ro0NL yes

nicolas-grekas · 2016-11-24T17:49:43Z

src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php

@@ -798,6 +798,20 @@ public function testLazyLoadedService()

        $this->assertTrue($classInList);
    }
+
+    public function testInitialziePropertiesBeforeMethodCalls()


typo: Initialize

The approach is OK though?

looks good yes

ro0NL · 2016-11-24T17:51:29Z

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

ro0NL · 2016-11-24T18:09:39Z

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.

Tobion · 2016-11-25T00:20:24Z

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.

nicolas-grekas · 2016-11-25T09:05:59Z

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.

stof · 2016-11-25T09:34:21Z

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.

nicolas-grekas · 2016-11-25T09:44:49Z

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

fabpot · 2016-11-25T20:20:50Z

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.

nicolas-grekas · 2016-11-25T21:57:36Z

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.)

fabpot · 2016-11-25T22:01:18Z

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

linaori · 2016-11-25T23:09:39Z

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> .

ro0NL · 2016-11-26T07:47:52Z

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.

carsonbot added Status: Needs Review Bug labels Nov 19, 2016

ro0NL changed the base branch from master to 2.7 November 20, 2016 09:01

[DI] Initialize properties before method calls

a9f4d00

fabpot added this to the 3.2 milestone Nov 24, 2016

nicolas-grekas modified the milestone: 3.2 Nov 24, 2016

nicolas-grekas reviewed Nov 24, 2016

View reviewed changes

ro0NL added 2 commits November 24, 2016 17:53

test added

1eb2065

phpdumper test

9eafc0f

nicolas-grekas closed this Nov 25, 2016

ro0NL deleted the di/props-before-calls branch November 25, 2016 09:50

fabpot mentioned this pull request Nov 27, 2016

Release v3.2.0-RC2 #20649

Merged

This was referenced Dec 13, 2016

Release v2.7.22 #20890

Merged

Release v2.8.15 #20892

Merged

Release v3.1.8 #20895

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DI] Initialize properties before method calls #20566

[DI] Initialize properties before method calls #20566

ro0NL commented Nov 19, 2016 •

edited

Loading

Tobion commented Nov 19, 2016

linaori commented Nov 19, 2016

ro0NL commented Nov 19, 2016 •

edited

Loading

fabpot commented Nov 22, 2016

ro0NL commented Nov 22, 2016

fabpot commented Nov 24, 2016

xabbuh commented Nov 24, 2016

nicolas-grekas commented Nov 24, 2016

nicolas-grekas commented Nov 24, 2016

xabbuh commented Nov 24, 2016

ro0NL commented Nov 24, 2016

stof commented Nov 24, 2016

nicolas-grekas Nov 24, 2016

ro0NL Nov 24, 2016

nicolas-grekas Nov 24, 2016

ro0NL commented Nov 24, 2016

ro0NL commented Nov 24, 2016

Tobion commented Nov 25, 2016

nicolas-grekas commented Nov 25, 2016

stof commented Nov 25, 2016

nicolas-grekas commented Nov 25, 2016

fabpot commented Nov 25, 2016

nicolas-grekas commented Nov 25, 2016

fabpot commented Nov 25, 2016

linaori commented Nov 25, 2016 via email

ro0NL commented Nov 26, 2016

[DI] Initialize properties before method calls #20566

[DI] Initialize properties before method calls #20566

Conversation

ro0NL commented Nov 19, 2016 • edited Loading

Tobion commented Nov 19, 2016

linaori commented Nov 19, 2016

ro0NL commented Nov 19, 2016 • edited Loading

fabpot commented Nov 22, 2016

ro0NL commented Nov 22, 2016

fabpot commented Nov 24, 2016

xabbuh commented Nov 24, 2016

nicolas-grekas commented Nov 24, 2016

nicolas-grekas commented Nov 24, 2016

xabbuh commented Nov 24, 2016

ro0NL commented Nov 24, 2016

stof commented Nov 24, 2016

nicolas-grekas Nov 24, 2016

Choose a reason for hiding this comment

ro0NL Nov 24, 2016

Choose a reason for hiding this comment

nicolas-grekas Nov 24, 2016

Choose a reason for hiding this comment

ro0NL commented Nov 24, 2016

ro0NL commented Nov 24, 2016

Tobion commented Nov 25, 2016

nicolas-grekas commented Nov 25, 2016

stof commented Nov 25, 2016

nicolas-grekas commented Nov 25, 2016

fabpot commented Nov 25, 2016

nicolas-grekas commented Nov 25, 2016

fabpot commented Nov 25, 2016

linaori commented Nov 25, 2016 via email

ro0NL commented Nov 26, 2016

ro0NL commented Nov 19, 2016 •

edited

Loading

ro0NL commented Nov 19, 2016 •

edited

Loading