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

Remove tojson filter #2623

Closed
wants to merge 5 commits into from
Closed

Remove tojson filter #2623

wants to merge 5 commits into from

Conversation

greyli
Copy link
Member

@greyli greyli commented Feb 7, 2018

Jinja2 added the tojson filter in version 2.9. Now that we bumped the dependency's versions to the latest, maybe we should delete it in Flask.

@ThiefMaster
Copy link
Member

I somehow doubt that this logic still runs after your change: https://github.com/greyli/flask/blob/master/flask/json/__init__.py#L84-L100

@greyli
Copy link
Member Author

greyli commented Feb 7, 2018

@ThiefMaster It seems that the function you linked is used by jsonify(). Do you mean we should remove htmlsafe_dump() and htmlsafe_dumps()?

@ThiefMaster
Copy link
Member

No, what I mean is that your PR changes the behavior:

  • tojson_filter calls htmlsafe_dumps
  • htmlsafe_dumps calls htmlsafe_dumps
  • htmlsafe_dumps calls dumps
  • dumps calls _dump_arg_defaults, which is the function I mentioned in my last comment.

But the Jinja version of the filter does not do this! I see two options here:

  • Close this PR and leave it as-is - i.e. let flask replace the builtin Jinja filter as is the case right now, which works fine
  • Populate the json.dumps_function and json.dumps_kwargs policies of Jinja when setting up the Jinja environment in Flask.

But since _dump_arg_defaults depends on application state (e.g. the blueprint handling the current request) the second option is not really feasible without very nasty hacks, so I think we'll just keep the current implementation.

@ThiefMaster ThiefMaster closed this Feb 7, 2018
@greyli greyli deleted the greyli-remove-tojson-1 branch February 7, 2018 09:05
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants