@QueryParam default value from the PHP code directly #416

Open
dewos opened this Issue Apr 2, 2013 · 7 comments

Comments

Projects
None yet
3 participants
@lsmith77

This comment has been minimized.

Show comment
Hide comment
@lsmith77

lsmith77 Apr 3, 2013

Member

can you try and implement this?

Member

lsmith77 commented Apr 3, 2013

can you try and implement this?

@dewos

This comment has been minimized.

Show comment
Hide comment
@dewos

dewos Apr 6, 2013

Contributor

Sorry, not now. I'm overloaded with work.

Contributor

dewos commented Apr 6, 2013

Sorry, not now. I'm overloaded with work.

@benbender

This comment has been minimized.

Show comment
Hide comment
@benbender

benbender Apr 21, 2013

The problem is a bit deeper I think. If I have something like:

    /**
     * @Rest\QueryParam(name="page",  requirements="\d+", default="1", description="Page of the overview")
     * @Rest\QueryParam(name="limit", requirements="\d+", default="25", description="Limit per page")
     */
    public function indexAction($page = 1, $limit = 25)

and set param_fetcher_listener to force I'm getting an exception from:

FOS/RestBundle/EventListener/ParamFetcherListener.php at line 64
with the message:
ParamFetcher parameter conflicts with a path parameter 'page' for route 'api_model_index'"

If I remove the defaults from the method-signature, everything is working fine. So we have a conflct between ParamFetcher from FOSRest and ParamConverter from SensioExtraBundle.

I would be happy to provide a patch, but I'm unsure what`s the intended behavior would be. The simplest (and imho propably best) solution would be, to simple remove the Exception from ParamFetcherListener and document the behaviour, that if both Listeners are used, the ParamConverter takes precedence.

The problem is a bit deeper I think. If I have something like:

    /**
     * @Rest\QueryParam(name="page",  requirements="\d+", default="1", description="Page of the overview")
     * @Rest\QueryParam(name="limit", requirements="\d+", default="25", description="Limit per page")
     */
    public function indexAction($page = 1, $limit = 25)

and set param_fetcher_listener to force I'm getting an exception from:

FOS/RestBundle/EventListener/ParamFetcherListener.php at line 64
with the message:
ParamFetcher parameter conflicts with a path parameter 'page' for route 'api_model_index'"

If I remove the defaults from the method-signature, everything is working fine. So we have a conflct between ParamFetcher from FOSRest and ParamConverter from SensioExtraBundle.

I would be happy to provide a patch, but I'm unsure what`s the intended behavior would be. The simplest (and imho propably best) solution would be, to simple remove the Exception from ParamFetcherListener and document the behaviour, that if both Listeners are used, the ParamConverter takes precedence.

@lsmith77

This comment has been minimized.

Show comment
Hide comment
@lsmith77

lsmith77 Apr 21, 2013

Member

i dont really use either of the features in any project, so its hard for me to tell what the best direction is ..

Member

lsmith77 commented Apr 21, 2013

i dont really use either of the features in any project, so its hard for me to tell what the best direction is ..

@benbender

This comment has been minimized.

Show comment
Hide comment
@benbender

benbender Apr 21, 2013

As I thought a bit more about this topic, it became clear, that the problem is more or less conceptional. The root-problem is that the force-option copies content from one "context" (in symfony terms query- to request) to another and there is no defined solution to handle possible conflicts.

If I think about the expected behavior, I would say that defaults of QueryParamFetcher should take precedence because it is the place where I define the query-parameter I am actually sending. This would also play nice with the term "force" in the ParamFetcher-config.

So imho there are two possible solutions:

  • Give precedence to QueryParamFetcher:
    • the QueryParam overwrite defaults of the route-definition / ParamConverter.
    • the defaults are defined in the QueryParam-Annotation. Possible defaults in the method-signature or route-definition are ignored if both set.
    • There should no exception to be thrown if both set defaults
    • this behavior should be documented
  • Define the actual behavior as expected and document it because we think the conflict can't be decided by the bundle

Either way, there should be a defined and documented way to resolve this (or not) :)

As I thought a bit more about this topic, it became clear, that the problem is more or less conceptional. The root-problem is that the force-option copies content from one "context" (in symfony terms query- to request) to another and there is no defined solution to handle possible conflicts.

If I think about the expected behavior, I would say that defaults of QueryParamFetcher should take precedence because it is the place where I define the query-parameter I am actually sending. This would also play nice with the term "force" in the ParamFetcher-config.

So imho there are two possible solutions:

  • Give precedence to QueryParamFetcher:
    • the QueryParam overwrite defaults of the route-definition / ParamConverter.
    • the defaults are defined in the QueryParam-Annotation. Possible defaults in the method-signature or route-definition are ignored if both set.
    • There should no exception to be thrown if both set defaults
    • this behavior should be documented
  • Define the actual behavior as expected and document it because we think the conflict can't be decided by the bundle

Either way, there should be a defined and documented way to resolve this (or not) :)

@dewos

This comment has been minimized.

Show comment
Hide comment
@dewos

dewos May 5, 2013

Contributor
  • +1 the QueryParam overwrite defaults of the route-definition / ParamConverter.
Contributor

dewos commented May 5, 2013

  • +1 the QueryParam overwrite defaults of the route-definition / ParamConverter.
@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost May 20, 2014

Could anyone review my issue:

#774

Thanks in advance.

ghost commented May 20, 2014

Could anyone review my issue:

#774

Thanks in advance.

@lsmith77 lsmith77 added this to the 2.0 milestone Jul 10, 2015

@lsmith77 lsmith77 modified the milestone: 2.0 Nov 10, 2015

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