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.

Sign up for GitHub

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

Jump to bottom

Append appropriate closing tag to Liquid block in an excerpt #6724

Merged
merged 5 commits into from Feb 20, 2018
Merged

Append appropriate closing tag to Liquid block in an excerpt #6724

merged 5 commits into from Feb 20, 2018

Conversation

ashmaroli
Copy link
Member

Resolves #5596

If a Liquid::Block tag subclass gets caught between the excerpt_separator, then append the concerned block delimiter to the excerpt to avoid a Liquid Exception due to invalid syntax

@ashmaroli ashmaroli added the fix label Jan 28, 2018
@ashmaroli ashmaroli requested review from parkr, pathawks, Crunch09 and a user January 28, 2018 19:24
@ghost
Copy link

ghost commented Jan 30, 2018 •
edited by ghost

Would it make sense having full Liquid blocks in excerpts in the first place? Assuming you had something like an if block in your excerpt, I'm fairly sure that would cause unexpected results for the user if the entire excerpt was suddenly hidden or something. Not sure whether that's intuitive behavior.

parkr
parkr reviewed Jan 31, 2018
View reviewed changes
Copy link
Member

@parkr parkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love this!!!!!!!

lib/jekyll/excerpt.rb Outdated
# append appropriate closing tag (to a Liquid block), to the "head" if the
# partitioning resulted in leaving the closing tag somewhere in the "tail"
# partition.
if head =~ %r!{%\s*(\w+).+\s*%}!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regexp's are very slow! Can we use head.include?("{%") before we do regexp? I think this runs on every page generation...

@ashmaroli
Copy link
Member Author

Would it make sense having full Liquid blocks in excerpts in the first place?

@oe IMO, Users are responsible for the "content" within their files.
If a user has multiple documents, each starting with an {% if %}, then its definitely something that needs to be moved into a layout.
On the other hand, if its one or two documents with a liquid block, the excerpt will render fine (via this patch).. the only case where it would be rendered empty is if {% capture %} had been used in which case, they may want to reconsider using the default excerpt_separator..

I don't think Jekyll should halt the build process in such cases.. If need be, Jekyll can issue a warning that a liquid block has been handled as required in the excerpt, and may not render as expected.. but that's for another PR..

@ghost
Copy link

ghost commented Jan 31, 2018

@ashmaroli Alright, fair enough.

Crunch09
Crunch09 approved these changes Feb 5, 2018
View reviewed changes
Copy link
Member

@Crunch09 Crunch09 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really good addition 👍

pathawks
pathawks requested changes Feb 7, 2018
View reviewed changes
Copy link
Member

@pathawks pathawks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this will only close one Liquid tag, is that correct?

Could we please output a warning if we ever do this? Users should be alerted so they can provide an explicit excerpt in cases where we truncate something that we shouldn't.

lib/jekyll/excerpt.rb Outdated
head =~ %r!{%\s*(\w+).+\s*%}!
tag_name = Regexp.last_match(1)

if liquid_block?(tag_name) && head.match(%r!{%\s*end#{tag_name}.+\s*%}!).nil?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's with the .+?

If is necessary, you can get rid of the \s*, since .+ will also swallow any whitespace.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch.. .+ is not needed since end-tags do not take argument(s)..

@ashmaroli
Copy link
Member Author

So this will only close one Liquid tag, is that correct?

yes. That is correct.. I consider excerpts that would contain multiple block-tags or nested block-tags as edge-cases which we shouldn't bother handling.. Let the build fail and leave it to the author to take appropriate action..

@pathawks
Copy link
Member

pathawks commented Feb 7, 2018

I wonder if we could have an option to fail the build when something like this happens.

I would rather the build fail than have a wonky site, but I understand that is maddening when not running Jekyll locally. This change will be an improvement for many users 👍

@ashmaroli
Copy link
Member Author

I wonder if we could have an option to fail the build

I disagree.. IMO, that'd be over-engineering..
Let Liquid decide how to go about based on its :error_mode

@pathawks
Copy link
Member

pathawks commented Feb 7, 2018

Let Liquid decide how to go about…

I mean instead of Jekyll quietly trying to patch things up, we could just let it fail. This is not over-engineering, it is our current behavior.

@ashmaroli
Copy link
Member Author

it is our current behavior

ohh.. I thought you were referring to the above mentioned edge-cases..
yeah.. having a config option say, smart_excerpts: true # false, by default would be nice..

@pathawks
Copy link
Member

pathawks commented Feb 7, 2018

Not necessary for this PR though 👍

@DirtyF DirtyF added this to the v3.8.0 milestone Feb 7, 2018
@ashmaroli ashmaroli mentioned this pull request Feb 9, 2018
Closed
18 tasks
@ghost ghost mentioned this pull request Feb 17, 2018
Closed
13 tasks
@ghost
Copy link

ghost commented Feb 20, 2018

@jekyllbot: merge +bug

@jekyllbot jekyllbot merged commit a662bc2 into jekyll:master Feb 20, 2018
jekyllbot added a commit that referenced this pull request Feb 20, 2018
@jekyllbot
Update history to reflect merge of #6724 [ci skip]
9262ff7
@ashmaroli ashmaroli deleted the excerpt-liquid-block-end branch February 20, 2018 14:51
@DirtyF DirtyF mentioned this pull request Mar 27, 2018
Closed
@DirtyF
Copy link
Member

DirtyF commented Apr 1, 2018

This prevents my blog from building, when I have Liquid in excerpts.

@ashmaroli ashmaroli mentioned this pull request Apr 1, 2018
Merged
@ashmaroli ashmaroli mentioned this pull request May 15, 2018
Merged
@jekyll jekyll locked and limited conversation to collaborators Jul 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@parkr parkr parkr left review comments

@pathawks pathawks pathawks approved these changes

@Crunch09 Crunch09 Crunch09 approved these changes

Assignees
No one assigned
Labels
fix frozen-due-to-age
Projects
None yet
Milestone
v3.8.0
Development

Successfully merging this pull request may close these issues.

Excerpt.extract_excerpt() fails when first paragraph contains part of a multi-line Liquid tag.
6 participants
@ashmaroli @pathawks @DirtyF @parkr @Crunch09 @jekyllbot