8000 Inject also query parameters as controller arguments · Issue #3254 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Inject also query parameters as controller arguments #3254

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
Koc opened this issue Feb 2, 2012 · 9 comments
Closed

Inject also query parameters as controller arguments #3254

Koc opened this issue Feb 2, 2012 · 9 comments
Labels
Feature HttpKernel

Comments

@Koc
Copy link
Contributor
Koc commented Feb 2, 2012

There is some inconsistency now. For example I have route

    # app/config/routing.yml
    hello:
        pattern:      /hello/{name}
        defaults:     { _controller: AcmeHelloBundle:Hello:index }

and action

<?php

    public function indexAction($name)
    {
      // ...
    }

And after some time we want change route

    # app/config/routing.yml
    hello:
        pattern:      /hello/
        defaults:     { _controller: AcmeHelloBundle:Hello:index }

After this change urls will containe name as get parameter, this part is ok. But controller hasn't more argument $hello, so after changing in route I should go to each controllers and change also some code.

https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/Controller/ControllerResolver.php#L110

<?php

$attributes = array_merge($request->query->all(), $request->attributes->all());
@piotrpasich
Copy link

Hi,

in my opinion You have to check in action where the $name will be used. Maybe the index action wouldn't work fine without $name or with $name = null, that's right? You may define default value for name like this:

hello:
    pattern:      /hello
    defaults:     { _controller: AcmeHelloBundle:Hello:index , name: 'defaultName' }

and it will be working without going to action and removing $name argument .

@Koc
Copy link
Contributor Author
Koc commented Mar 16, 2012

no, $name cann't be null in my case

@stof
Copy link
Member
stof commented Apr 4, 2012

@fabpot what do you think about it ?

@fabpot
Copy link
Member
fabpot commented Apr 4, 2012

I've already rejected the same proposal in the past.

Right now, the possible values are defined by the route and you cannot add any other one. But if you allow values from the query string, then the end user is able to inject any value in your controllers. This is a major security hole.

@fabpot fabpot closed this as completed Apr 4, 2012
@lsmith77
Copy link
Contributor
lsmith77 commented Apr 4, 2012

wouldn't the proper way be to implement uri templates for query parameters (see #3227)?

@gggeek
Copy link
gggeek commented Feb 7, 2014

@fabpot sorry for trying to revive an old topic, but what if the only parameters from the query string which would get transformed into controller arguments were the ones set in the routing definition? I see little security problem with it.
It would make it simpler to do a controller which has a pagination parameter and can at the same time be invoked/reused directly from php layer (where request might not be available/built, eg. when running cli scripts)

@stof
Copy link
Member
stof commented Feb 7, 2014

@gggeek your use case seems to be a bad architecture. If you need to reuse your logic outside the controller (for a CLI for instance), you should extract the logic to a separate class rather than trying to call your controller from the CLI (the CLI does not expect an HTTP Response anyway)

@gggeek
Copy link
gggeek commented Feb 7, 2014

@stof I do not disagree in theory, but that adds a lot of boilerplate code if you do it for many controllers

@Koc Koc mentioned this issue Aug 21, 2016
Closed
@walva
Copy link
Contributor
walva commented Sep 15, 2020 •
edited
Loading

Hello,
I just want to mention that it is possible to inject a query value in the controller parameter by creating a converter.
But I agree this must be use carrefully.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Feature HttpKernel
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@fabpot @Koc @lsmith77 @gggeek @stof @piotrpasich @walva
0