Enable strict (or lax) liquid parsing via a config variable. #5053

Merged
merged 1 commit into from Jul 15, 2016

Conversation

Projects
None yet
4 participants
@stevecheckoway
Contributor

stevecheckoway commented Jul 3, 2016

Insert

liquid:
  error_mode: strict # or lax or warn

in _config.yml to change the error mode. #4372

Enable strict (or lax) liquid parsing.
Insert

liquid:
  error_mode: strict # or lax or warn

in _config.yml to change the error mode.
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 5, 2016

Member

Hey @stevecheckoway, thanks for the pull request. We try not to change configuration settings too often as they cause problems for folks unnecessarily. Can you help explain why you submitted this pull request?

Member

parkr commented Jul 5, 2016

Hey @stevecheckoway, thanks for the pull request. We try not to change configuration settings too often as they cause problems for folks unnecessarily. Can you help explain why you submitted this pull request?

@stevecheckoway

This comment has been minimized.

Show comment
Hide comment
@stevecheckoway

stevecheckoway Jul 5, 2016

Contributor

I find having strict liquid parsing helpes diagnose errors. In general, liquid warnings aren't shown to the user (and I failed to fix that). This makes it easy to see those earnings (as errors) without changing the default behavior.

Contributor

stevecheckoway commented Jul 5, 2016

I find having strict liquid parsing helpes diagnose errors. In general, liquid warnings aren't shown to the user (and I failed to fix that). This makes it easy to see those earnings (as errors) without changing the default behavior.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jul 5, 2016

Contributor

I've personally had a need for this before, I thought we enabled strict by default now? Or maybe it was removed because there were problems, can't quite remember.

Contributor

envygeeks commented Jul 5, 2016

I've personally had a need for this before, I thought we enabled strict by default now? Or maybe it was removed because there were problems, can't quite remember.

@stevecheckoway

This comment has been minimized.

Show comment
Hide comment
@stevecheckoway

stevecheckoway Jul 5, 2016

Contributor

:warn is the default. It uses the same strict parsing but records the first error in the Template object and then attempts :lax parsing. The warnings are ignored and I haven't had the opportunity look into why in detail.

Mailing strict parsing the default would certainly work for me.

Contributor

stevecheckoway commented Jul 5, 2016

:warn is the default. It uses the same strict parsing but records the first error in the Template object and then attempts :lax parsing. The warnings are ignored and I haven't had the opportunity look into why in detail.

Mailing strict parsing the default would certainly work for me.

@stevecheckoway

This comment has been minimized.

Show comment
Hide comment
@stevecheckoway

stevecheckoway Jul 6, 2016

Contributor

Actually, I was incorrect. I believed the Liquid documentation:

      # Sets how strict the parser should be.
      # :lax acts like liquid 2.5 and silently ignores malformed tags in most cases.
      # :warn is the default and will give deprecation warnings when invalid syntax is used.
      # :strict will enforce correct syntax.
      attr_writer :error_mode

but in reality

      def error_mode
        @error_mode || :lax
      end

It's not so hard to log the warnings by setting :warn and then plumbing the warnings through Jekyll::LiquidRender::File. I created a branch that does just that. I can create a PR for that, if desired.

Regardless of displaying the warnings or not, I still think it's useful to be able to make the errors fatal with :strict.

Contributor

stevecheckoway commented Jul 6, 2016

Actually, I was incorrect. I believed the Liquid documentation:

      # Sets how strict the parser should be.
      # :lax acts like liquid 2.5 and silently ignores malformed tags in most cases.
      # :warn is the default and will give deprecation warnings when invalid syntax is used.
      # :strict will enforce correct syntax.
      attr_writer :error_mode

but in reality

      def error_mode
        @error_mode || :lax
      end

It's not so hard to log the warnings by setting :warn and then plumbing the warnings through Jekyll::LiquidRender::File. I created a branch that does just that. I can create a PR for that, if desired.

Regardless of displaying the warnings or not, I still think it's useful to be able to make the errors fatal with :strict.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 7, 2016

Member

I am OK with this, then, as long as it doesn't cause trouble for current users. What does a warning message look like?

Member

parkr commented Jul 7, 2016

I am OK with this, then, as long as it doesn't cause trouble for current users. What does a warning message look like?

@parkr parkr added the enhancement label Jul 7, 2016

@parkr parkr added this to the undetermined milestone Jul 7, 2016

@stevecheckoway

This comment has been minimized.

Show comment
Hide comment
@stevecheckoway

stevecheckoway Jul 7, 2016

Contributor

I simply copied the formatting of the error messages:

    Liquid Warning: Liquid syntax error (line 13): Expected end_of_string but found pipe in "a in b | c" in index.md
    Liquid Warning: Liquid syntax error (line 70): Expected end_of_string but found pipe in "a in b | c" in _layouts/index.html

The line numbers are off by the number of lines in the YAML front matter, but I haven't had a chance to investigate fixing that. If that number is available somewhere, then it'd be a simple matter to check if the error message is a Liquid::Error and increment the line number accordingly. I haven't had a chance to look into that yet, but it's probably a simple matter to modify document.rb to store the number of front matter lines.

Just as an aside, is Jekyll::Tags::IncludeTagError used anywhere? If not, it could be removed and the error formatting code simplified.

Contributor

stevecheckoway commented Jul 7, 2016

I simply copied the formatting of the error messages:

    Liquid Warning: Liquid syntax error (line 13): Expected end_of_string but found pipe in "a in b | c" in index.md
    Liquid Warning: Liquid syntax error (line 70): Expected end_of_string but found pipe in "a in b | c" in _layouts/index.html

The line numbers are off by the number of lines in the YAML front matter, but I haven't had a chance to investigate fixing that. If that number is available somewhere, then it'd be a simple matter to check if the error message is a Liquid::Error and increment the line number accordingly. I haven't had a chance to look into that yet, but it's probably a simple matter to modify document.rb to store the number of front matter lines.

Just as an aside, is Jekyll::Tags::IncludeTagError used anywhere? If not, it could be removed and the error formatting code simplified.

@parkr

This comment has been minimized.

Show comment
Hide comment
Member

parkr commented Jul 15, 2016

LGTM.

@jekyll/core?

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jul 15, 2016

Contributor

LGTM.

Contributor

envygeeks commented Jul 15, 2016

LGTM.

@parkr parkr changed the title from Enable strict (or lax) liquid parsing. to Enable strict (or lax) liquid parsing via a config variable. Jul 15, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 15, 2016

Member

@jekyllbot: merge +minor

Member

parkr commented Jul 15, 2016

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 4dee1f6 into jekyll:master Jul 15, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
jekyll/lgtm Approved by @parkr and @envygeeks.

jekyllbot added a commit that referenced this pull request Jul 15, 2016

@stevecheckoway stevecheckoway deleted the stevecheckoway:strict-liquid branch Jul 24, 2016

stevecheckoway added a commit to stevecheckoway/jekyll that referenced this pull request Jul 24, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment