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

Use String#encode for xml_escape filter instead of CGI.escapeHTML #4694

Merged
merged 2 commits into from Mar 22, 2016

Conversation

Projects
None yet
4 participants
@pathawks
Member

pathawks commented Mar 21, 2016

Is there a reason to use CGI.escapeHTML over encode?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 21, 2016

Member

Is there a reason to use CGI.escapeHTML over encode?

Questions of compatibility come into play, as well as "do they actually do the same thing?" I have never used encode seriously so we'll see what the tests say!

Member

parkr commented Mar 21, 2016

Is there a reason to use CGI.escapeHTML over encode?

Questions of compatibility come into play, as well as "do they actually do the same thing?" I have never used encode seriously so we'll see what the tests say!

@parkr parkr added the undetermined label Mar 21, 2016

@parkr parkr added this to the undetermined milestone Mar 21, 2016

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 21, 2016

Contributor

The only problem here is that encode will wrap the string inside of a string. So: "hello & world".encode(:xml => :attr) will become "\"hello & world\"" I think by default to maintain compatibility it need use text ("".encode(:xml => :text)) and accept an extra attribute to enable attribute quoting (though lets be honest, we already quote most of the time so that would be far and few between.)

Contributor

envygeeks commented Mar 21, 2016

The only problem here is that encode will wrap the string inside of a string. So: "hello & world".encode(:xml => :attr) will become "\"hello & world\"" I think by default to maintain compatibility it need use text ("".encode(:xml => :text)) and accept an extra attribute to enable attribute quoting (though lets be honest, we already quote most of the time so that would be far and few between.)

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Mar 21, 2016

Member

Currently xml_escape will escape quotes as ".

encode(:xml => :attr) will wrap the string in quotes, but will also escape quotes as ". That is why I am deleting " from the result. (So 'hello & "world"'.encode(:xml => :attr) will become hello & "world")

encode(:xml => :text) does not wrap the string in quotes, but does not escape quotes either.

Tests pass.

Member

pathawks commented Mar 21, 2016

Currently xml_escape will escape quotes as ".

encode(:xml => :attr) will wrap the string in quotes, but will also escape quotes as ". That is why I am deleting " from the result. (So 'hello & "world"'.encode(:xml => :attr) will become hello & "world")

encode(:xml => :text) does not wrap the string in quotes, but does not escape quotes either.

Tests pass.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 21, 2016

Contributor

Remove the delete and use gsub (gsub(/\A"|"\Z/, "")) so it speaks more to it's intention and I'm 👍 on it as it's completely backwards, I just have a problem with the delete this point because the code doesn't speak well to it's intentions to non-Rubyist's.

Contributor

envygeeks commented Mar 21, 2016

Remove the delete and use gsub (gsub(/\A"|"\Z/, "")) so it speaks more to it's intention and I'm 👍 on it as it's completely backwards, I just have a problem with the delete this point because the code doesn't speak well to it's intentions to non-Rubyist's.

@pathawks pathawks closed this Mar 21, 2016

@pathawks pathawks reopened this Mar 21, 2016

@@ -117,7 +117,7 @@ def date_to_rfc822(date)
#
# Returns the escaped String.
def xml_escape(input)
CGI.escapeHTML(input.to_s)
input.to_s.encode(:xml => :attr).gsub(/\A"|"\Z/, "")

This comment has been minimized.

@pathawks
@pathawks
@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 21, 2016

Contributor

/cc @jekyll/core :shipit:

Contributor

envygeeks commented Mar 21, 2016

/cc @jekyll/core :shipit:

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 21, 2016

Member

Do we have tests for inspect? I'd expect:

{{ page.layout | inspect }}

to output

"default"

and the user would see

"default"

on their site so they know it's a string.

Member

parkr commented Mar 21, 2016

Do we have tests for inspect? I'd expect:

{{ page.layout | inspect }}

to output

"default"

and the user would see

"default"

on their site so they know it's a string.

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Mar 21, 2016

Member

Do we have tests for inspect?

Here's what I was able to find:

    context "inspect filter" do
      should "return a HTML-escaped string representation of an object" do
        assert_equal "{&quot;&lt;a&gt;&quot;=&gt;1}", @filter.inspect({ "<a>" => 1 })
      end
    end

If you like, I could add a test specifically for strings:

      should "quote strings" do
        assert_equal "&quot;string&quot;", @filter.inspect("string")
      end

I just tested this locally and it passes.

Member

pathawks commented Mar 21, 2016

Do we have tests for inspect?

Here's what I was able to find:

    context "inspect filter" do
      should "return a HTML-escaped string representation of an object" do
        assert_equal "{&quot;&lt;a&gt;&quot;=&gt;1}", @filter.inspect({ "<a>" => 1 })
      end
    end

If you like, I could add a test specifically for strings:

      should "quote strings" do
        assert_equal "&quot;string&quot;", @filter.inspect("string")
      end

I just tested this locally and it passes.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 21, 2016

Member

@pathaws Yes, please add a test specifically for strings. The gsub worried me 😄

Member

parkr commented Mar 21, 2016

@pathaws Yes, please add a test specifically for strings. The gsub worried me 😄

@@ -394,6 +394,10 @@ def to_liquid
should "return a HTML-escaped string representation of an object" do
assert_equal "{&quot;&lt;a&gt;&quot;=&gt;1}", @filter.inspect({ "<a>" => 1 })
end
should "quote strings" do
assert_equal "&quot;string&quot;", @filter.inspect("string")

This comment has been minimized.

@pathawks
@pathawks
@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 21, 2016

Contributor

gsub has no idea that &quot; is " so if you send a quote and it escapes it then it'll pass.

Contributor

envygeeks commented Mar 21, 2016

gsub has no idea that &quot; is " so if you send a quote and it escapes it then it'll pass.

@parkr parkr changed the title from Use encode for xml_escape filter to Use String#encode for xml_escape filter instead of CGI.escapeHTML Mar 22, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 22, 2016

Member

Sah weeet.

Thanks, @pathawks!

@jekyllbot: merge +minor

Member

parkr commented Mar 22, 2016

Sah weeet.

Thanks, @pathawks!

@jekyllbot: merge +minor

jekyllbot added a commit that referenced this pull request Mar 22, 2016

@jekyllbot jekyllbot merged commit e873934 into jekyll:master Mar 22, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

jekyllbot added a commit that referenced this pull request Mar 22, 2016

@parkr parkr modified the milestones: 3.2, undetermined Mar 22, 2016

@parkr parkr added enhancement and removed undetermined labels Mar 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment