[FrameworkBundle] Add shortcut methods to controllers #11593

Cydonia7 · 2014-08-06T18:16:25Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#11166
License	MIT
Doc PR	symfony/symfony-docs#4109

To-do list :

submit changes to the documentation

Added redirectToRoute, addFlash, isGranted and checkGranted to controllers. The code seems so simple I didn't feel like adding controller tests was needed since we're just shortcuting other services calls.

Cydonia7 · 2014-08-06T18:37:31Z

There is an error in the tests but not coming from the code for this PR.

fabpot · 2014-08-07T13:09:04Z

src/Symfony/Bundle/FrameworkBundle/Controller/Controller.php

+     *
+     * @return RedirectResponse
+     */
+    public function redirectToRoute($route, array $parameters = array(), $status = 302)


All those method should be protected

Cydonia7 · 2014-08-07T13:33:36Z

@fabpot That's done, thanks for the feedback! (in commit 82d29b0)

stof · 2014-08-07T13:38:59Z

src/Symfony/Bundle/FrameworkBundle/Controller/Controller.php

+     */
+    protected function addFlash($type, $message)
+    {
+        $this->get('session')->getFlashBag()->add($type, $message);


the shortcut needs to check whether the session service is defined, and throw a LogicException if it is not there. The session system can be disabled in FrameworkBundle (for instance when your app is a stateless API)

malarzm · 2014-08-14T14:29:23Z

src/Symfony/Bundle/FrameworkBundle/Controller/Controller.php

+            throw new \LogicException('You can not use the addFlash method if sessions are disabled.');
+        }
+
+        $this->get('session')->getFlashBag()->add($type, $message);


Isn't this too optimistic given that SessionInterface doesn't enforce getFlashBag()?

Maybe we should add an interface for flashbags and make Session implement it then check if it does in this method.

Just found issue about it so I'll cross-reference it: #11279 - anyway I think that some FlashAwareSessionInterface isn't bad. However it's not up to me, just saying :)

I'll leave it this way (see messages below for details).

weaverryan · 2014-08-16T14:54:26Z

@Cydonia7 Can you open an issue or PR on the docs for this?

It looks good to me. There may be other shortcuts we want to add, but I'm happy with these 3.

Do we need any CHANGELOG or UPGRADE to be modified? This is technically a tiny BC break for someone that has these methods in their own base controller already but with a different signature - that's why I was wondering about updating one of these files (but maybe that's overkill).

Cydonia7 · 2014-08-17T09:14:11Z

@weaverryan I already created a PR for the docs but I just forgot to reference it in the first message, now that's done (symfony/symfony-docs#4109). Do you think I should implement FlashAwareSessionInterface to check if getFlashbag exists ?

weaverryan · 2014-08-17T15:27:01Z

Perfect - thanks @Cydonia7!

I don't think you should make any changes to the flashbag stuff. In reality, when you're using the framework, the session service is a Session object, not a SessionInterface. In other words, imo, since you're using the framework, we know you'll get back something that has access to a flashbag (unless you're really changing something core and overriding the session with something that doesn't support flashes, in which case, you're smart enough to know not to try calling this method).

Cydonia7 · 2014-08-17T17:00:28Z

I agree with you, waiting for someone to give more feedback or merge the PR then. Do I have to squash the commits first ?

stof · 2014-08-18T11:47:15Z

@Cydonia7 you can squash if you want, but it is not necessary. We are able to do it automatically when merging.

Please add a note in the changelog of the FrameworkBundle to mention the new methods, so that people know that we have added them (as we did for Controller::isCsrfTokenValid)

Cydonia7 · 2014-08-18T19:34:27Z

@stof That's done. I didn't know Github was clever enough to allow squashing pull requests, pretty cool but it should be removed from the patches documentation then.

stof · 2014-08-18T21:21:30Z

@Cydonia7 it is not github. It is the Symfony core team (we don't use the green merge button)

stof · 2014-08-18T21:22:00Z

👍

romainneutron · 2014-08-19T08:59:10Z

👍

Cydonia7 · 2014-08-23T08:43:13Z

I finally squashed the commits as @wouterj said it was better.

aferrandini · 2014-08-26T00:38:00Z

src/Symfony/Bundle/FrameworkBundle/Controller/Controller.php

+     */
+    protected function redirectToRoute($route, array $parameters = array(), $status = 302)
+    {
+        return new RedirectResponse($this->generateUrl($route, $parameters), $status);


This is a bit redundant, you can use the method Controller::redirect.
Why don't you use it like?

$this->redirect($this->generateUrl($route, $parameters), $status);

That's done.

weaverryan · 2014-09-10T10:09:38Z

@Cydonia7 If you have a chance, can you rebase again to resolve the conflict? I think this can be merged even with the conflict, but maybe it'll help.

This has a 👍 from me too fwiw.

Cydonia7 · 2014-09-10T13:07:57Z

@weaverryan I think it should be okay.

jakzal · 2014-09-13T00:24:33Z

src/Symfony/Bundle/FrameworkBundle/Controller/Controller.php

+     * @param mixed  $object     The object
+     * @param string $message    The message passed to the exception
+     *
+     * @throws \Symfony\Component\Security\Core\Exception\AccessDeniedException


Should be a short form with a full namespace added to the use statements.

shortcuts to controllers.

webmozart · 2014-09-18T17:43:56Z

👍

fabpot · 2014-09-19T06:10:46Z

👍

fabpot · 2014-09-19T06:11:24Z

Thank you @Cydonia7.

…(Cydonia7) This PR was merged into the 2.6-dev branch. Discussion ---------- [FrameworkBundle] Add shortcut methods to controllers | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #11166 | License | MIT | Doc PR | symfony/symfony-docs#4109 To-do list : - [x] submit changes to the documentation Added redirectToRoute, addFlash, isGranted and checkGranted to controllers. The code seems so simple I didn't feel like adding controller tests was needed since we're just shortcuting other services calls. Commits ------- 74d8c9a Add redirectToRoute, addFlash, isGranted and denyAccessUnlessGranted shortcuts to controllers.

shoomyth · 2014-09-19T09:31:26Z

I thought session service is deprecated in container and RequestStack should be used instead?

stof · 2014-09-19T13:56:24Z

@shoomyth request is deprecated, not session

shoomyth · 2014-09-19T14:01:18Z

@stof ref #10557
I see this is in 3.0, my remark of deprecation was just FYI

stof · 2014-09-19T21:57:15Z

this is a discussion about a change for 3.0. We haven't made any decision yet

The service `security.context` is now deprecated due to PR symfony#11690. This commit updates PR symfony#11593 replacing `security.context` with `security.authorization_checker` in the `isGranted()` controller shortcut.

This PR was merged into the 2.6 branch. Discussion ---------- [FrameworkBundle] Update deprecated service call | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | none, but related to #11593 and #11690 | License | MIT | Doc PR | none Service `security.context` is now deprecated due to PR #11690. This commit updates PR #11593 replacing `security.context` with `security.authorization_checker` in the `isGranted()` controller shortcut introduced in Symfony 2.6. Commits ------- 2625193 [FrameworkBundle] Update deprecated service call

This PR was merged into the 2.6 branch. Discussion ---------- Rebased "add shortcut methods" Replaces #4109 Original Pr description: > | Q | A > | ------------- | --- > | Doc fix? | no > | New docs? | yes (symfony/symfony#11593) > | Applies to | 2.6+ > | Fixed tickets | #4666 > > This commit is associated to symfony/symfony#11593 that adds new shortcut methods to controllers. > > If anything is wrong with my commit or this description, please tell me and I will fix it as soon as possible. I'm glad I finally try to help this awesome project. Commits ------- 994ed3a Little fixes f807d14 Fixes 5b015f2 Modifications according to comments 7f9bc8c And now the same for isGranted where it is possible 46e2505 Changed to addFlash where it is possible (ie in controllers) 643c458 redirect changed to redirectToRoute 7ae62e8 Minor improvements 4611ce9 Minor format improvements 3bcb186 Added shortcut methods for controllers

fabpot reviewed Aug 7, 2014
View reviewed changes

stof reviewed Aug 7, 2014
View reviewed changes

Cydonia7 mentioned this pull request Aug 8, 2014

Added shortcut methods for controllers symfony/symfony-docs#4109

Closed

romainneutron added the FrameworkBundle label Aug 11, 2014

malarzm reviewed Aug 14, 2014
View reviewed changes

Cydonia7 mentioned this pull request Aug 23, 2014

[DX] No more squashing when doing patches symfony/symfony-docs#4167

Closed

aferrandini reviewed Aug 26, 2014
View reviewed changes

jakzal reviewed Sep 13, 2014
View reviewed changes

Add redirectToRoute, addFlash, isGranted and denyAccessUnlessGranted

74d8c9a

shortcuts to controllers.

fabpot merged commit 74d8c9a into symfony:master Sep 19, 2014

Cydonia7 deleted the add-shortcut-methods branch September 20, 2014 16:06

stloyd mentioned this pull request Oct 7, 2014

Access level error in AddressController addFlash Sylius/Sylius#1979

Closed

lrlopez mentioned this pull request Nov 30, 2014

[FrameworkBundle] Update deprecated service call #12787

Merged

wouterj mentioned this pull request Feb 26, 2015

Rebased "add shortcut methods" symfony/symfony-docs#5046

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FrameworkBundle] Add shortcut methods to controllers #11593

[FrameworkBundle] Add shortcut methods to controllers #11593

Cydonia7 commented Aug 6, 2014

Cydonia7 commented Aug 6, 2014

fabpot Aug 7, 2014

Cydonia7 commented Aug 7, 2014

stof Aug 7, 2014

malarzm Aug 14, 2014

Cydonia7 Aug 14, 2014

malarzm Aug 14, 2014

Cydonia7 Aug 17, 2014

weaverryan commented Aug 16, 2014

Cydonia7 commented Aug 17, 2014

weaverryan commented Aug 17, 2014

Cydonia7 commented Aug 17, 2014

stof commented Aug 18, 2014

Cydonia7 commented Aug 18, 2014

stof commented Aug 18, 2014

stof commented Aug 18, 2014

romainneutron commented Aug 19, 2014

Cydonia7 commented Aug 23, 2014

aferrandini Aug 26, 2014

Cydonia7 Aug 26, 2014

weaverryan commented Sep 10, 2014

Cydonia7 commented Sep 10, 2014

jakzal Sep 13, 2014

webmozart commented Sep 18, 2014

fabpot commented Sep 19, 2014

fabpot commented Sep 19, 2014

shoomyth commented Sep 19, 2014

stof commented Sep 19, 2014

shoomyth commented Sep 19, 2014

stof commented Sep 19, 2014

[FrameworkBundle] Add shortcut methods to controllers #11593

[FrameworkBundle] Add shortcut methods to controllers #11593

Conversation

Cydonia7 commented Aug 6, 2014

Cydonia7 commented Aug 6, 2014

Choose a reason for hiding this comment

Cydonia7 commented Aug 7, 2014

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

weaverryan commented Aug 16, 2014

Cydonia7 commented Aug 17, 2014

weaverryan commented Aug 17, 2014

Cydonia7 commented Aug 17, 2014

stof commented Aug 18, 2014

Cydonia7 commented Aug 18, 2014

stof commented Aug 18, 2014

stof commented Aug 18, 2014

romainneutron commented Aug 19, 2014

Cydonia7 commented Aug 23, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan commented Sep 10, 2014

Cydonia7 commented Sep 10, 2014

Choose a reason for hiding this comment

webmozart commented Sep 18, 2014

fabpot commented Sep 19, 2014

fabpot commented Sep 19, 2014

shoomyth commented Sep 19, 2014

stof commented Sep 19, 2014

shoomyth commented Sep 19, 2014

stof commented Sep 19, 2014