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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convertible: set self.output in #render_all_layouts and #do_layout #5337

Merged
merged 1 commit into from Sep 8, 2016

Conversation

Projects
None yet
5 participants
@parkr
Member

parkr commented Sep 7, 2016

In #5308, I modified Convertible to essentially delegate to the Renderer. This removed a lot of duplicated code (馃樃) but introduced a slight difference: the Renderer doesn't really modify the document it's rendering鈥搃t's meant only to render and return the result. Sadly, then, the methods aren't entirely equivalent. This PR sets self.output in two places to ensure the old functionality is preserved.

Without this, the jekyll-sitemap gem barfed when rendering the source of jekyllrb.com. It uses the Page API's rather than the Renderer. Without these changes the site_map.output value is nil on the latest jekyll/jekyll master.

/cc @jekyll/stability

@parkr parkr added the fix label Sep 7, 2016

@jekyllbot jekyllbot assigned oe and parkr Sep 7, 2016

@parkr parkr assigned pathawks and unassigned parkr Sep 7, 2016

@parkr parkr referenced this pull request Sep 7, 2016

Closed

Jekyll HEAD has a breaking change that breaks some plugins. #5328

6 of 10 tasks complete
@parkr

This comment has been minimized.

Show comment
Hide comment
Member

parkr commented Sep 7, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Sep 8, 2016

Member

@envygeeks would you mind taking a 馃憖 ?

Member

parkr commented Sep 8, 2016

@envygeeks would you mind taking a 馃憖 ?

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Sep 8, 2016

Contributor

This is LGTM!

Contributor

envygeeks commented Sep 8, 2016

This is LGTM!

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Sep 8, 2016

Member

Thanks. :) LGTM.

Member

parkr commented Sep 8, 2016

Thanks. :) LGTM.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Sep 8, 2016

Member

@jekyllbot: merge +bug

Member

parkr commented Sep 8, 2016

@jekyllbot: merge +bug

@jekyllbot jekyllbot merged commit a70abbe into master Sep 8, 2016

2 of 3 checks passed

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

@jekyllbot jekyllbot added bug fix labels Sep 8, 2016

@jekyllbot jekyllbot deleted the fix-convertible-5308 branch Sep 8, 2016

jekyllbot added a commit that referenced this pull request Sep 8, 2016

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Sep 8, 2016

Member

馃帀

Member

pathawks commented Sep 8, 2016

馃帀

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