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

Invalid filters are ignored without warning #422

Open
tadrinth opened this issue Aug 17, 2014 · 12 comments
Open

Invalid filters are ignored without warning #422

tadrinth opened this issue Aug 17, 2014 · 12 comments

Comments

@tadrinth
Copy link

@tadrinth tadrinth commented Aug 17, 2014

Using a filter that Liquid does not recognize causes the invalid filter to be ignored and no warning will be generated.

It would be nice if there was a way to get a warning in this case, instead of these errors being silently ignored. I was expected the strict error mode to catch invalid filters upon parse, and I think that would be the ideal solution.

@fw42

This comment has been minimized.

Copy link
Contributor

@fw42 fw42 commented Aug 17, 2014

makes sense to me, @trishume?

@tadrinth

This comment has been minimized.

Copy link
Author

@tadrinth tadrinth commented Aug 17, 2014

Looks like you would want to do the check on line 74 of variable.rb, but you would need a Strainer available to do the check. Seems like you could create a Strainer in Template.parse and merge it into the options that get passed around.

@tadrinth

This comment has been minimized.

Copy link
Author

@tadrinth tadrinth commented Aug 17, 2014

Also, I see error handling in variable.render that looks like it should be complaining about unrecognized filters, but I'm not hitting it in my tests:
source = "{{ e | hippoasdfds }}"; t = ::Liquid::Template.parse(source, :error_mode => :strict); t.render
=> ""

@trishume

This comment has been minimized.

Copy link
Contributor

@trishume trishume commented Aug 18, 2014

Yah it would be great if this errored under strict mode, the only problem might be when people add filters between render and parse the errors would have to come during parse, and there is currently no framework for strict mode rendering.

So yes this should be implemented, but it's harder than it might seem.

@trishume

This comment has been minimized.

Copy link
Contributor

@trishume trishume commented Aug 19, 2014

Wow, @tadrinth the filter not found code you noticed can not be called since the FilterNotFound error is never used anywhere else. #LiquidWAT

Unknown filters are defined in the tests as doing nothing, but I'll look into erroring in strict mode.

@carsonreinke

This comment has been minimized.

Copy link
Contributor

@carsonreinke carsonreinke commented Aug 26, 2014

So, does their need to be PR for missing filters in render under strict mode?

@tadrinth

This comment has been minimized.

Copy link
Author

@tadrinth tadrinth commented Aug 26, 2014

Been thinking about this, and I think the best solution is an additional 'strict filters' flag that could be passed to parse. Render may also need a strict mode, but in the case where you know you have defined all your filters in advance of parsing, you should be able to catch errors at the parse step. When I'm validating a liquid template that someone has entered, I don't want to have to render the liquid with dummy data to validate that the filters are correct. I just want to parse it.

If the bad filters cannot be detected during strict parsing, then I would want to be able to detect bad filters even if this particular strict render does not encounter them. IE, if I have IF THEN bar ELSE END, and I render the template with foo=true, the bad filter will be skipped, but I still want to be warned about it. That seems like it would be very difficult to implement, though; detecting that error during parse would be way easier.

Adding a filter error mode seems like it would be a much smaller change than adding a strict render mode, as well.

@trishume

This comment has been minimized.

Copy link
Contributor

@trishume trishume commented Aug 26, 2014

@tadrinth that sounds like a good idea, it would certainly be a step towards being able to know that if your liquid template parses, then it will run correctly.

@carsonreinke

This comment has been minimized.

Copy link
Contributor

@carsonreinke carsonreinke commented Sep 21, 2014

@fw42

This comment has been minimized.

Copy link
Contributor

@fw42 fw42 commented Sep 21, 2014

@dylanahsmith

This comment has been minimized.

Copy link
Member

@dylanahsmith dylanahsmith commented Sep 22, 2014

It was from reading this issue that I found out that FilterNotFoundError wasn't used. I meant to link to this issue from #448 but must have forgotten to do that.

@amw

This comment has been minimized.

Copy link

@amw amw commented Jun 20, 2016

Just wasted 30 minutes figuring out why a filter I tried to use didn't generate expected results. Turns out it was not implemented in the version of Jekyll that I used (only on master, not released as gem yet).

🤔😮😒

Silently ignoring a missing filter is definitely not nice...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.