Draft: Remove safe filters #28
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
balazs-endresz
commented
Oct 5, 2022
| ] | ||
| ): | ||
| field.label = conditional_escape(field.label) | ||
| if getattr(settings, "TBXFORMS_ALLOW_HTML_LABEL", False): |
There was a problem hiding this comment.
Suggested change
| if getattr(settings, "TBXFORMS_ALLOW_HTML_LABEL", False): | |
| if field.label and getattr(settings, "TBXFORMS_ALLOW_HTML_LABEL", False): |
This might be still needed here, and for the help text below.
jams2
reviewed
Apr 11, 2023
| super().__init__(*args, **kwargs) | ||
| self.helper = FormHelper(self) | ||
|
|
||
| class BaseForm(TbxFormsBaseForm, forms.Form): |
There was a problem hiding this comment.
This change means 61 test failures that will require a lot of markup fixtures to be updated. I think that should be on another PR to keep the scope of this one manageable.
There was a problem hiding this comment.
Sure, it could be done separately first. But that might mean more work if all those have to be updated again for this PR.
Last time I looked at this I gave up when realising that we weren't using TbxFormsBaseForm in tests. That seemed like a pretty major issue.
olivierphi
added a commit
to olivierphi/tbxforms
that referenced
this pull request
Jan 4, 2024
kbayliss
pushed a commit
that referenced
this pull request
Feb 2, 2024
* Update test matrix - Add Python 3.12 - Remove Django 2.2, 3.0 and 3.1 - Add Django 4.1 and 4.2 * Make tests pass * Upgrade Flake8, so that it works on Python 3.12 too * Remove `|safe` filters See #28 * Add tests for HTML escaping
|
Closing because all the points here have been addressed in other PRs already. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Crispy forms has removed all uses of
|saferecently: django-crispy-forms/django-crispy-forms#296(except for
help_text: https://grep.app/search?q=%7Csafe&filter[repo][0]=django-crispy-forms/django-crispy-forms but we handle those differently, so we can remove|safefrom those too)This PR still needs more work but so far includes the following:
|safe, there's still a couple left thoughBaseFormintests/forms.pynow inherits fromTbxFormsBaseForm(I'm not sure how the tests worked like that)form.helper = FormHelper()removed from all tests, it should already exists if inheriting fromTbxFormsBaseFormTBXFORMS_ALLOW_HTML_BUTTON. See also notes in README why. Basically values for buttons can be very likely always marked as safe in python (unlike label and help text, which come from field definitions and can never contain user input). So we don't need an option for this.test_show_legend_as_heading_incorrect_escapingto make sure we don't render html where we're not supposed to.