Skip to content

Added docs for the SessionValueResolver #7322

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

Closed
wants to merge 2 commits into from

Conversation

linaori
Copy link
Contributor

@linaori linaori commented Jan 6, 2017

Explains the functionality added in symfony/symfony#21164

@xabbuh xabbuh added this to the 3.3 milestone Jan 8, 2017
@xabbuh xabbuh added the On hold label Jan 8, 2017
controller.rst Outdated

use Symfony\Component\HttpFoundation\Request;
.. versionadded:: 3.3
The ability to request a ``Session`` in actions was introduced in Symfony 3.
Copy link
Contributor

Choose a reason for hiding this comment

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

3.3

@linaori
Copy link
Contributor Author

linaori commented Feb 27, 2017

Rebased and addressed the remark. @xabbuh I forgot to link to this PR in the feature PR, but now that it's merged, the On hold can be removed.

@xabbuh xabbuh removed the On hold label Feb 27, 2017
controller.rst Outdated

public function indexAction(Session $session)
Copy link
Member

Choose a reason for hiding this comment

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

What about using SessionInterface instead?

controller.rst Outdated
public function indexAction(Request $request)
{
$session = $request->getSession();
To retrieve the session, add the :class:`Symfony\\Component\\HttpFoundation\\Session\\Session`
Copy link
Contributor

Choose a reason for hiding this comment

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

What about:
"To retrieve the session, type hint a controller argument with :class:Symfony\\Component\\HttpFoundation\\Session\\SessionInterface or any implementation:"
?
Then keep the example below which is much more useful to get access to method not available through the interface.

@linaori linaori force-pushed the feature/session-avr branch from c1be8a1 to 3be2287 Compare March 2, 2017 10:25
@linaori
Copy link
Contributor Author

linaori commented Mar 2, 2017

I've addressed the comments and added a small example of how to use the Session instead of the SessionInterface for getFlashBag

Copy link
Contributor

@HeahDude HeahDude left a comment

Choose a reason for hiding this comment

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

👍

@HeahDude
Copy link
Contributor

HeahDude commented Mar 2, 2017

This one is not on hold anymore, symfony/symfony#21164 is merged.

Copy link
Member

@wouterj wouterj left a comment

Choose a reason for hiding this comment

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

Thanks! Left 2 minor comments that can be applied during the merge.

Every ``SessionInterface`` implementation is supported. If you have your
own implementation, type-hint this in the arguments instead.

As a developer, you might prefer not to extend the ``Controller``. To use the
Copy link
Member

Choose a reason for hiding this comment

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

This paragraph should be moved to the end of the next section ("Flash Messages") and can maybe be put in a .. sidebar:: or .. tip:: directive instead.

@@ -18,6 +18,9 @@ functionality.
Functionality Shipped with the HttpKernel
-----------------------------------------

.. versionadded:: 3.3
The ``SessionValueResolver`` was introduced in Symfony 3.3.

Symfony ships with four value resolvers in the HttpKernel component:
Copy link
Member

Choose a reason for hiding this comment

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

four -> five

@xabbuh
Copy link
Member

xabbuh commented Apr 15, 2017

Thank you @iltar.

@xabbuh xabbuh closed this in b418615 Apr 15, 2017
xabbuh added a commit that referenced this pull request Apr 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants