Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[HttpKernel] Added the SessionValueResolver#21164

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:masterfromlinaori:feature/session-avr
Feb 26, 2017
Merged

[HttpKernel] Added the SessionValueResolver#21164

fabpot merged 1 commit intosymfony:masterfromlinaori:feature/session-avr
Feb 26, 2017

Conversation

@linaori
Copy link
Contributor

@linaorilinaori commentedJan 4, 2017•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#21159
LicenseMIT
Doc PRsymfony/symfony-docs#7322

This feature adds theSessionValueResolver. That means that you no longer have to rely on injecting aSessionInterface implementation via the constructor or getting this implementation from theRequest. Regardless of method, it does not know about thegetFlashBag().

By adding theSession to the action arguments, you can now type-hint against the implementation rather than interface, which contains thegetFlashBag(), making it accessible rather than using duck-typing.

It should also feel less like injecting a service into the constructor which has a state or getting a service from the request.

Old Situation

class Controller{publicfunction__construct(SessionInterface$session) {/* ... */ }publicfunctionfooAction(Request$request)    {$this->get('session')->get(...);$request->getSession()->get(...);$this->session->get(...)// duck-typing$this->get('session')->getFlashBag();$request->getSession()->getFlashBag();$this->session->getFlashBag();$this->addFlash(...);    }}

New Situation- The controller shortcut for flashbag could in theory be removed now

class Controller{publicfunctionfooAction(Session$session)    {$session->get(...);$session->getFlashBag();    }}

chalasr, yannickl88, ogizanagi, sstok, jvasseur, yceruto, tyx, wouterj, ste93cry, patie, and gerryvdm reacted with thumbs up emojichalasr reacted with hooray emoji
@@ -0,0 +1,9 @@
<?php

Copy link

@yannickl88yannickl88Jan 4, 2017•
edited
Loading

Choose a reason for hiding this comment

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

Small somewhat unrelated sidenote, I noticed only theNullableController.php has a copyright docblock but the others do not... Most of the fixture files have them, should it also be added?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

For consistency, I can add them yes

*/
publicfunctionresolve(Request$request,ArgumentMetadata$argument)
{
yield$request->getSession();
Copy link
Contributor

Choose a reason for hiding this comment

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

The test above does not ensure that the returned value will be aSession instance, only the argument type is checked, this could return any implementation of the interface (https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/Request.php#L741), so you should add a test for it I guess.

Copy link
Contributor

Choose a reason for hiding this comment

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

you should add a test for it I guess.

I meant a unit test.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Added, seetestGetSessionArgumentsWithInterface

*/
publicfunctionsupports(Request$request,ArgumentMetadata$argument)
{
return (Session::class ===$argument->getType() ||is_subclass_of($argument->getType(), Session::class)) &&$request->hasSession();
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest:

if (Session::class !==$argument->getType() || !is_subclass_of($argument->getType(), Session::class)) {returnfalse;}return$request->getSession()instanceof Session;

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Good catch, that's what I get for staying up late and rushing it ;)

rvanlaak reacted with hooray emoji
@linaori
Copy link
ContributorAuthor

I think fabbot.io is giving false positives, I haven't touched those lines and I think they are correct the way they are

/cc@fabpot

@fabpot
Copy link
Member

Indeed, can you try to rebase to see if that fixes the issue?

@linaori
Copy link
ContributorAuthor

@fabpot

[braces] Is not configurable.

@fabpot
Copy link
Member

Opps, I thought thatPHP-CS-Fixer/PHP-CS-Fixer#2421 was already merged but I was wrong :(

*/
publicfunctionsupports(Request$request,ArgumentMetadata$argument)
{
if (Session::class !==$argument->getType() && !is_subclass_of($argument->getType(), Session::class)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we check on SessionInterface to allow type-hinting with the interface when we only need methods that are in the interface ?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I explicitly mentionedSession overSessionInterface here due to statefulness, which is implementation specific. I could change it to the interface of course.

What do you guys think of this?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should support both as we also maintain both. TheSession type hint would be useful if you really need the flash bag, for example. Using theSessionInterface is great for interoperability especially in third-party bundles.

rvanlaak reacted with thumbs up emoji
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

Copy link
Member

Choose a reason for hiding this comment

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

These changes should probably better be introduced on older branches.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I'll put them on 3.1 in that case

@javiereguiluz
Copy link
Member

For now I'm 👎 about this proposal. My reasons:

  • The fact that this makes easier to get the flashBag() is irrelevant to me. 99% of the times you don't have to get the FlashBag, you just add new flash messages. That's why we have$this->addFlash()
  • Currently, getting the session is simple ($request->getSession()) so there isn't much to optimize there (unlike other parts such as Security, which could be heavily optimize).
  • It's not a very technical reason ... but to me it doesn't feel natural to get the Session in that way in Symfony. We all know that this is a Request + Response framework ... so I expect controller to get the Request and the routing parameters. Using theSession $session typehint looks "non-native". If I get this feature, why not addDoctrine $em as a valid typehint to get the entity manager too?

@linaori
Copy link
ContributorAuthor

The fact that this makes easier to get the flashBag() is irrelevant to me. 99% of the times you don't have to get the FlashBag, you just add new flash messages. That's why we have $this->addFlash()

Except that I don't extend the base controller, so this is of no use for me. Everyone using Controller as a Service will run into this problem.

Currently, getting the session is simple ($request->getSession()) so there isn't much to optimize there (unlike other parts such as Security, which could be heavily optimize).

It's not about optimization, but about type-hints (see#21159, it lists all issues related).getFlashBag() can only be received by duck-typing.

It's not a very technical reason ... but to me it doesn't feel natural to get the Session in that way in Symfony. We all know that this is a Request + Response framework ... so I expect controller to get the Request and the routing parameters. Using the Session $session typehint looks "non-native". If I get this feature, why not add Doctrine $em as a valid typehint to get the entity manager too?

If you ask me:

  • Constructor stateless dependencies, e.g. services, things that are the same no matter what action is called.
  • Action stateful dependencies, e.g. entities, parameters or other objects that can have different states each time it gets called.

The session can be different each time it's called, therefore it shouldn't be a constructor dependency. However, when using theRequest::getSession(), you don't have access togetFlashBag(), which isSession specific.

why not add Doctrine $em as a valid typehint to get the entity manager too?

Doctrine/The EM is a stateless dependency and not an object with contextual value.

nicolas-grekas, jvasseur, chalasr, wouterj, sstok, and MatthiasMolitor reacted with thumbs up emoji

nicolas-grekas added a commit that referenced this pull requestJan 6, 2017
This PR was merged into the 3.1 branch.Discussion----------Added missing headers in fixture files| Q             | A| ------------- | ---| Branch?       | 3.1| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | n/a| Fixed tickets | ~| License       | MIT| Doc PR        | ~The headers were missing in a few files as mentioned in#21164 (comment)Commits-------c9c2474 Added missing headers in fixture files
returnfalse;
}

return$request->hasSession();
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should check that the session is of the type-hinted class ($request->getSession() instanceof $argument->getType())?

HeahDude reacted with thumbs up emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

The return type ofgetSession() isSessionInterface. The AVR supports everything extending theSessionInterface, thus merely checking if ithas as session, is enough imo

Copy link
Contributor

Choose a reason for hiding this comment

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

But if someone type hint withSession and that$request->getSession() doesn't return aSession but another class implementingSessionInterface we will get a type error instead of the "no argument value resolver" error.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Right! I'll add a test case

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

@jvasseur should be fixed now

@lyrixx
Copy link
Member

I like this proposal. It's a step toward#10557

*/
publicfunctionsupports(Request$request,ArgumentMetadata$argument)
{
if (SessionInterface::class !==$argument->getType() && !is_subclass_of($argument->getType(), SessionInterface::class)) {
Copy link
Contributor

@HeahDudeHeahDudeJan 7, 2017•
edited
Loading

Choose a reason for hiding this comment

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

If you want to support both type hints (implementation and interface), what about:

$type =$argument->getType();if (SessionInterface::class !==$type && !is_subclass_of($type, SessionInterface::class)) {returnfalse;}return$request->getSession()instanceof$type;

?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Hm yeah, I was playing with that idea earlier, but I wasn't sure if instanceof works with that (at least not with a method call), let's see!

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Yup, that works a lot cleaner!

HeahDude reacted with hooray emoji
Copy link
Contributor

@HeahDudeHeahDude left a comment

Choose a reason for hiding this comment

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

👍

@umulmrum
Copy link
Contributor

Just stumbled across this PR and would like to support it 👍

It currently feels quite wrong to use the session flashbag when using controllers as services instead of extending the base controller (I regard container injection as anti-pattern, even in controllers).

bestform reacted with thumbs up emoji

@linaori
Copy link
ContributorAuthor

Rebased against the master, all is green now

Copy link
Member

@chalasrchalasr left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@xabbuhxabbuh left a comment

Choose a reason for hiding this comment

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

👍

Status: Reviewed

@fabpot
Copy link
Member

Thank you@iltar.

HeahDude reacted with hooray emoji

@fabpotfabpot merged commitb4464dc intosymfony:masterFeb 26, 2017
fabpot added a commit that referenced this pull requestFeb 26, 2017
This PR was merged into the 3.3-dev branch.Discussion----------[HttpKernel] Added the SessionValueResolver| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#21159| License       | MIT| Doc PR        | (soon)This feature adds the `SessionValueResolver`. That means that you no longer have to rely on injecting a `SessionInterface` implementation via the constructor or getting this implementation from the `Request`. Regardless of method, it does not know about the `getFlashBag()`.By adding the `Session` to the action arguments, you can now type-hint against the implementation rather than interface, which contains the `getFlashBag()`, making it accessible rather than using duck-typing._It should also feel less like injecting a service into the constructor which has a state or getting a service from the request._**Old Situation**```phpclass Controller{    public function __construct(SessionInterface $session) { /* ... */ }    public function fooAction(Request $request)    {        $this->get('session')->get(...);        $request->getSession()->get(...);        $this->session->get(...)        // duck-typing        $this->get('session')->getFlashBag();        $request->getSession()->getFlashBag();        $this->session->getFlashBag();        $this->addFlash(...);    }}```**New Situation** _- The controller shortcut for flashbag could in theory be removed now_```phpclass Controller{    public function fooAction(Session $session)    {        $session->get(...);        $session->getFlashBag();    }}```Commits-------b4464dc Added the SessionValueResolver
nicolas-grekas added a commit that referenced this pull requestFeb 27, 2017
This PR was merged into the 3.3-dev branch.Discussion----------[HttpKernel] Refactored SessionValueResolver| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | ~| License       | MIT| Doc PR        | ~I thought the comment has been addressed in#21164, but it may have been unintentionally lost while rebasing?Commits-------f0e832a [HttpKernel] Refactored SessionValueResolver
@nicolas-grekasnicolas-grekas modified the milestones:3.x,3.3Mar 24, 2017
xabbuh added a commit to symfony/symfony-docs that referenced this pull requestApr 15, 2017
This PR was squashed before being merged into the master branch (closes#7322).Discussion----------Added docs for the SessionValueResolverExplains the functionality added insymfony/symfony#21164Commits-------88b740e Added docs for the SessionValueResolver
@fabpotfabpot mentioned this pull requestMay 1, 2017
@linaorilinaori deleted the feature/session-avr branchFebruary 8, 2019 13:38
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@xabbuhxabbuhxabbuh approved these changes

@chalasrchalasrchalasr approved these changes

+3 more reviewers

@jvasseurjvasseurjvasseur left review comments

@yannickl88yannickl88yannickl88 left review comments

@HeahDudeHeahDudeHeahDude approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

3.3

Development

Successfully merging this pull request may close these issues.

12 participants

@linaori@fabpot@javiereguiluz@lyrixx@umulmrum@jvasseur@xabbuh@yannickl88@chalasr@HeahDude@nicolas-grekas@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp