New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Insert correct parameter_bag service in AbstractController #27415

Merged
merged 1 commit into from May 30, 2018

Conversation

Projects
None yet
5 participants
@curry684
Contributor

curry684 commented May 29, 2018

Reverts this feature being broken in 3051289#diff-ef10778bc68793f8c8f4b71a7ba67790R86 - getParameter could never work now as it was querying the container itself instead of the parameter bag.

Q A
Branch? 4.1
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
License MIT

Also see comments at #25439 (comment)

@fabpot

fabpot approved these changes May 30, 2018

@fabpot

This comment has been minimized.

Member

fabpot commented May 30, 2018

Can you add a test to make sure we won't have any regressions in the future?

@curry684

This comment has been minimized.

Contributor

curry684 commented May 30, 2018

@fabpot added a pure regression guard on all subscribed services.

/**
* This test protects the default subscribed core services against accidental modification.
*
* @see https://github.com/symfony/symfony/pull/27415

This comment has been minimized.

@nicolas-grekas

nicolas-grekas May 30, 2018

Member

please remove this link, we don't put github links in the source

*
* @see https://github.com/symfony/symfony/pull/27415
*/
public function testSubscribedServices()

This comment has been minimized.

@nicolas-grekas

nicolas-grekas May 30, 2018

Member

This doesn't really prevent anything, as it just duplicates the method.
I've no better idea, except maybe an integration test that would rely on autowiring+calling getParameter.
Worth it? As is right now, I'd prefer removing the current test case.

This comment has been minimized.

@curry684

curry684 May 30, 2018

Contributor

I kinda had the same issue - this fix has to go out today and I don't have time to write a really complex test for it.

I do not agree however it doesn't help at all and should be removed. Most specifically it would have prevented the mistake you made in the first place. The test itself may be dumb but it prevents BC breaks in removing or renaming core services that are depended upon, and changing their type. For example if someone changes the use for ContainerInterface in AbstractController to the PSR version this would catch it. I also specifically wrote it like this instead of an array compare to allow adding new subscriptions without failing. Unit tests are meant to prevent regressions and breaking existing code, without limiting new features. That's what it does. In a dumb way, but it works :P

This comment has been minimized.

@nicolas-grekas

nicolas-grekas May 30, 2018

Member

it would have prevented the mistake you made

I'm not sure, because I would have changed the test accordingly, as that was not a typo buy my intention to target ContainerInterface. What I missed was that this broke autowiring...

Anyway, OK for keeping the test as is.

@nicolas-grekas nicolas-grekas added this to the 4.1 milestone May 30, 2018

@curry684

This comment has been minimized.

Contributor

curry684 commented May 30, 2018

Changed test as discussed with @nicolas-grekas on Slack.

@nicolas-grekas nicolas-grekas changed the base branch from master to 4.1 May 30, 2018

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented May 30, 2018

Thank you @curry684.

@nicolas-grekas nicolas-grekas merged commit 37270d7 into symfony:4.1 May 30, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request May 30, 2018

bug #27415 Insert correct parameter_bag service in AbstractController…
… (curry684)

This PR was submitted for the master branch but it was squashed and merged into the 4.1 branch instead (closes #27415).

Discussion
----------

Insert correct parameter_bag service in AbstractController

Reverts this feature being broken in 3051289#diff-ef10778bc68793f8c8f4b71a7ba67790R86 - `getParameter` could never work now as it was querying the container itself instead of the parameter bag.

| Q             | A
| ------------- | ---
| Branch?       | 4.1
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| License       | MIT

Also see comments at #25439 (comment)

Commits
-------

37270d7 Insert correct parameter_bag service in AbstractController

@curry684 curry684 deleted the curry684:fix-getparameter branch May 30, 2018

@fabpot fabpot referenced this pull request May 30, 2018

Merged

Release v4.1.0 #27424

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment