[Form] allow additional http methods in form configuration #26324
Conversation
|
Can we just remove that |
| @@ -70,6 +71,10 @@ public function boot() | |||
| if ($trustedHosts = $this->container->getParameter('kernel.trusted_hosts')) { | |||
| Request::setTrustedHosts($trustedHosts); | |||
| } | |||
|
|
|||
| if ($this->container->hasParameter('form.allowed_methods')) { | |||
| FormConfigBuilder::setAllowedMethods($this->container->getParameter('form.allowed_methods')); | |||
There was a problem hiding this comment.
I would suggest to try harder not using any global state for this feature.
Could be just an option for FormConfigBuilder maybe?
There was a problem hiding this comment.
FormConfigBuilder is not instantiated by the DI container, but by the ResolvedFormType class which is created by ResolvedFormTypeFactory, so the injection of a parameter is too cumbersome. I tried to create a TypeExtension to inject the parameter as an option, but it can be overridden on any form type (this means that we can pass method and allowed methods as options and it I don't think it makes sense).
Now i tried to inject the allowed methods directly in the form factory which is set into the FormConfigBuilder. What do you think about this solution?
4b82548 to
4f5ab03
Compare
|
What if if we move the check from This will give developers a possibility to write an extension for the |
There was a problem hiding this comment.
The solution suggested by @vudaltsov looks much better to me.
3e2a477 to
227da93
Compare
| @@ -29,7 +29,6 @@ | |||
| <!-- FormFactory --> | |||
| <service id="form.factory" class="Symfony\Component\Form\FormFactory" public="true"> | |||
| <argument type="service" id="form.registry" /> | |||
| <argument type="service" id="form.resolved_type_factory" /> | |||
There was a problem hiding this comment.
The second constructor parameter was deleted in 3607eb3 (included in 3.3 beta 1).
Should I restore it?
There was a problem hiding this comment.
I think @ro0NL meant that this change could probably be made on a lower version of the FrameworkBundle too (i.e. all versions that do not support the Form component in version 3.2 or lower).
|
@alekitto , why don't you like my solution? Forms have a clear extension mechanism - that is |
|
@vudaltsov in fact i’ve used it in my last revision. I moved the allowedMethods const in FormType and used it to set the ‘method’ option allowed values |
| * | ||
| * @var array | ||
| */ | ||
| private static $allowedMethods = array( |
|
@alekitto , I am so sorry, I missed that! Thank you :) |
| @@ -185,6 +203,11 @@ public function configureOptions(OptionsResolver $resolver) | |||
| $resolver->setAllowedTypes('label_attr', 'array'); | |||
| $resolver->setAllowedTypes('upload_max_size_message', array('callable')); | |||
| $resolver->setAllowedTypes('help', array('string', 'null')); | |||
|
|
|||
| $resolver->setAllowedValues('method', self::$allowedMethods); | |||
There was a problem hiding this comment.
You can work around with a closure function to avoid duplicated values ([lower|upper]case) in $allowedMethods:
$resolver->setAllowedValues('method', function ($value) {
return \in_array($value, self::$allowedMethods);
});|
My doubt at this point: |
|
@yceruto My first proposal on the issue was to remove the check entirely. You can find the reason for maintaining these checks in this comment |
|
@alekitto Yes, I agree, but this proposal open $this->createFormBuilder()->setMethod('RESET');why can't I do this too (without add a type extension): $this->createFormBuilder(null, ['method' => 'RESET']);? Note that the first alternative doesn't comply with the ref comment. |
|
@yceruto You've got a point. I'm still thinking that the best option is to remove the checks and suggest to always use |
|
@alekitto I bet on that too 👍 |
|
So you suggest one check in |
|
@vudaltsov I understood: Is there any restriction to use a custom request method in HTTP world? if no: IMHO, it is not worth sacrificing this natural possibility in favor of the DX misspelling feature. |
|
Technically the standard IIRC does not make any restriction on which HTTP methods you can use. There are some defined in the RFCs for which you are able to infer some semantics but that doesn't stop you from using your own methods. So maybe just removing the check here is the way to go. |
|
@xabbuh Correct. The RFC 7231 defines some method names (GET, HEAD, POST, PUT, DELETE, CONNECT, OPTIONS, TRACE) and others have been registered in different RFCs, but does not restrict method names at all. |
|
@alekitto Can you also add an entry to the component's changelog file? |
|
@xabbuh Ok, done 👍 |
| @@ -9,6 +9,7 @@ CHANGELOG | |||
| * deprecated calling `FormRenderer::searchAndRenderBlock` for fields which were already rendered | |||
| * added a cause when a CSRF error has occurred | |||
| * deprecated the `scale` option of the `IntegerType` | |||
| * removed restriction on allowed http methods | |||
|
The build failure is not related, but requires a fix on how we run tests on Travis CI (#28788 or something like that). |
|
Thank you @alekitto. |
…tion (alekitto) This PR was merged into the 4.2-dev branch. Discussion ---------- [Form] allow additional http methods in form configuration | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #26287 | License | MIT | Doc PR | TBD In order to allow HTTP methods other than GET, PUT, POST, DELETE and PATCH, the `allowed_methods` option under `framework.form` configuration has been added. This configuration option adds the specified methods to the `FormConfigBuilder` whitelist, allowing that methods be used in form configuration via `setMethod` or the `method` option. The use-case, that has been discussed in #26287, required the usage of custom HTTP method for describing a resource in an API application. Commits ------- 27d228c [Form] remove restriction on allowed http methods
In order to allow HTTP methods other than GET, PUT, POST, DELETE and PATCH, the
allowed_methodsoption underframework.formconfiguration has been added.This configuration option adds the specified methods to the
FormConfigBuilderwhitelist, allowing that methods be used in form configuration viasetMethodor themethodoption.The use-case, that has been discussed in #26287, required the usage of custom HTTP method for describing a resource in an API application.