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

MetaTag: when encoding for XML special characters, handle non-string objects #326

Merged
merged 1 commit into from
Oct 3, 2020

Conversation

parkr
Copy link
Member

@parkr parkr commented Oct 2, 2020

Fixes #325.

The jekyll-github-metadata plugin, for example, sets site.title and
site.name to Jekyll::GitHubMetadata::Value objects which respond to
#to_s and #to_liquid, but NOT #encode. Therefore, we should cast
to a string if #encode is not yet available.

The reason we cast to a string is because the previous code was
"#{k}=#{v}", which was implicitly casting to a string for v.

…objects

The jekyll-github-metadata plugin, for example, sets `site.title` and
`site.name` to `Jekyll::GitHubMetadata::Value` objects which respond to
`#to_s` and `#to_liquid`, but NOT `#encode`. Therefore, we should cast
to a string if `#encode` is not yet available.

The reason we cast to a string is because the previous code was
"#{k}=#{v}", which was implicitly casting to a string for `v`.
@parkr parkr requested a review from a team October 2, 2020 20:09
Copy link
Member

@DirtyF DirtyF left a comment

Choose a reason for hiding this comment

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

Crystal clear now, sorry for letting this typing error sleep in 😊

@DirtyF DirtyF added the fix label Oct 2, 2020
@ashmaroli
Copy link
Member

@parkr Do we really need to have that respond_to?(:encode) check?
Can't we cast and proceed right away?

attributes.map { |k, v| "#{k}=#{v.to_s.encode(:xml => :attr)}" }.join(" ")

@parkr
Copy link
Member Author

parkr commented Oct 3, 2020

No worries at all 😄

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit df10ac8 into master Oct 3, 2020
@jekyllbot jekyllbot deleted the non-string-titles branch October 3, 2020 15:02
jekyllbot added a commit that referenced this pull request Oct 3, 2020
@jekyll jekyll locked and limited conversation to collaborators Oct 3, 2021
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.

XML escaping the site title with jekyll-github-metadata errors
4 participants