Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Form] Changing CSRF token default name#60537
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM. No comment.
Is it worth an entry in CHANGELOG? If yes, as bug or feature? |
On my side, I think it's fine merging as "minor". Let's see if anyone else thinks different. |
I am not convinced. This has the potential to break applications where people render forms by referencing the CSRF field explicitly with the „old“ name. |
I think this deserves being mentioned in the changelog, to account for the fact it might impact projects accessing the field by name. Note that this PR does not change the name for users of the fullstack framework as you haven't changed the default value in the FrameworkBundle config. |
| @@ -253,7 +253,7 @@ private function addFormSection(ArrayNodeDefinition $rootNode, callable $enableI | |||
| ->children() | |||
| ->scalarNode('enabled')->defaultNull()->end() // defaults to framework.csrf_protection.enabled | |||
| ->scalarNode('token_id')->defaultNull()->end() | |||
| ->scalarNode('field_name')->defaultValue('_token')->end() | |||
| ->scalarNode('field_name')->defaultValue('_csrf_token')->end() | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I am still 👎 on this change as I don't think it's worth it potentially breaking applications just to change the name of this field.
If we really want to change the default name we should IMO follow the usual deprecation-first approach: Meaning here that not setting the value explicitly would trigger a deprecation which would then allow us to change the default in 8.0.
Thanks, I now changed it there too (and in the tests), and created a |
Hi, I tested this on a project where the field is rendered manually, and as@xabbuh pointed out, it does break the token verification. |
Let's close then. Thanks for proposing@ThomasLandauer but this looks like too costly for too little benefits. |
See#60534 (comment)