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

Fix forms & csrf_enabled deprecation doc #287

Conversation

JocelynDelalande
Copy link
Contributor

I had hard times understanding the deprecation of csrf_enabled. The only way I managed to do it was by reading form.py.

Let's make the doc and deprecation message better :-)

Mention the non-depredacted way to disable CSRF for a given form.
@JocelynDelalande JocelynDelalande force-pushed the jd-fix-form-csrf-enabled-deprecation-doc branch from e38d90d to a919882 Compare March 29, 2017 12:44
@codecov-io
Copy link

codecov-io commented Mar 29, 2017

Codecov Report

Merging #287 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #287      +/-   ##
==========================================
- Coverage   99.66%   99.66%   -0.01%     
==========================================
  Files          19       18       -1     
  Lines         897      895       -2     
  Branches       74       74              
==========================================
- Hits          894      892       -2     
  Misses          3        3
Impacted Files Coverage Δ
flask_wtf/form.py 100% <ø> (ø) ⬆️
tests/test_form.py 100% <100%> (ø) ⬆️
flask_wtf/__init__.py 100% <0%> (ø) ⬆️
flask_wtf/recaptcha/__init__.py

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eff54ec...b8ff4f9. Read the comment docs.

@JocelynDelalande
Copy link
Contributor Author

oops, forgot to fix the associated test, that's done by now :-)

JocelynDelalande added a commit to JocelynDelalande/ihatemoney that referenced this pull request Mar 29, 2017
Copy link
Member

@davidism davidism left a comment

Choose a reason for hiding this comment

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

I think I meant to show setting Meta.csrf = False while defining the form, rather than passing the override, since you probably don't want a form that is for both CSRF and non-CSRF contexts.

@JocelynDelalande
Copy link
Contributor Author

Thanks for reviews!

I think I meant to show setting Meta.csrf = False while defining the form, rather than passing the override, since you probably don't want a form that is for both CSRF and non-CSRF contexts.

@davidism I don't understand what you mean. Could you provide an example please :-) ?

@davidism
Copy link
Member

class MyForm(FlaskForm):
    class Meta:
        csrf = False

form = MyForm()

@JocelynDelalande
Copy link
Contributor Author

@davidism ok, thanks :-). So, from my understanding, there are two ways to disable CSRF for a form

  • the one you are speaking (with Meta), is defining the default at class-level (set the default for a given form class).
  • the one that I'm dealing with (through warning message) is to disable CSRF at instanciation time ; thus overriding what could have been done at class-level, so it's instance-level

The warning where I changed the message is clearly for an instance-level option (it's a constructor parameter), so it seems the right one to put here. @davidism What do you think ?

Side note: is the Meta thing for forms documented somewhere ?

@davidism
Copy link
Member

davidism commented Mar 31, 2017

As I said in the review, I meant the warning to refer to the meta attribute, as it makes more sense to design a form that never has CSRF than to have to decide on the fly. I just forgot to capitalize Meta and change the example in the docs.

@JocelynDelalande
Copy link
Contributor Author

As I said in the review, I meant the warning to refer to the meta attribute,

I got that :)

as it makes more sense to design a form that never has CSRF than to have to decide on the fly.

Depends. If this deprecation warning is reached, it's in the context of a code that decides it "on the fly". So suggestin a "drop-in" replacement could be useful to the developper.

Would mentioning both solutions in the warning (meta= arg and Meta class) looks good to you ?

@davidism
Copy link
Member

davidism commented Mar 31, 2017

Fair enough, I agree with you about the context of this message. Let's change it to "Pass meta={'csrf': False} instead." to be more explicit about what to do.

@JocelynDelalande JocelynDelalande force-pushed the jd-fix-form-csrf-enabled-deprecation-doc branch from a919882 to b61f9d6 Compare March 31, 2017 16:21
@JocelynDelalande
Copy link
Contributor Author

Fair enough, I agree with you about the context of this message. Let's change it to "Pass meta={'csrf': False} instead." to be more explicit about what to do.

@davidism Ok, I just updated my PR accordingly.

I let someone else document the Meta behavior in another PR/commit at the place where you find it relevant.

docs/form.rst Outdated
@@ -11,7 +11,7 @@ form with csrf protection. We encourage you do nothing.

But if you want to disable the csrf protection, you can pass::

form = FlaskForm(csrf_enabled=False)
form = FlaskForm(meta={'csrf_enabled': False})
Copy link
Member

Choose a reason for hiding this comment

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

Should be 'csrf' here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well spotted, thanks, fixed.

@JocelynDelalande JocelynDelalande force-pushed the jd-fix-form-csrf-enabled-deprecation-doc branch from b61f9d6 to b8ff4f9 Compare March 31, 2017 17:18
It was speaking about meta.csrf which is pretty much unclear :

- what is this `meta` ?
- suggests that `csrf` is an attribute of `meta` (which is wrong: it is a dict
  key)
@davidism davidism merged commit eb7859d into wtforms:master Mar 31, 2017
Jojo144 pushed a commit to Jojo144/ihatemoney that referenced this pull request Mar 21, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants