[HttpKernel] Added the SessionValueResolver #21164

linaori · 2017-01-04T21:27:14Z

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	symfony/symfony-docs#7322

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

class 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

class Controller
{
    public function fooAction(Session $session)
    {
        $session->get(...);
        $session->getFlashBag();
    }
}

yannickl88 · 2017-01-04T21:35:28Z

src/Symfony/Component/HttpKernel/Tests/Fixtures/Controller/ExtendingSession.php

@@ -0,0 +1,9 @@
+<?php
+


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

For consistency, I can add them yes

HeahDude · 2017-01-04T22:01:45Z

src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/SessionValueResolver.php

+     */
+    public function resolve(Request $request, ArgumentMetadata $argument)
+    {
+        yield $request->getSession();


The test above does not ensure that the returned value will be a Session 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.

you should add a test for it I guess.

I meant a unit test.

Added, see testGetSessionArgumentsWithInterface

HeahDude · 2017-01-04T22:08:22Z

src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/SessionValueResolver.php

+     */
+    public function supports(Request $request, ArgumentMetadata $argument)
+    {
+        return (Session::class === $argument->getType() || is_subclass_of($argument->getType(), Session::class)) && $request->hasSession();


I suggest:

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

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

linaori · 2017-01-05T18:42:47Z

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 · 2017-01-05T18:44:14Z

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

linaori · 2017-01-05T18:46:35Z

@fabpot

[braces] Is not configurable.

fabpot · 2017-01-05T18:52:40Z

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

jvasseur · 2017-01-05T20:18:44Z

src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/SessionValueResolver.php

+     */
+    public function supports(Request $request, ArgumentMetadata $argument)
+    {
+        if (Session::class !== $argument->getType() && !is_subclass_of($argument->getType(), Session::class)) {


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

I explicitly mentioned Session over SessionInterface here due to statefulness, which is implementation specific. I could change it to the interface of course.

What do you guys think of this?

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

xabbuh · 2017-01-06T05:37:57Z

src/Symfony/Component/HttpKernel/Tests/Fixtures/Controller/BasicTypesController.php

+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+


These changes should probably better be introduced on older branches.

I'll put them on 3.1 in that case

javiereguiluz · 2017-01-06T08:26:22Z

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 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?

linaori · 2017-01-06T08:39:15Z

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 the Request::getSession(), you don't have access to getFlashBag(), which is Session 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.

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

jvasseur · 2017-01-06T10:22:55Z

src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/SessionValueResolver.php

+            return false;
+        }
+
+        return $request->hasSession();


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

The return type of getSession() is SessionInterface. The AVR supports everything extending the SessionInterface, thus merely checking if it has as session, is enough imo

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

Right! I'll add a test case

@jvasseur should be fixed now

lyrixx · 2017-01-06T10:38:14Z

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

HeahDude · 2017-01-07T22:22:13Z

src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/SessionValueResolver.php

+     */
+    public function supports(Request $request, ArgumentMetadata $argument)
+    {
+        if (SessionInterface::class !== $argument->getType() && !is_subclass_of($argument->getType(), SessionInterface::class)) {


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)) { return false; } return $request->getSession() instanceof $type;

?

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!

Yup, that works a lot cleaner!

HeahDude

👍

umulmrum · 2017-02-22T08:18:34Z

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).

linaori · 2017-02-22T09:12:45Z

Rebased against the master, all is green now

chalasr

👍

xabbuh

👍

Status: Reviewed

fabpot · 2017-02-26T22:45:10Z

Thank you @iltar.

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** ```php class 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_ ```php class Controller { public function fooAction(Session $session) { $session->get(...); $session->getFlashBag(); } } ``` Commits ------- b4464dc Added the SessionValueResolver

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

This PR was squashed before being merged into the master branch (closes #7322). Discussion ---------- Added docs for the SessionValueResolver Explains the functionality added in symfony/symfony#21164 Commits ------- 88b740e Added docs for the SessionValueResolver

carsonbot added Status: Needs Review HttpKernel Feature labels Jan 4, 2017

yannickl88 reviewed Jan 4, 2017

View reviewed changes

HeahDude suggested changes Jan 4, 2017

View reviewed changes

HeahDude reviewed Jan 4, 2017

View reviewed changes

jvasseur reviewed Jan 5, 2017

View reviewed changes

xabbuh reviewed Jan 6, 2017

View reviewed changes

linaori mentioned this pull request Jan 6, 2017

Added missing headers in fixture files #21171

Merged

rvanlaak mentioned this pull request Jan 6, 2017

[RFC] ArgumentValueResolver for the Session (or Flashbag) #21159

Closed

jvasseur reviewed Jan 6, 2017

View reviewed changes

linaori mentioned this pull request Jan 6, 2017

Added docs for the SessionValueResolver symfony/symfony-docs#7322

Closed

nicolas-grekas added this to the 3.x milestone Jan 6, 2017

HeahDude reviewed Jan 7, 2017

View reviewed changes

HeahDude approved these changes Jan 7, 2017

View reviewed changes

Added the SessionValueResolver

b4464dc

chalasr approved these changes Feb 24, 2017

View reviewed changes

xabbuh approved these changes Feb 26, 2017

View reviewed changes

fabpot merged commit b4464dc into symfony:master Feb 26, 2017

HeahDude mentioned this pull request Feb 27, 2017

[HttpKernel] Refactored SessionValueResolver #21773

Merged

linaori mentioned this pull request Feb 27, 2017

[FrameworkBundle] Add new "controller.service_arguments" tag to inject services into actions #21771

Merged

3 tasks

nicolas-grekas modified the milestones: 3.x, 3.3 Mar 24, 2017

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

linaori deleted the feature/session-avr branch February 8, 2019 13:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HttpKernel] Added the SessionValueResolver #21164

[HttpKernel] Added the SessionValueResolver #21164

linaori commented Jan 4, 2017 •

edited

Loading

yannickl88 Jan 4, 2017 •

edited

Loading

linaori Jan 4, 2017

HeahDude Jan 4, 2017

HeahDude Jan 4, 2017

linaori Jan 5, 2017

HeahDude Jan 4, 2017

linaori Jan 5, 2017

linaori commented Jan 5, 2017

fabpot commented Jan 5, 2017

linaori commented Jan 5, 2017

fabpot commented Jan 5, 2017

jvasseur Jan 5, 2017

linaori Jan 5, 2017

xabbuh Jan 6, 2017

xabbuh Jan 6, 2017

linaori Jan 6, 2017

javiereguiluz commented Jan 6, 2017

linaori commented Jan 6, 2017

jvasseur Jan 6, 2017

linaori Jan 6, 2017

jvasseur Jan 6, 2017

linaori Jan 6, 2017

linaori Jan 6, 2017

lyrixx commented Jan 6, 2017

HeahDude Jan 7, 2017 •

edited

Loading

linaori Jan 7, 2017

linaori Jan 7, 2017

HeahDude left a comment

umulmrum commented Feb 22, 2017

linaori commented Feb 22, 2017

chalasr left a comment

xabbuh left a comment

fabpot commented Feb 26, 2017

[HttpKernel] Added the SessionValueResolver #21164

[HttpKernel] Added the SessionValueResolver #21164

Conversation

linaori commented Jan 4, 2017 • edited Loading

yannickl88 Jan 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linaori commented Jan 5, 2017

fabpot commented Jan 5, 2017

linaori commented Jan 5, 2017

fabpot commented Jan 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiereguiluz commented Jan 6, 2017

linaori commented Jan 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyrixx commented Jan 6, 2017

HeahDude Jan 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HeahDude left a comment

Choose a reason for hiding this comment

umulmrum commented Feb 22, 2017

linaori commented Feb 22, 2017

chalasr left a comment

Choose a reason for hiding this comment

xabbuh left a comment

Choose a reason for hiding this comment

fabpot commented Feb 26, 2017

linaori commented Jan 4, 2017 •

edited

Loading

yannickl88 Jan 4, 2017 •

edited

Loading

HeahDude Jan 7, 2017 •

edited

Loading