Skip to content
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

Inject also query parameters as controller arguments #3254

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

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

@walva
Copy link

walva commented Sep 15, 2020

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
Projects
None yet
Development

No branches or pull requests

7 participants