Insert correct parameter_bag service in AbstractController #27415
Conversation
|
Can you add a test to make sure we won't have any regressions in the future? |
|
@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 |
There was a problem hiding this comment.
please remove this link, we don't put github links in the source
| * | ||
| * @see https://github.com/symfony/symfony/pull/27415 | ||
| */ | ||
| public function testSubscribedServices() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
|
Changed test as discussed with @nicolas-grekas on Slack. |
6d5b815 to
37270d7
Compare
|
Thank you @curry684. |
… (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
Reverts this feature being broken in 3051289#diff-ef10778bc68793f8c8f4b71a7ba67790R86 -
getParametercould never work now as it was querying the container itself instead of the parameter bag.Also see comments at #25439 (comment)