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

Guard against type error in absolute url #6280

Merged
merged 4 commits into from Aug 10, 2017

Conversation

Projects
None yet
3 participants
@benbalter
Contributor

benbalter commented Aug 8, 2017

If a user sets an image hash in a post's YAML front matter, and that hash does not contain a path key, this line in Jekyll Feed will pass the full image hash to the absolute_url filter.

Via #6185 and #6253 we normalize the baseurl and site.url inputs, but we don't normalize the argument passed to the absolute_url filter (we normalize the input passed to relative_url), meaning if you pass anything other than a string, it method will raise a TypeError when it tries to determine if the non-url is absolute.

This PR calls an explicit to_s on the input, seeking to return a malformed URL, rathering than breaking the build (which is often opaque due to a single post missing a post.image.path field raising an error in generating feed.xml).

benbalter added some commits Aug 8, 2017

@parkr

parkr approved these changes Aug 9, 2017

Code looks solid. 👍

This kind of error is nice; it provides the user with the information that the url they provided is complete bogus – "whoops, gotta fix that!" It calls them to action in a way that suppressing this might not. Do you agree that we're losing value there?

@parkr parkr added the fix label Aug 9, 2017

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter Aug 9, 2017

Contributor

it provides the user with the information that the url they provided is complete bogus... It calls them to action

@parkr see the bug report in jekyll/jekyll-feed#178. Given that it took some of Jekyll's top contributors 3+ weeks to determine the root cause of the opaque Can't convert Hash into String. in feed.xml error (with no way of knowing which post caused it), I'm not sure how much value raising the error provides.

An alternative approach would be a more descriptive error with page/post-level backtrace, but that's beyond what I'm able to invest right now.

Contributor

benbalter commented Aug 9, 2017

it provides the user with the information that the url they provided is complete bogus... It calls them to action

@parkr see the bug report in jekyll/jekyll-feed#178. Given that it took some of Jekyll's top contributors 3+ weeks to determine the root cause of the opaque Can't convert Hash into String. in feed.xml error (with no way of knowing which post caused it), I'm not sure how much value raising the error provides.

An alternative approach would be a more descriptive error with page/post-level backtrace, but that's beyond what I'm able to invest right now.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 10, 2017

Member

@jekyllbot: merge +fix

Member

parkr commented Aug 10, 2017

@jekyllbot: merge +fix

@jekyllbot jekyllbot merged commit 045226f into master Aug 10, 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 deleted the guard-against-type-error-in-absolute-url branch Aug 10, 2017

@jekyllbot jekyllbot added bug fix labels Aug 10, 2017

jekyllbot added a commit that referenced this pull request Aug 10, 2017

parkr added a commit that referenced this pull request Aug 10, 2017

parkr added a commit that referenced this pull request Aug 10, 2017

parkr added a commit that referenced this pull request Aug 10, 2017

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 10, 2017

Member

Backporting to 3.5 targeting a 3.5.2.

Member

parkr commented Aug 10, 2017

Backporting to 3.5 targeting a 3.5.2.

@parkr parkr referenced this pull request Aug 10, 2017

Closed

Release v3.5.2 #6292

6 of 6 tasks complete

parkr added a commit that referenced this pull request Aug 10, 2017

Merge pull request #6287 from jekyll/3.5-stable-backport-6280
Backport to 3.5: Guard against type error in absolute url (#6280)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment