Skip to content

Add ControllerTrait::getParameter() #25439

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 11, 2017
Merged

Add ControllerTrait::getParameter() #25439

merged 1 commit into from
Dec 11, 2017

Conversation

chalasr
Copy link
Member

@chalasr chalasr commented Dec 11, 2017

Q A
Branch? 4.1
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #25288 (comment)
License MIT
Doc PR n/a

@chalasr chalasr added this to the 4.1 milestone Dec 11, 2017
@chalasr chalasr force-pushed the abstract_ctrl_get_parameter branch from 96d41b7 to 54f063e Compare December 11, 2017 15:57
stof
stof previously requested changes Dec 11, 2017
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/Tests/Controller/ControllerTraitTest.php Outdated
return;
}

if (method_exists($this, 'expectException')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove this check for new code. As Symfony 4 requires PHP 7.1+, it will not need to run tests on PHPUnit 4.8 (which is needed for PHP 5.5 and older). So expectException can always be used.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, we have a bunch of similar checks to remove https://github.com/symfony/symfony/search?utf8=%E2%9C%93&q=setExpectedException&type=

@chalasr chalasr force-pushed the abstract_ctrl_get_parameter branch from 54f063e to 799bc10 Compare December 11, 2017 16:02
@chalasr chalasr force-pushed the abstract_ctrl_get_parameter branch 2 times, most recently from 8d047c9 to 2262186 Compare December 11, 2017 16:12
@nicolas-grekas
Copy link
Member

should we add an hasParameter also?

@chalasr
Copy link
Member Author

chalasr commented Dec 11, 2017

@nicolas-grekas Controller does not have it (little value in userland?) but no objection for me

nicolas-grekas
nicolas-grekas reviewed Dec 11, 2017
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/Controller/ControllerTrait.php Outdated
*
* @return mixed
*
* @final since version 4.1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just @final (since always - it didn't exist before)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

nicolas-grekas
nicolas-grekas approved these changes Dec 11, 2017
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok without hasParameter, since it doesn't exist on Controller

@chalasr chalasr force-pushed the abstract_ctrl_get_parameter branch from 2262186 to 28397e5 Compare December 11, 2017 16:27
@fabpot
Copy link
Member

fabpot commented Dec 11, 2017

Thank you @chalasr.

@fabpot fabpot merged commit 28397e5 into symfony:master Dec 11, 2017
fabpot added a commit that referenced this pull request Dec 11, 2017
@fabpot
feature #25439 Add ControllerTrait::getParameter() (chalasr)
2c976f7 
This PR was merged into the 4.1-dev branch.

Discussion
----------

Add ControllerTrait::getParameter()

| Q             | A
| ------------- | ---
| Branch?       | 4.1
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #25288 (comment)
| License       | MIT
| Doc PR        | n/a

Commits
-------

28397e5 Add ControllerTrait::getParameter()
@chalasr chalasr deleted the abstract_ctrl_get_parameter branch December 11, 2017 22:20
@weaverryan weaverryan mentioned this pull request Jan 7, 2018
Closed
@fabpot fabpot mentioned this pull request May 7, 2018
Merged
@curry684
Copy link
Contributor

I just tried using this because of today's blog post and it seems quite fundamentally broken?

The parameter_bag is injected as ContainerInterface::class in AbstractController, after which getParameter tries to return $this->container->get('parameter_bag')->get($name), which obviously always fails with a ServiceNotFoundException as it's querying a service instead of the parameters....

@curry684
Copy link
Contributor

Oh it got screwed over later at 3051289#diff-ef10778bc68793f8c8f4b71a7ba67790R86 while refactoring, @nicolas-grekas missed 3 characters in typing it hehe.

@curry684 curry684 mentioned this pull request May 29, 2018
Merged
nicolas-grekas added a commit that referenced this pull request May 30, 2018
@nicolas-grekas
bug #27415 Insert correct parameter_bag service in AbstractController…
ce616bf 
… (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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@stof stof stof left review comments

Assignees
No one assigned
Labels
Feature FrameworkBundle Status: Reviewed
Projects
None yet
Milestone
4.1
Development

Successfully merging this pull request may close these issues.
6 participants
@chalasr @nicolas-grekas @fabpot @curry684 @stof @carsonbot