Query param fetcher #185

Merged
merged 49 commits into from May 16, 2012

Conversation

Projects
None yet
Owner

lsmith77 commented Feb 7, 2012

maybe instead of a listener we can use a param converter:
http://symfony.com/doc/2.0/bundles/SensioFrameworkExtraBundle/annotations/converters.html

Owner

stof commented Feb 7, 2012

does the QueryParam stuff depend on FrameworkExtraBundle currently ?

Owner

lsmith77 commented Feb 7, 2012

no .. currently it does not. but i don't really like having to use a listener to set the request attribute.

Request/QueryFetcher.php
+ throw new \InvalidArgumentException('No _controller for request.');
+ }
+
+ if (false !== strpos($_controller, '::')) {
@vicb

vicb Feb 7, 2012

Contributor

Could you leverage the ControllerResolver ?

@lsmith77

lsmith77 Feb 7, 2012

Owner

yeah maybe ..

@lsmith77

lsmith77 Feb 7, 2012

Owner

looking at the code it seems like it would be inefficient for the case when the controller is not implemented as a service. but it looks like we are lacking support for a:b:c notation atm.

@vicb

vicb Feb 7, 2012

Contributor

That would be more dry and future proof

@lsmith77

lsmith77 Feb 7, 2012

Owner

ok .. we should now cover all the cases

@lsmith77

lsmith77 Feb 7, 2012

Owner

yeah it would .. but i don't think the overhead makes this legitimate.

@vicb

vicb Feb 7, 2012

Contributor

could the controller event help with efficiency ?

@lsmith77

lsmith77 Feb 7, 2012

Owner

hmm it could be a possibility. in that case we would move the other listener from a request listener to a controller listener as well. in that case of course the parameters would no longer be available in any listeners run before the controller, but i guess that would be ok.

@lsmith77

lsmith77 Feb 7, 2012

Owner

ok .. i have an implementation using a controller event .. will push that into a new branch

Request/QueryFetcher.php
+
+ // Set default if the requirements do not match
+ if ($param !== $this->params[$name]->default
+ && !preg_match('/^' . $this->params[$name]->requirements . '/xs', $param)
@vicb

vicb Feb 7, 2012

Contributor

is '$' missing on purpose ?

Request/QueryFetcher.php
+ if ($param !== $this->params[$name]->default
+ && !preg_match('/^' . $this->params[$name]->requirements . '/xs', $param)
+ ) {
+ $param = $this->params[$name]->default;
@vicb

vicb Feb 7, 2012

Contributor

factorize $default

Request/QueryFetcher.php
+ throw new \InvalidArgumentException(sprintf("No @QueryParam configuration for parameter '%s'.", $name));
+ }
+
+ $param = $this->request->query->get($name, $this->params[$name]->default);
@vicb

vicb Feb 7, 2012

Contributor

using has() would allow always validating against the requirement (thinking of the default value here but not sure of what is the desired behavior)

@lsmith77

lsmith77 Feb 7, 2012

Owner

that $param !== $default further down is just a performance optimization to avoid the regexp if it isn't needed. so i think the code is ok as is.

@vicb

vicb Feb 7, 2012

Contributor

just wanted to make sure you were aware of the edge case here.

EventListener/QueryFetcherListener.php
+ public function onKernelRequest(GetResponseEvent $event)
+ {
+ $request = $event->getRequest();
+ $request->attributes->set('queryFetcher', $this->container->get('fos_rest.request.query_fetcher'));
@vicb

vicb Feb 7, 2012

Contributor

do we really need a listener, can't it be lazy loaded (just a thought, too late to check for now)

@lsmith77

lsmith77 Feb 7, 2012

Owner

we need a listener to set the request attribute, so that it can be injected via the action signature, which is imperative to controllers that do not inject the DIC ..

closures and fonctions won't be catched here as they are not arrays. The first if should only check for null (or eventually for is_callable if you want)

Contributor

vicb commented Feb 9, 2012

Some thoughts:

Is the queryFetcher a good idea ? Let's imagine I do some processing in my controller based on the query parameters and then I call doSomeGreatStuff($request). This function would get the query parameters as $request->query->... and then we have a mismatch. Would it be possible to drop the queryFetcher and replace (or update) the query parameter bag instead so that the API does not get changed ?

I can imagine some day we have a router component that is aware of the query parameters. This PR could not work as the parameters are processed in the controller only. So I would prefer if the changes are moved to the router layer.

Owner

lsmith77 commented Feb 9, 2012

Overriding the Request parameter bag would remove the need to set the request attribute. How would it behave in that case though? I guess it would then only overload get() to set the default from the annotation (what would happen if a default is passed?) and execute the given requirements checks from the annotation?

Owner

lsmith77 commented Feb 9, 2012

the other question would be where and when do we override the default parameter bag? would this require a custom Request class or a listener again?

Contributor

vicb commented Feb 9, 2012

@lsmith77

  • What I can imagine is to update the parameters in the bag and then use the bag as before.
  • I would go for the router layer for the reason explained in my previous message. I had first thought of a custom bag class but I don't even think it is necessary (see my point above).
Owner

lsmith77 commented Feb 9, 2012

Well I see some major issues here:

  1. it would mean we do a lot of work that may not become necessary
  2. if we do it in the router, then we will have the issue with determining the class/method again
Contributor

vicb commented Feb 9, 2012

Well maybe the best place is the controller event. IMO the most important thing is to drop the $queryFetcher and be able to use doSomeGreatStuff($request). I let you work out the details.

Owner

lsmith77 commented Feb 9, 2012

If we do this, then we would best wrap the standard ParameterBag inside one that automatically sets the defaults and checks the requirements when any of the get*() methods are called. However this may also lead to a bit of unclearity as to when these requirements are applied. Aka sometimes when you use the request it would apply the requirements .. but for example inside a request listener they would not. I think this makes things more obscure, so I prefer the current approach.

Contributor

vicb commented Feb 9, 2012

That might indeed be a bad idea but the current implementation is even worse then. The solution would be to change to router to support matching query params.

Owner

lsmith77 commented Feb 9, 2012

I don't see the current implementation as worse at all. It just means that adopting this approach requires to change some API calls.

Now changing the router is of course another option. But I am not sure if this is possible without either not supporting all the dumpers or increasing the scope of this PR to require a gigantic amount of work. Then again I guess we will get this once we have uri-template support:
http://tools.ietf.org/html/draft-gregorio-uritemplate-08
symfony/symfony#3227

Owner

lsmith77 commented Feb 9, 2012

Btw .. in this case we would of course get around the entire controller setting business, since we would add this information into the routes. Thinking about it .. maybe we could do this today as well? Aka instead of reading the annotations at runtime, we could integrate the query param annotations into the rest route loader and write this information in there somewhere (not sure if its possible) and then read it out in the query fetcher ..

Contributor

vicb commented Feb 9, 2012

I think you mentioned that the code should work after re-factoring. Keeping the $queryFetcher prevent this, right ? Passing it as a parameter does not solve the solution either: it would introduce coupling and legacy (/3rd party) services would not work.

Owner

lsmith77 commented Feb 9, 2012

Prevents what? Which refactoring do you mean specifically?

Contributor

vicb commented Feb 9, 2012

Moving some code from an action to a service (i.e. doSomeGreatStuff($request)). Wasn't that you were speaking about.
Anyway the other part doSomeLegacyStuff($request), doSome3rdPartyStuff($request) is still valid.

Owner

lsmith77 commented Feb 9, 2012

Ah yes. Any service after the controller listener can inject the query fetcher as it is now and things will work cleanly. Any server before the controller listener will get an exception. So no surprises.

Owner

lsmith77 commented May 15, 2012

We can easily create a request listener that does that or rather extend the current one to optionally do that. The reason I say optionally is that with what you describe all parameters would always have to be parsed and validated even if they are not used, but in most cases most of the configured query params will likely be used anyway.

Of course one could image getting some query params only conditinally:

if ($search = $queryFetcher->getParameter('search')) {
    $term = $queryFetcher->getParameter('term');
}

but I personally would just fetch all the queryparams in one block anyway and deal with them conditionally later.

$search = $queryFetcher->getParameter('search');
$term = $queryFetcher->getParameter('term');
…
if ($search) { … }

grEvenX commented May 15, 2012

I agree on the expressability part of injecting the query params.
If one defines a route with parameters and also uses queryparam, how will it affect the method signature and which (if any) would take precedence?

E.g:

/*
 * @Route("/{id}", requirements={"id" = "\d+"})
 * @QueryParam(name="id",requirements={"id" = "\d+"})
 */
public function getCustomerAction($id) 
Owner

stof commented May 15, 2012

@grEvenX The bundle does not change the request attributes, and so will not change the params available for the method arguments.

Owner

lsmith77 commented May 15, 2012

@stof but we could ..

@grEvenX I think a conflict should raise an exception.

Owner

stof commented May 15, 2012

it would duplicate the param in 2 parameter bags (query and attributes). I don't think it is a good idea

Owner

asm89 commented May 15, 2012

Honestly I think it doesn't make sense to inject the query parameters in your method signature. That would make your code even less portable. My original idea was that the query param thing could help you with checking very basic requirements on the query parameters (like &page should be \d+) and return a default value (1 for page) or null otherwise. The upside of this would be less checking of stuff like that "by hand", but also having controller methods annotated with the appropriate query parameters which is very nice for api documentation etc.

That would make your code even less portable.

Can you elaborate?

Do you mean, by hard coding the query params in the method signature one limits possibility of extending the method in a subclass with a different implementation which would require other params?

Owner

lsmith77 commented May 15, 2012

ok .. i have implemented the option of forcing the query params to be set as attributes. on conflict it throws an exception. however we also need to improve the route generation to skip any configured query parameter.

Owner

lsmith77 commented May 15, 2012

btw .. just FYI .. you can always fork this branch and submit PRs back to this branch if you have ideas for improvements. for example it would be cool of someone could fix the above noted issue with using query params in the method signature causing them to be put into the route.

Owner

lsmith77 commented May 15, 2012

nevermind .. already took care of it ..
anything else people think should be done?

Owner

lsmith77 commented May 15, 2012

main thing missing now are docs ..

Owner

stof commented May 15, 2012

@lsmith77 As I already said a while ago, I think you should add an interface for the QueryParamReader and use it in the typehints, to allow people to replace the implementation if they want.

Owner

lsmith77 commented May 15, 2012

QueryParamReader or QueryFetcher? Anyway, I will leave that to who ever wants to make the first alternative implementation :)

Owner

stof commented May 15, 2012

probably both :)
But I was talking about the reader first, thus allowing people to write an implementation reading from elsewhere than annotations if they don't like annotations

DependencyInjection/Configuration.php
@@ -39,6 +39,7 @@ public function getConfigTreeBuilder()
$rootNode
->children()
+ ->scalarNode('query_fetcher_listener')->defaultValue(false)->end()
@stof

stof May 16, 2012

Owner

shouldn't it be a booleanNode ? And you could use defaultFalse()

@stof

stof May 16, 2012

Owner

ok, it is not a boolean. But you should probably limit the possible values

@lsmith77

lsmith77 May 16, 2012

Owner

whats the syntax for that?

DependencyInjection/Configuration.php
+ ->scalarNode('query_fetcher_listener')->defaultFalse()
+ ->validate()
+ ->ifNotInArray($this->forceOptionValues)
+ ->thenInvalid('The query_fetcher_listener option does not support %s. Please choose one of '.json_encode($this->forceOptionValues))
@stof

stof May 16, 2012

Owner

missing one indentation level

DependencyInjection/Configuration.php
+ ->scalarNode('view_response_listener')->defaultValue('force')
+ ->validate()
+ ->ifNotInArray($this->forceOptionValues)
+ ->thenInvalid('The view_response_listener option does not support %s. Please choose one of '.json_encode($this->forceOptionValues))
@stof

stof May 16, 2012

Owner

same here

lsmith77 added a commit that referenced this pull request May 16, 2012

@lsmith77 lsmith77 merged commit 0aa7534 into master May 16, 2012

Contributor

schmittjoh commented May 16, 2012

About silently setting the default value on validation failure, would it not make more sense to redirect to the URL with the default value? Also in terms of SEO, and duplicate content, this seems more sensible, no?

Owner

lsmith77 commented May 16, 2012

i thought about that too .. we could provide such an option as part of the listener eventually. it should of course only be down for proper GET requests. however i don't think that query parameters are that important for SEO, but it is relevant for caches i guess.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment