Skip to content

Create new shortcut methods in the Controller\Controller #11166

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.

Sign up for GitHub

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

Jump to bottom
Closed
edwines opened this issue Jun 18, 2014 · 12 comments
Closed

Create new shortcut methods in the Controller\Controller #11166

edwines opened this issue Jun 18, 2014 · 12 comments
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony)

Comments

@edwines
Copy link

edwines commented Jun 18, 2014

Following the new DX initiative, I want to propose to add some new shortcut methods to the Symfony\Bundle\FrameworkBundle\Controller\Controller.

I like a lot the shortcuts created in the KnpRadBundle; so my proposal is to implement:

  • addFlash($type, $message)
  • redirectToRoute($routeName, $parameters)
  • isGranted($role, $object)
@dirkluijk
Copy link

+1

Who cares about coupling should use controllers as services in the first place. Symfony's default Controller is meant for users who are already using the full-stack framework, so why not adding more shortcuts?

@bezhermoso
Copy link

Maybe a getParameter, too, in place of having to do $this->get('service_container')->getParameter('foo')

@merk
Copy link
Contributor

merk commented Jun 27, 2014

@bezhermoso $this->container->getParameter('foo')

@bezhermoso
Copy link

@merk Oh, cool! :) Disregard my suggestion then. Haha, I feel stupid for not realizing that XD

@manuelj555
Copy link

👍

lukaswoj added a commit to lukaswoj/symfony that referenced this issue Jul 5, 2014
@lukaswoj
symfony#11166 [FrameworkBundle] - Added addFlash(), redirectToRoute()…
aef56d4 
… and isGranted() shortcut methods to Controller
@lukaswoj
Copy link

lukaswoj commented Jul 5, 2014

Hi there
I'm trying to contribute for the first time so I'm not sure if did what was expected.
Do you guys thing those methods needs tests ?

@weaverryan
Copy link
Member

@lukaswoj Probably not. There is a ControllerTest class, but only one method is tested. And it makes sense, almost all of the methods in Controller are just one-liners that call a service :).

@Cydonia7
Copy link
Contributor

Cydonia7 commented Aug 5, 2014

I feel like the commit proposed by @lukaswoj has some wrong things in it and this is also the first time I would like to help on the core so I don't know.

Is it better to wait for him to change his work according to the comments or propose my own solution ?

Also, I think we should also add checkGranted($role, $object), that would cause the exception to be raised since it doesn't belong in an isGranted method that should only return a boolean according to conventions.

@ghost
Copy link

ghost commented Aug 5, 2014

@Cydonia7 if your proposal is quite different, then you should submit your own. If you think their pull request can be fixed, then you should comment on it.

@lukaswoj
Copy link

lukaswoj commented Aug 5, 2014

Sorry guys for abandoning this like that.
@Cydonia7 - if you can propose something - go for it, please. I consider my first contribution as a false-start - will improve next time.

@Cydonia7
Copy link
Contributor

Cydonia7 commented Aug 6, 2014

Here is my try to add these methods : https://github.com/Cydonia7/symfony/commit/73d8f327dd0ef7c0513966ed579094a013ea82f8

I think it is good enough to create a PR, what do you think ?

@weaverryan
Copy link
Member

@Cydonia7 It's certainly a fine good start. Go ahead and create a PR - we can have some more discussion there. Thanks for taking that on!

@Cydonia7 Cydonia7 mentioned this issue Aug 6, 2014
Merged
1 task
@fabpot fabpot closed this as completed Sep 19, 2014
fabpot added a commit that referenced this issue Sep 19, 2014
@fabpot
feature #11593 [FrameworkBundle] Add shortcut methods to controllers …
ebfda57 
…(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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
10 participants
@fabpot @javiereguiluz @weaverryan @lukaswoj @merk @edwines @manuelj555 @bezhermoso @dirkluijk @Cydonia7