[DI] Cleanup array_key_exists #19689

ro0NL · 2016-08-21T12:10:30Z

Q	A
Branch?	2.7
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
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

Investigated this a bit, and to me it looks like left-over code. null doesnt end up in $this->services by design (this was done in #8582) so it seems. The test added then for regression still passes :)

I cant believe we guarantee BC for users doing $this->services['id'] = null (due protected), allowing them to break the design of has, get and initialized right now.

This also happens for $this->definitions in ContainerBuilder, maybe because Container did it alteady.. but im not sure.

Then again, there's this comment: #14470 (comment)

nicolas-grekas · 2016-08-22T09:43:57Z

I tend to think this could theoretically be OK , BUT practically I don't see any reasons to take the risk of breaking something given that the existing array_key_exists checks don't hurt performance nor anything else.
Thus 👎

ro0NL · 2016-08-22T11:54:13Z

What would we break? Only null is affected.. we shouldnt consider people actually relying on this quirk from our side. Imo.

ro0NL · 2016-08-22T11:56:43Z

Not cleaning up out of fear.. or not knowing about intention of code is 👎 for me.

nicolas-grekas · 2016-08-22T12:29:38Z

we shouldnt consider people actually relying on this quirk from our side.

That's where our opinions diverge. By experience, people do use extension points to do clever things we can't imagine beforehand.

ro0NL · 2016-08-22T12:41:04Z

Understand, and i know this is difficult. My point is, this was not intended/designed to be a extension point at all. We make it really hard for ourselves also this way.

This should be considered rationally, do we intend array_key_exists or not. Im really curious about that...

My opionion:

whatever the user is doing with $this->services, doesnt matter
has is designed to return true when set (hence isset)

nicolas-grekas · 2016-08-22T13:49:56Z

Re-read, removing my 👎
Should be on 2.7 btw.

ro0NL · 2016-08-22T14:04:55Z

Rebased. Diff is the same :-) I cant edit the base branch on github i guess?

stof · 2016-08-22T14:09:29Z

@ro0NL you can edit it (they deployed this feature 1 week ago)

ro0NL · 2016-08-22T14:13:42Z

Made my day. Thanks @stof

nicolas-grekas · 2016-08-22T14:53:43Z

👍 :)
Status: reviewed

stof · 2016-08-22T14:54:43Z

👍

fabpot · 2016-08-22T15:31:38Z

Indeed, null was a significant value before #8582, but it's not anymore.

fabpot · 2016-08-22T15:32:58Z

Thank you @ro0NL.

This PR was squashed before being merged into the 2.7 branch (closes #19689). Discussion ---------- [DI] Cleanup array_key_exists | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | 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 Investigated this a bit, and to me it looks like left-over code. `null` doesnt end up in `$this->services` by design (this was done in #8582) so it seems. The test added then for regression still passes :) I cant believe we guarantee BC for users doing `$this->services['id'] = null` (due protected), allowing them to break the design of `has`, `get` and `initialized` right now. This also happens for `$this->definitions` in `ContainerBuilder`, maybe because `Container` did it alteady.. but im not sure. Then again, there's this comment: #14470 (comment) Commits ------- 3306c70 [DI] Cleanup array_key_exists

dmaicher · 2016-09-04T15:32:37Z

This seems to be causing some BC breaks 😢

#19840

nicolas-grekas · 2016-09-04T15:59:03Z

See #19847

This reverts commit c89f80a, reversing changes made to 386e5e7.

…" (nicolas-grekas) This PR was merged into the 2.7 branch. Discussion ---------- Revert "minor #19689 [DI] Cleanup array_key_exists (ro0NL)" | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | Tests pass? | yes | Fixed tickets | #19689 #19840 #19825 #19857 | License | MIT | Doc PR | - This reverts commit c89f80a, reversing changes made to 386e5e7. See discussion in #19847 I'll try adding test cases soon that ensure that: - [x] *when not leaving scope* synthetic services always throw and ignore the `ContainerInterface::NULL_ON_INVALID_REFERENCE` flag (on 3.x also) - [x] *when leaving scope* synthetic services always return null and ignore the `ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE` (until 2.8 since scopes are gone in 3.x) Commits ------- 8cb28bf [DI] Add anti-regression test ac742df Revert "minor #19689 [DI] Cleanup array_key_exists (ro0NL)"

* 2.7: [FrameworkBundle] Check for class existence before is_subclass_of Update GroupSequence.php Code enhancement and cleanup [DI] Add anti-regression test Revert "minor #19689 [DI] Cleanup array_key_exists (ro0NL)" [BrowserKit] Fix cookie expiration on 32 bit systems bumped Symfony version to 2.7.18 updated VERSION for 2.7.17 update CONTRIBUTORS for 2.7.17 updated CHANGELOG for 2.7.17 Update misleading comment about RFC4627

* 2.8: [FrameworkBundle] Check for class existence before is_subclass_of Update GroupSequence.php Code enhancement and cleanup [Form] Fix transformer tests after the ICU update [DI] Add anti-regression test Revert "minor #19689 [DI] Cleanup array_key_exists (ro0NL)" bumped Symfony version to 2.8.11 updated VERSION for 2.8.10 updated CHANGELOG for 2.8.10 [BrowserKit] Fix cookie expiration on 32 bit systems bumped Symfony version to 2.7.18 updated VERSION for 2.7.17 update CONTRIBUTORS for 2.7.17 updated CHANGELOG for 2.7.17 Update misleading comment about RFC4627

* 3.1: [FrameworkBundle] Check for class existence before is_subclass_of Update GroupSequence.php Code enhancement and cleanup [Form] Fix transformer tests after the ICU update [DI] Add anti-regression test Revert "minor #19689 [DI] Cleanup array_key_exists (ro0NL)" bumped Symfony version to 3.1.5 updated VERSION for 3.1.4 updated CHANGELOG for 3.1.4 bumped Symfony version to 2.8.11 updated VERSION for 2.8.10 updated CHANGELOG for 2.8.10 [BrowserKit] Fix cookie expiration on 32 bit systems bumped Symfony version to 2.7.18 updated VERSION for 2.7.17 update CONTRIBUTORS for 2.7.17 updated CHANGELOG for 2.7.17 Update misleading comment about RFC4627

nicolas-grekas · 2016-09-06T11:09:05Z

Reverted on 2.7 & 2.8, kept on 3.*

ro0NL · 2016-09-07T07:39:19Z

@nicolas-grekas @fabpot sorry for this :-)

* 2.7: [FrameworkBundle] Check for class existence before is_subclass_of Update GroupSequence.php Code enhancement and cleanup [DI] Add anti-regression test Revert "minor symfony#19689 [DI] Cleanup array_key_exists (ro0NL)" [BrowserKit] Fix cookie expiration on 32 bit systems bumped Symfony version to 2.7.18 updated VERSION for 2.7.17 update CONTRIBUTORS for 2.7.17 updated CHANGELOG for 2.7.17 Update misleading comment about RFC4627

* 2.8: [FrameworkBundle] Check for class existence before is_subclass_of Update GroupSequence.php Code enhancement and cleanup [Form] Fix transformer tests after the ICU update [DI] Add anti-regression test Revert "minor symfony#19689 [DI] Cleanup array_key_exists (ro0NL)" bumped Symfony version to 2.8.11 updated VERSION for 2.8.10 updated CHANGELOG for 2.8.10 [BrowserKit] Fix cookie expiration on 32 bit systems bumped Symfony version to 2.7.18 updated VERSION for 2.7.17 update CONTRIBUTORS for 2.7.17 updated CHANGELOG for 2.7.17 Update misleading comment about RFC4627

* 3.1: [FrameworkBundle] Check for class existence before is_subclass_of Update GroupSequence.php Code enhancement and cleanup [Form] Fix transformer tests after the ICU update [DI] Add anti-regression test Revert "minor symfony#19689 [DI] Cleanup array_key_exists (ro0NL)" bumped Symfony version to 3.1.5 updated VERSION for 3.1.4 updated CHANGELOG for 3.1.4 bumped Symfony version to 2.8.11 updated VERSION for 2.8.10 updated CHANGELOG for 2.8.10 [BrowserKit] Fix cookie expiration on 32 bit systems bumped Symfony version to 2.7.18 updated VERSION for 2.7.17 update CONTRIBUTORS for 2.7.17 updated CHANGELOG for 2.7.17 Update misleading comment about RFC4627

carsonbot added the Status: Needs Review label Aug 21, 2016

ro0NL added 2 commits August 22, 2016 14:02

cleanup

345d521

lets see what happens for definitions

4d76ad7

ro0NL changed the base branch from master to 2.7 August 22, 2016 14:13

carsonbot added Status: Reviewed and removed Status: Needs Review labels Aug 22, 2016

fabpot closed this Aug 22, 2016

ro0NL deleted the di/cleanup-array-key-exists branch August 22, 2016 15:54

dmaicher mentioned this pull request Sep 4, 2016

"You have requested a synthetic service ("request")." after updating from 2.8.9 to 2.8.10 #19840

Closed

nicolas-grekas mentioned this pull request Sep 4, 2016

[DI] Fix returning NULL on invalid ref for synthetic services #19847

Closed

nicolas-grekas added a commit to nicolas-grekas/symfony that referenced this pull request Sep 4, 2016

Revert "minor symfony#19689 [DI] Cleanup array_key_exists (ro0NL)"

ac742df

This reverts commit c89f80a, reversing changes made to 386e5e7.

nicolas-grekas mentioned this pull request Sep 4, 2016

Revert "minor #19689 [DI] Cleanup array_key_exists (ro0NL)" #19848

Merged

2 tasks

This was referenced Sep 7, 2016

Release v2.7.18 #19876

Merged

Release v2.8.11 #19877

Merged

fabpot mentioned this pull request Oct 3, 2016

Release v3.1.5 #20144

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DI] Cleanup array_key_exists #19689

[DI] Cleanup array_key_exists #19689

ro0NL commented Aug 21, 2016 •

edited

Loading

nicolas-grekas commented Aug 22, 2016

ro0NL commented Aug 22, 2016

ro0NL commented Aug 22, 2016

nicolas-grekas commented Aug 22, 2016 •

edited

Loading

ro0NL commented Aug 22, 2016 •

edited

Loading

nicolas-grekas commented Aug 22, 2016

ro0NL commented Aug 22, 2016

stof commented Aug 22, 2016

ro0NL commented Aug 22, 2016

nicolas-grekas commented Aug 22, 2016

stof commented Aug 22, 2016

fabpot commented Aug 22, 2016

fabpot commented Aug 22, 2016

dmaicher commented Sep 4, 2016

nicolas-grekas commented Sep 4, 2016

nicolas-grekas commented Sep 6, 2016

ro0NL commented Sep 7, 2016

[DI] Cleanup array_key_exists #19689

[DI] Cleanup array_key_exists #19689

Conversation

ro0NL commented Aug 21, 2016 • edited Loading

nicolas-grekas commented Aug 22, 2016

ro0NL commented Aug 22, 2016

ro0NL commented Aug 22, 2016

nicolas-grekas commented Aug 22, 2016 • edited Loading

ro0NL commented Aug 22, 2016 • edited Loading

nicolas-grekas commented Aug 22, 2016

ro0NL commented Aug 22, 2016

stof commented Aug 22, 2016

ro0NL commented Aug 22, 2016

nicolas-grekas commented Aug 22, 2016

stof commented Aug 22, 2016

fabpot commented Aug 22, 2016

fabpot commented Aug 22, 2016

dmaicher commented Sep 4, 2016

nicolas-grekas commented Sep 4, 2016

nicolas-grekas commented Sep 6, 2016

ro0NL commented Sep 7, 2016

ro0NL commented Aug 21, 2016 •

edited

Loading

nicolas-grekas commented Aug 22, 2016 •

edited

Loading

ro0NL commented Aug 22, 2016 •

edited

Loading