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

Escaped regular expressions when using post_url. #5605

Merged
merged 4 commits into from Nov 28, 2016

Conversation

Projects
None yet
6 participants
@Lunderberg
Contributor

Lunderberg commented Nov 26, 2016

Previously, the post_url function would give error messages when the post being listed contained special characters for use in regular expressions. These special characters are now escaped using
Regexp.escape.

This is in reference to issue #3486.

Escaped regular expressions when using post_url.
Previously, the post_url function would give error messages when the
post being listed contained special characters for use in regular
expressions.  These special characters are now escaped using
Regexp.escape.
@oe

This comment has been minimized.

Show comment
Hide comment
@oe
Member

oe commented Nov 26, 2016

@oe

oe approved these changes Nov 26, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 26, 2016

Member

Hello! Thanks so much for the pull request. In order to evaluate this change and to prevent it from regressing in the future, we would like to see a unit test added to test/test_tags.rb. You should see examples for this tag in that file. Let us know if we can help!

Member

parkr commented Nov 26, 2016

Hello! Thanks so much for the pull request. In order to evaluate this change and to prevent it from regressing in the future, we would like to see a unit test added to test/test_tags.rb. You should see examples for this tag in that file. Let us know if we can help!

Show outdated Hide outdated lib/jekyll/tags/post_url.rb
@@ -14,7 +14,8 @@ def initialize(name)
"'#{name}' does not contain valid date and/or title."
end
@name_regex = %r!^_posts/#{path}#{date}-#{slug}\.[^.]+|
escaped_slug = Regexp.escape(slug)
@name_regex = %r!^_posts/#{path}#{date}-#{escaped_slug}\.[^.]+|
^#{path}_posts/?#{date}-#{slug}\.[^.]+!x

This comment has been minimized.

@dotnil

dotnil Nov 27, 2016

Shouldn't the latter slug be escaped too?

@dotnil

dotnil Nov 27, 2016

Shouldn't the latter slug be escaped too?

This comment has been minimized.

@Lunderberg

Lunderberg Nov 27, 2016

Contributor

Thank you. I had missed that one, and the updated pull request has both slugs escaped.

@Lunderberg

Lunderberg Nov 27, 2016

Contributor

Thank you. I had missed that one, and the updated pull request has both slugs escaped.

@Lunderberg

This comment has been minimized.

Show comment
Hide comment
@Lunderberg

Lunderberg Nov 27, 2016

Contributor

There is now a unit test in test/test_tags.rb to test the escaping of special characters. The test succeeds with the change applied, and fails when the change is not present. I haven't used ruby very much before, so it is mainly a copy-paste-modify of an existing test, with small modifications.

Contributor

Lunderberg commented Nov 27, 2016

There is now a unit test in test/test_tags.rb to test the escaping of special characters. The test succeeds with the change applied, and fails when the change is not present. I haven't used ruby very much before, so it is mainly a copy-paste-modify of an existing test, with small modifications.

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 27, 2016

Member

@Lunderberg please update this test as well to have our CI go green.. Thanks! 😃

Member

ashmaroli commented Nov 27, 2016

@Lunderberg please update this test as well to have our CI go green.. Thanks! 😃

@parkr

parkr approved these changes Nov 27, 2016

One more thing then we're done! 😄

Show outdated Hide outdated test/test_tags.rb
refute_match(%r!markdown\-html\-error!, @result)
end
should "have the URL to the \"complex\" post from 2008-11-21" do

This comment has been minimized.

@parkr

parkr Nov 27, 2016

Member

This test name should be updated 😄

@parkr

parkr Nov 27, 2016

Member

This test name should be updated 😄

This comment has been minimized.

@ashmaroli

ashmaroli Nov 27, 2016

Member

can single quotes be used here, (for cleanliness)?

@ashmaroli

ashmaroli Nov 27, 2016

Member

can single quotes be used here, (for cleanliness)?

This comment has been minimized.

@Lunderberg

Lunderberg Nov 28, 2016

Contributor

Good points, both changes have been made.

@Lunderberg

Lunderberg Nov 28, 2016

Contributor

Good points, both changes have been made.

This comment has been minimized.

@ashmaroli

ashmaroli Nov 28, 2016

Member

😄 I was actually asking Parker if that was allowed, but since you @Lunderberg implemented it anyways, can we switch the quotes to have it "have the URL to the 'special-chars' post from 2016-11-26" for homogeneity with other unit-tests..? also, there's more instances of \"" inside test names within the same test-file.. can you patch that as well? Thanks

@ashmaroli

ashmaroli Nov 28, 2016

Member

😄 I was actually asking Parker if that was allowed, but since you @Lunderberg implemented it anyways, can we switch the quotes to have it "have the URL to the 'special-chars' post from 2016-11-26" for homogeneity with other unit-tests..? also, there's more instances of \"" inside test names within the same test-file.. can you patch that as well? Thanks

This comment has been minimized.

@ashmaroli

ashmaroli Nov 28, 2016

Member

cleaning other unit-test-names is perhaps beyond the scope of this pull.. so we'll tackle those in another PR..

@ashmaroli

ashmaroli Nov 28, 2016

Member

cleaning other unit-test-names is perhaps beyond the scope of this pull.. so we'll tackle those in another PR..

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 28, 2016

Member

Wonderful work, thank you!

@jekyllbot: merge +bug

Member

parkr commented Nov 28, 2016

Wonderful work, thank you!

@jekyllbot: merge +bug

@jekyllbot jekyllbot merged commit c3bf59c into jekyll:master Nov 28, 2016

2 checks passed

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

@jekyllbot jekyllbot added bug fix labels Nov 28, 2016

jekyllbot added a commit that referenced this pull request Nov 28, 2016

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