Skip to content

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

Merged
jekyllbot merged 2 commits intomasterfrom
url-to-s
Jul 30, 2017
Merged

Call to_s on site.url before attempting to concatenate strings#6253
jekyllbot merged 2 commits intomasterfrom
url-to-s

Conversation

@benbalter
Copy link
Copy Markdown
Contributor

@benbalter 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.

pathawks
pathawks approved these changes Jul 27, 2017
View reviewed changes
Copy link
Copy Markdown
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 👍

parkr
parkr approved these changes Jul 30, 2017
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
@jekyllbot
Update history to reflect merge of #6253 [ci skip]
3496797
This was referenced Aug 1, 2017
Merged
Merged
parkr pushed a commit that referenced this pull request Aug 12, 2017
@benbalter @parkr
Call to_s on site.url before attempting to concatenate strings (#6253)
7a928f2 
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.

Reviewers

@mattr- mattr- mattr- approved these changes

@parkr parkr parkr approved these changes

@pathawks pathawks pathawks approved these changes

Assignees

No one assigned

Labels

frozen-due-to-age

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@benbalter @parkr @mattr- @pathawks @jekyllbot