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

allow optional use of a filter based on its existence #1248

Merged
merged 1 commit into from Apr 4, 2021

Conversation

amy-lei
Copy link
Contributor

@amy-lei amy-lei commented Jun 26, 2020

Resolves #842

  • Undefined filters nested in if/elif/else statements and conditional expressions will cause an error only if executed during runtime

Files changed:

  • nodes.py
  • compiler.py
  • runtime.py

src/jinja2/runtime.py Outdated Show resolved Hide resolved
@davidism davidism added this to the 3.0.0 milestone Mar 27, 2021
@amy-lei amy-lei changed the title #842: Optionally use a filter based on its existence allow optional use of a filter based on its existence Mar 30, 2021
src/jinja2/compiler.py Outdated Show resolved Hide resolved
@amy-lei amy-lei force-pushed the test-for-filter branch 3 times, most recently from c0fb589 to 541bf72 Compare April 1, 2021 23:29
@ThiefMaster
Copy link
Member

Are we covering cases like {{ 'foo'|nosuchfilter if false else 'bar' }} and {{ 'foo' if true else 'bar'|nosuchfilter }} - neither should raise (since the filter is not actually used, but based on the code it looks like it may raise..

@amy-lei
Copy link
Contributor Author

amy-lei commented Apr 2, 2021

Are we covering cases like {{ 'foo'|nosuchfilter if false else 'bar' }} and {{ 'foo' if true else 'bar'|nosuchfilter }} - neither should raise (since the filter is not actually used, but based on the code it looks like it may raise..

Right, I forgot to account for those but probably should (and will) too.

@ThiefMaster
Copy link
Member

I have the feeling any block-level check is likely to have problematic edge cases - can't we just let it fail when actually accessing the filter?

@amy-lei
Copy link
Contributor Author

amy-lei commented Apr 2, 2021

I have the feeling any block-level check is likely to have problematic edge cases - can't we just let it fail when actually accessing the filter?

Hmm that's true. The error won't be as helpful though: TypeError: 'NoneType' object is not callable

@ThiefMaster
Copy link
Member

Good point, that error is awful and filters that are sometimes but not always undefined are very much an edge case for which we probably shouldn't sacrifice error verboseness with other, more common, cases

@ThiefMaster
Copy link
Member

Could we compile filters to something like this at the beginning of the template?

try:
    filter_tmp_0 = self.filters['filter_name']
except KeyError:
    @internalcode
    def filter_tmp_0(*unused):
        raise TemplateRuntimeError("no filter named 'filter_name'")

Then the code where the filter is actually used just uses filter_tmp_0 which will either call the filter or raise an exception - without adding extra runtime overhead every time the filter is actually invoked.

@amy-lei
Copy link
Contributor Author

amy-lei commented Apr 2, 2021

Ooh that's a good idea! I moved it to pull_dependencies in the new commit. Let me know if I misunderstood. Will address conditional expressions next.

@davidism
Copy link
Member

davidism commented Apr 4, 2021

I'm working on a separate PR to create an if 'name' is filter test.

@davidism davidism merged commit af5d80e into pallets:master Apr 4, 2021
@davidism davidism deleted the test-for-filter branch April 4, 2021 17:20
@davidism davidism mentioned this pull request Apr 5, 2021
6 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 19, 2021
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.

optionally use a filter based on its existence
3 participants