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

Disable "http_method_override" by default #892

Merged
1 commit merged into from
Feb 10, 2021
Merged

Disable "http_method_override" by default #892

1 commit merged into from
Feb 10, 2021

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Feb 7, 2021

Q A
License MIT
Doc issue/PR symfony/symfony-docs#14939

When this is turned on (the default) one can hit a route that accepts only PUT, but with a POST.
Shouldn't we disable this by default, as it's not that commonly needed, isn't it?

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request passes validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request passes validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request passes validation.

OskarStark added a commit to symfony/symfony-docs that referenced this pull request Feb 8, 2021
This PR was merged into the 4.4 branch.

Discussion
----------

Clarify the role of http_method_override

See also symfony/recipes#892

Commits
-------

91ecbda Clarify the role of http_method_override
@dunglas
Copy link
Member

dunglas commented Feb 8, 2021

I'm in favor of this change but shouldn't we target only 5.3?

@xabbuh
Copy link
Member

xabbuh commented Feb 9, 2021

We need to be careful here as such a change means that applications simulating DELETE requests won't work anymore. I don't remember if EasyAdminBundle, for example, uses them by default (@javiereguiluz any idea about this?).

@javiereguiluz
Copy link
Member

@xabbuh thanks for the ping! Yes, we did that in the past, but we now use a normal form with POST method and a CSRF token (https://github.com/EasyCorp/EasyAdminBundle/blob/423c7f3b2dfd8fd3204bbe43a97390c4169d1d39/src/Resources/views/crud/includes/_delete_form.html.twig#L2-L4). This is the same as we do in the Symfony Demo app.

@xabbuh
Copy link
Member

xabbuh commented Feb 9, 2021

Thanks for the reply Javier! What about CMS like solutions like Sulu?

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request passes validation.

@nicolas-grekas
Copy link
Member Author

I'm in favor of this change but shouldn't we target only 5.3?

Updated, for 5.3 only it is now.

applications simulating DELETE requests won't work anymore.

This won't break any apps since that's only for new apps.

What about CMS like solutions like Sulu?

They already ship their own framework.yaml, so that flex won't override it.
https://github.com/sulu/skeleton/blob/2.x/config/packages/framework.yaml

@ghost ghost merged commit d5faaca into symfony:master Feb 10, 2021
javiereguiluz added a commit to symfony/demo that referenced this pull request Feb 17, 2021
This PR was merged into the main branch.

Discussion
----------

Disable HTTP method override

We don't need this since the changes introduced in #427 and this goes in sync with the official Symfony recipe: symfony/recipes#892

Commits
-------

47eb3d9 Disable HTTP method override
weaverryan added a commit to symfony/maker-bundle that referenced this pull request Mar 2, 2021
This PR was merged into the 1.0-dev branch.

Discussion
----------

[make:crud] use POST method instead of DELETE

Starting in Symfony 5.3, `framework.http_method_override` defaults to `false`. This prevents CRUD form submission for `PUT`, `PATCH` &/or `DELETE` methods.  See symfony/recipes#892

~This PR conditionally sets  `http_method_override` to true when using `make:crud` if using Symfony 5.3+~

We stop using the `DELETE` method override in favor of using `POST` on the delete operation.

Commits
-------

fc9cd2a [make:crud] use post method for deletes
@nicolas-grekas nicolas-grekas deleted the nohttp_method_override branch March 17, 2021 08:10
sayjun0505 added a commit to sayjun0505/sym_proj that referenced this pull request Apr 16, 2023
This PR was merged into the main branch.

Discussion
----------

Disable HTTP method override

We don't need this since the changes introduced in #427 and this goes in sync with the official Symfony recipe: symfony/recipes#892

Commits
-------

47eb3d9 Disable HTTP method override
mwhorse46 added a commit to mwhorse46/sym_proj that referenced this pull request Apr 16, 2023
This PR was merged into the main branch.

Discussion
----------

Disable HTTP method override

We don't need this since the changes introduced in #427 and this goes in sync with the official Symfony recipe: symfony/recipes#892

Commits
-------

47eb3d9 Disable HTTP method override
lchrusciel added a commit to Sylius/SyliusResourceBundle that referenced this pull request Sep 9, 2023
…425)

This PR was merged into the 1.11 branch.

Discussion
----------

| Q               | A
| --------------- | -----
| Bug fix?        | yes
| New feature?    | no
| BC breaks?      | no
| Deprecations?   | no
| Related tickets |
| License         | MIT

1/ Delete path name can have some conflicts depending on operation sorting.
On previous routing system, these routing sorting system was hard-coded.
If we place the Bulk delete operation after the delete operation, the bulk delete is broken due to path conflicts.

2/ On current version of Symfony, http method overrides is not enabled by default and then these paths have another issue with same path for delete and edit operation (with POST method).
symfony/recipes#892

Before
```
app_backend_contact_create               GET|POST        ANY      ANY    /admin/contacts/new                             
app_backend_contact_update               GET|PUT         ANY      ANY    /admin/contacts/{id}/edit                       
app_backend_contact_bulk_delete          DELETE          ANY      ANY    /admin/contacts/bulk_delete                     
app_backend_contact_delete               DELETE          ANY      ANY    /admin/contacts/{id}                          
app_backend_contact_show                 GET             ANY      ANY    /admin/contacts/{id}                            
app_backend_contact_index                GET             ANY      ANY    /admin/contacts 
```

After
```
app_backend_contact_create               GET|POST        ANY      ANY    /admin/contacts/new                             
app_backend_contact_update               GET|PUT         ANY      ANY    /admin/contacts/{id}/edit                       
app_backend_contact_bulk_delete          DELETE          ANY      ANY    /admin/contacts/bulk_delete                     
app_backend_contact_delete               DELETE          ANY      ANY    /admin/contacts/{id}/delete                           
app_backend_contact_show                 GET             ANY      ANY    /admin/contacts/{id}                            
app_backend_contact_index                GET             ANY      ANY    /admin/contacts 
```

Commits
-------

2222054 Update delete path name to avoid route conflicts
c59467d Fix PHPUnit tests
GSadee pushed a commit to Sylius/Resource that referenced this pull request Feb 2, 2024
…425)

This PR was merged into the 1.11 branch.

Discussion
----------

| Q               | A
| --------------- | -----
| Bug fix?        | yes
| New feature?    | no
| BC breaks?      | no
| Deprecations?   | no
| Related tickets |
| License         | MIT

1/ Delete path name can have some conflicts depending on operation sorting.
On previous routing system, these routing sorting system was hard-coded.
If we place the Bulk delete operation after the delete operation, the bulk delete is broken due to path conflicts.

2/ On current version of Symfony, http method overrides is not enabled by default and then these paths have another issue with same path for delete and edit operation (with POST method).
symfony/recipes#892

Before
```
app_backend_contact_create               GET|POST        ANY      ANY    /admin/contacts/new                             
app_backend_contact_update               GET|PUT         ANY      ANY    /admin/contacts/{id}/edit                       
app_backend_contact_bulk_delete          DELETE          ANY      ANY    /admin/contacts/bulk_delete                     
app_backend_contact_delete               DELETE          ANY      ANY    /admin/contacts/{id}                          
app_backend_contact_show                 GET             ANY      ANY    /admin/contacts/{id}                            
app_backend_contact_index                GET             ANY      ANY    /admin/contacts 
```

After
```
app_backend_contact_create               GET|POST        ANY      ANY    /admin/contacts/new                             
app_backend_contact_update               GET|PUT         ANY      ANY    /admin/contacts/{id}/edit                       
app_backend_contact_bulk_delete          DELETE          ANY      ANY    /admin/contacts/bulk_delete                     
app_backend_contact_delete               DELETE          ANY      ANY    /admin/contacts/{id}/delete                           
app_backend_contact_show                 GET             ANY      ANY    /admin/contacts/{id}                            
app_backend_contact_index                GET             ANY      ANY    /admin/contacts 
```

Commits
-------

22220546a709117a2a39fa39ee6d059faf26cfc2 Update delete path name to avoid route conflicts
c59467d660a7845d2ced23d43c90aa105e94299f Fix PHPUnit tests
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants