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

Add ControllerTrait::getParameter() #25439

Merged
merged 1 commit into from Dec 11, 2017

Conversation

Projects
None yet
6 participants
@chalasr
Member

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
return;
}
if (method_exists($this, 'expectException')) {

This comment has been minimized.

@stof

stof Dec 11, 2017

Member

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.

This comment has been minimized.

@chalasr
@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 11, 2017

should we add an hasParameter also?

@chalasr

This comment has been minimized.

Member

chalasr commented Dec 11, 2017

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

*
* @return mixed
*
* @final since version 4.1

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 11, 2017

Member

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

This comment has been minimized.

@chalasr

chalasr Dec 11, 2017

Member

fixed

@nicolas-grekas

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

@fabpot

fabpot approved these changes Dec 11, 2017

@fabpot

This comment has been minimized.

Member

fabpot commented Dec 11, 2017

Thank you @chalasr.

@fabpot fabpot merged commit 28397e5 into symfony:master Dec 11, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Dec 11, 2017

feature #25439 Add ControllerTrait::getParameter() (chalasr)
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 chalasr:abstract_ctrl_get_parameter branch Dec 11, 2017

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

@curry684

This comment has been minimized.

Contributor

curry684 commented May 29, 2018

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

This comment has been minimized.

Contributor

curry684 commented May 29, 2018

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

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment