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.

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

Call to_s on site.url before attempting to concatenate strings #6253

Merged
merged 2 commits into from Jul 30, 2017

Conversation

benbalter
Copy link
Contributor

We call to_s before we call site.baseurl, but we don't call to_s before we call site.url, meaning if site.url is something other than a string, things blow up.

The specific use case for the to_s here, other than users that set url to something else, is the ability to stub site.url with a Proc that would lazily calculate the URL when called.

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.

Rubocop thinks the line is too long.
Otherwise LGTM 👍

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.

Test?

@parkr
Copy link
Member

parkr commented Jul 30, 2017

In test/test_filters.rb there should be a section for URL-related filters. Throw in something that would break Addressable::URI.parse and just make sure it ends up as a string. I would recommend using URI or some class that has a built-in to_s, or you could build a tiny custom class for this purpose. If you're not able to get to writing a test, I can see about adding one for this myself.

Also a quick fmt error. 😸

@parkr
Copy link
Member

parkr commented Jul 30, 2017

Added 2 tests: one for site.url and one to ensure the same behaviour for site.baseurl.

Copy link
Member

@mattr- mattr- left a comment

Choose a reason for hiding this comment

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

:shipit:

@parkr
Copy link
Member

parkr commented Jul 30, 2017

@jekyllbot: merge +fix

@jekyllbot jekyllbot merged commit b35c0d8 into master Jul 30, 2017
@jekyllbot jekyllbot deleted the url-to-s branch July 30, 2017 19:12
jekyllbot added a commit that referenced this pull request Jul 30, 2017
parkr pushed a commit that referenced this pull request Aug 12, 2017
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants