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

benbalter commented Jul 27, 2017

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 left a comment

Rubocop thinks the line is too long.
Otherwise LGTM 👍

@parkr
parkr approved these changes Jul 30, 2017
Copy link
Member

parkr left a comment

Test?

@parkr

This comment has been minimized.

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. 😸

Also adds tests
@parkr

This comment has been minimized.

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.

@mattr-
mattr- approved these changes Jul 30, 2017
Copy link
Member

mattr- left a comment

:shipit:

@parkr

This comment has been minimized.

Copy link
Member

parkr commented Jul 30, 2017

@jekyllbot: merge +fix

@jekyllbot jekyllbot merged commit b35c0d8 into master Jul 30, 2017
2 checks passed
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 url-to-s branch Jul 30, 2017
jekyllbot added a commit that referenced this pull request Jul 30, 2017
parkr added a commit that referenced this pull request Aug 12, 2017
Merge pull request 6253 in 3.5-stable branch
@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
5 participants
You can’t perform that action at this time.