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

Static files contain front matter default keys when to_liquid'd #6162

Merged
merged 4 commits into from Jul 1, 2017

Conversation

Projects
None yet
4 participants
@benbalter
Contributor

benbalter commented Jun 20, 2017

Trying to track down jekyll/jekyll-sitemap#176, it seems in 3.5, when you call to_liquid on a convertible (page, static_file), the resulting hash doesn't contain front matter default keys, although calling a specific key returns the expected value. This means that things like to_json or anything that looks for .key? will not act as expected.

This PR currently contains a failing test in hopes of finding the root cause (and adding tests for front matter defaults going forward).

/cc @parkr

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jun 20, 2017

Member

the resulting hash doesn't contain front matter default keys, although calling a specific key returns the expected value. This means that things like to_json or anything that looks for .key? will not act as expected.

The key resolves because we inherit the default_proc. It makes more sense to get all frontmatter defaults for the given page we're processing and do Utils.deep_merge_hashes(defaults, frontmatter).

Member

parkr commented Jun 20, 2017

the resulting hash doesn't contain front matter default keys, although calling a specific key returns the expected value. This means that things like to_json or anything that looks for .key? will not act as expected.

The key resolves because we inherit the default_proc. It makes more sense to get all frontmatter defaults for the given page we're processing and do Utils.deep_merge_hashes(defaults, frontmatter).

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter Jun 20, 2017

Contributor

It makes more sense to get all frontmatter defaults for the given page we're processing and do Utils.deep_merge_hashes(defaults, frontmatter).

I thought we were doing that via https://github.com/jekyll/jekyll/blob/master/lib/jekyll/convertible.rb#L122-L123?

Contributor

benbalter commented Jun 20, 2017

It makes more sense to get all frontmatter defaults for the given page we're processing and do Utils.deep_merge_hashes(defaults, frontmatter).

I thought we were doing that via https://github.com/jekyll/jekyll/blob/master/lib/jekyll/convertible.rb#L122-L123?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jun 20, 2017

Member

That looks like it should work for Page and Layout, but Document and StaticFile don't use Convertible.

Member

parkr commented Jun 20, 2017

That looks like it should work for Page and Layout, but Document and StaticFile don't use Convertible.

@benbalter benbalter changed the title from WIP: Ensure convertibles contain front matter default keys when to_liquid'd to WIP: Ensure drop-based objects contain front matter default keys when to_liquid'd Jun 22, 2017

benbalter added some commits Jun 22, 2017

Show outdated Hide outdated lib/jekyll/static_file.rb
@@ -104,7 +100,7 @@ def to_liquid
end
def data
@data ||= {}
@data ||= @site.frontmatter_defaults.all(relative_path, type)

This comment has been minimized.

@parkr

parkr Jun 22, 2017

Member

This isn't guaranteed to run if @data is set by some other process. Should we just do this in #initialize?

@parkr

parkr Jun 22, 2017

Member

This isn't guaranteed to run if @data is set by some other process. Should we just do this in #initialize?

This comment has been minimized.

@benbalter

benbalter Jun 29, 2017

Contributor

Good catch. Implemented via e9c691e.

@benbalter

benbalter Jun 29, 2017

Contributor

Good catch. Implemented via e9c691e.

@benbalter benbalter changed the title from WIP: Ensure drop-based objects contain front matter default keys when to_liquid'd to Static files contain front matter default keys when to_liquid'd Jun 29, 2017

@pathawks pathawks added this to the 3.5.1 milestone Jul 1, 2017

@pathawks pathawks requested a review from parkr Jul 1, 2017

@parkr

parkr approved these changes Jul 1, 2017

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 1, 2017

Member

@jekyllbot: merge +bug

Member

parkr commented Jul 1, 2017

@jekyllbot: merge +bug

@jekyllbot jekyllbot merged commit f91b614 into master Jul 1, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jekyllbot jekyllbot added bug fix labels Jul 1, 2017

@jekyllbot jekyllbot deleted the static-file-front-matter-defaults branch Jul 1, 2017

jekyllbot added a commit that referenced this pull request Jul 1, 2017

cbeams added a commit to bisq-network/bisq-website that referenced this pull request Aug 6, 2017

Set sitemap: false in defaults
This change tests whether the fix in jekyll/jekyll#6162 actually
addresses the problem described in jekyll/jekyll-sitemap#180, and finds
that it does _not_ fix the problem.

The problem is that binary files (pdf files in our case) do not inherit
`sitemap: false` like other Jekyll page types do.

The test scenario was simple:

 1. Upgrade to Jekyll 3.5.1, the first version that contains the changes
 in jekyll/jekyll#6162.

 2. Prior to the _config.yml changes in this commit, generate
 _site/sitemap.xml by running `bundle exec jekyll build`. This produces
 the expected result of a complete sitemap.xml wherein all pages are
 included by default.

 3. Copy _site/sitemap.xml => sitemap-before.xml

 4. Apply the changes in this commit and run `bundle exec jekyll build`
 once again. The expected result is an empty sitemap.xml, but the actual
 result is a sitemap.xml that contains no entries except for binary
 files.

 4. Copy _site/sitemap.xml => sitemap-after.xml

 5. `sdiff -w 120 sitemap-before.xml sitemap-after.xml` and paste the
 results into a comment on jekyll/jekyll-sitemap#180.

DirtyF added a commit to DirtyF/jekyll-sitemap that referenced this pull request Aug 26, 2017

@DirtyF DirtyF referenced this pull request Aug 26, 2017

Closed

Bump Travis configuration #187

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