[RFC] Added the constraint option on the ParamFetcher

[ClementGautier]

Hi friends of Symfony :)

For business needs, I want that my API allows some complex QueryParams like deep array structures. For example I want to be able to query /api/users?filters[0][key]=name&filters[0][operator]=equals&filters[0][value]=Foobar.

I wanted those parameters validated in order to return a BadRequestResponse like its did using the requirements of the QueryParam or RequestParam so I added an option: constraint and this constraint is validated by the Validator component.

requirements and constraint cannot be combined.

[lsmith77]

ok .. let me know when you have some docs to review

[ClementGautier]

@lsmith77 It's ready for review

[stof]

The typehint should be the ValidatorInterface, not the implementation

[ClementGautier]

@stof Fixed, thx.

[adrienbrault]

Whether you want nested values or validation, I think you should use a form:

public function fooAction(Request $request)
{
    $form = $this->get('form.factory')->createNamed('', 'acme_foo_filter');
    $form->submit($request->query->all(), false);
}

I also think QueryParam should always be replaced by using a form.

What the FOSRestBundle could do to help is add some kind of ParamConverter to get the bound form as an argument:

/**
 * @SomeFosRestConverter(name = "form", type = "acme_foo_filter", request = {"query", "_attributes"})
 */
public function fooAction(Form $filterForm)
{

}

[ClementGautier]

@adrienbrault I disagree. With your proposal we should drop completly the requirements support as the @RequestParam and the @QueryParam. In this PR i'm basicly adding functionalities to the requirements option. With the constraint option we could drop the support of requirements as these 2 things are the same:

/** @QueryParam(name="foo", constraint=@Regex("\d+")) */

and

/** @QueryParam(name="foo", requirements="\d+") */

I'm only extending the functionalities here.

The "problem" with your SomeFosRestFormConverter proposal is that we would not be able to set datas before submiting request datas. As these Query parameters are used for basic purpose like filtering we will not use these form for the rendering process so, for me, it's not a usefull usage of forms.

[stof]

@adrienbrault I disagree as well. The main feature of the Form component is data binding. If all you need is validating data, binding a form is useless overhead (and not negligeable overhead). It is better to run only the validator in such case

[lsmith77]

Right, I guess this is mostly for cases where it is not really necessary to map things to a model/form type and where an array structure is really sufficient.

[adrienbrault]

I'm not arguing against that change, I'm arguing against the whole @QueryParam concept.

[ClementGautier]

Soooo ... Could we merge this as it ? We can discuss of droping QueryParam support in an other PR I guess.

[lsmith77]

can remove this empty line?

[ClementGautier]

I finaly reviewed the code. I hope it will be ok for you.

[lsmith77]

needs a rebase according to github

[willdurand]

preg_quote()?

[ClementGautier]

Rebased and added preg_quote as @willdurand suggested

[ClementGautier]

wait, the annotation does not work:

Attribute "requirements" of @QueryParam declared on method Acme\Bundle\AcmeBundle\Controller\AcmeController::cgetAction() expects a(n) string|\Symfony\Component\Validator\Constraint, but got an instance of Acme\Bundle\AcmeBundle\Validator\Constraints\AcmeConstraint

[ClementGautier]

I changed string|Constraint to mixed. It's ok now. Sorry

[lsmith77]

thx!

[ClementGautier]

thx ;)

[snc]

It looks like the requirements are not working anymore because of preg_quote. The requirements="\d+" results in 'pattern' => '#^\\d\+$#xsu'.

[lsmith77]

ok, i guess we need to document that quoting is the responsibility of the user in that case and remove it. can you work on a PR, ideally with a test case?

[stof]

Well, given that the requirement is a regex, applying preg_quote is indeed wrong

[eXtreme]

Yup it broke all regexes in my "requirements".

[willdurand]

Ahm, that was more a question than a suggestion.. Sorry :-(

[lsmith77]

please test ed28a05

[snc]

Looks good @lsmith77.

[lsmith77]

ok .. i have released 1.3.1

[eXtreme]

BTW it is a funny situation because this should have been covert by unit tests but.. tests have been updated as well. :)

[SilverSting]

How can I set custom options for the Constraint?
The following doesn't work, the {"message": ...} ends up wrapped by another array.

requirements=@Constraints\NotBlank({"message": "my custom message"})

[mvrhov]

Oh well. The upcoming sf 2.5 broke the validator implementation.

[stof]

annotations have never been meant to work as @Constraints\NotBlank({"message": "my custom message"}) but as @Constraints\NotBlank(message="my custom message").

Your syntax is passeing an array as param to the constraint, with a message key in it. This param gets set in the default property of the constraint