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

Fix bug where `post_url` tag matched incorrect post with subdirectory #4873

Merged
merged 3 commits into from Oct 5, 2016

Conversation

Projects
None yet
5 participants
@mlocher
Contributor

mlocher commented May 7, 2016

Instead of matching the the value provided to post_url against the base name, test against the relative path.

Updated the regexp to match both _posts/#{path}/#{date}-#{slug} as well as #{path}/_posts/#{date}-#{slug}

This is a very rough draft, and it's entirely possible that I missed something. But I wanted to get feedback on this approach and check if I should pursue further and work on tests.

This will break the build for anybody who worked around #3926 by removing the path from the calls to post_url, but it would be easy to adapt the regexp to match based on date and slug only.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 10, 2016

Member

@mlocher This is nice, thank you! Does this happen to break the build for anyone just doing a plain ol' {% post_url 2010-02-02-hi %}?

Member

parkr commented May 10, 2016

@mlocher This is nice, thank you! Does this happen to break the build for anyone just doing a plain ol' {% post_url 2010-02-02-hi %}?

@mlocher

This comment has been minimized.

Show comment
Hide comment
@mlocher

mlocher May 11, 2016

Contributor

@parkr Thanks :)

This would not break builds for people not using subdirectories and referencing posts via {% post_url 2010-02-02-hi %}, it would however break builds for people using subdirectories and not mentioning them in the call to post_url.

_posts/2010-02-02-hi.md and {% post_url 2010-02-02-hi %} 👍
_posts/category/2010-02-02-hi.md and {% post_url category/2010-02-02-hi %} 👍
category/_posts/2010-02-02-hi.md and {% post_url category/2010-02-02-hi %} 👍
category/_posts/2010-02-02-hi.md and {% post_url 2010-02-02-hi %} 👎
_posts/category/2010-02-02-hi.md and {% post_url 2010-02-02-hi %} 👎

It would be easy to make the category optional (and allow the last two options) by extending the regular expression (and I'd be happy to prepare this).

I'd also be happy to start working on tests, if you're fine with the approach.

Contributor

mlocher commented May 11, 2016

@parkr Thanks :)

This would not break builds for people not using subdirectories and referencing posts via {% post_url 2010-02-02-hi %}, it would however break builds for people using subdirectories and not mentioning them in the call to post_url.

_posts/2010-02-02-hi.md and {% post_url 2010-02-02-hi %} 👍
_posts/category/2010-02-02-hi.md and {% post_url category/2010-02-02-hi %} 👍
category/_posts/2010-02-02-hi.md and {% post_url category/2010-02-02-hi %} 👍
category/_posts/2010-02-02-hi.md and {% post_url 2010-02-02-hi %} 👎
_posts/category/2010-02-02-hi.md and {% post_url 2010-02-02-hi %} 👎

It would be easy to make the category optional (and allow the last two options) by extending the regular expression (and I'd be happy to prepare this).

I'd also be happy to start working on tests, if you're fine with the approach.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 17, 2016

Member

it would however break builds for people using subdirectories and not mentioning them in the call to post_url.

@mlocher Can we fall back to the old behaviour? We can't break sites until Jekyll 4... :/ A deprecation message would do fine but it should still Just Work ™️

Member

parkr commented May 17, 2016

it would however break builds for people using subdirectories and not mentioning them in the call to post_url.

@mlocher Can we fall back to the old behaviour? We can't break sites until Jekyll 4... :/ A deprecation message would do fine but it should still Just Work ™️

@mlocher

This comment has been minimized.

Show comment
Hide comment
@mlocher

mlocher May 18, 2016

Contributor

@parkr Sorry, I was unclear, the call to {% post_url 2010-02-02-hi %} would still work via the deprecated_equality method (which I didn't touch) and I just added a test case to make sure the correct URL is generated and a Deprecation warning is shown in the log.

I tried to add a test case for linking to the category/2008-9-23-categories.markdown file as well, but anything outside of the _posts directory didn't get loaded into the site.posts.docs Array during the tests. This works when running a standard jekyll build though.

  • ==
    • _posts/2010-02-02-hi.md and {% post_url 2010-02-02-hi %} 👍
    • _posts/category/2010-02-02-hi.md and {% post_url category/2010-02-02-hi %} 👍
    • category/_posts/2010-02-02-hi.md and {% post_url category/2010-02-02-hi %} 👍
    • category/_posts/2010-02-02-hi.md and {% post_url 2010-02-02-hi %} 👎
    • _posts/category/2010-02-02-hi.md and {% post_url 2010-02-02-hi %} 👎
  • deprecated_equality
    • not called for 1 to 3
    • category/_posts/2010-02-02-hi.md and {% post_url 2010-02-02-hi %} 👍
    • _posts/category/2010-02-02-hi.md and {% post_url 2010-02-02-hi %} 👍

Tests are missing for the two use cases referencing a post like category/_posts/2010-02-02-hi.md

Contributor

mlocher commented May 18, 2016

@parkr Sorry, I was unclear, the call to {% post_url 2010-02-02-hi %} would still work via the deprecated_equality method (which I didn't touch) and I just added a test case to make sure the correct URL is generated and a Deprecation warning is shown in the log.

I tried to add a test case for linking to the category/2008-9-23-categories.markdown file as well, but anything outside of the _posts directory didn't get loaded into the site.posts.docs Array during the tests. This works when running a standard jekyll build though.

  • ==
    • _posts/2010-02-02-hi.md and {% post_url 2010-02-02-hi %} 👍
    • _posts/category/2010-02-02-hi.md and {% post_url category/2010-02-02-hi %} 👍
    • category/_posts/2010-02-02-hi.md and {% post_url category/2010-02-02-hi %} 👍
    • category/_posts/2010-02-02-hi.md and {% post_url 2010-02-02-hi %} 👎
    • _posts/category/2010-02-02-hi.md and {% post_url 2010-02-02-hi %} 👎
  • deprecated_equality
    • not called for 1 to 3
    • category/_posts/2010-02-02-hi.md and {% post_url 2010-02-02-hi %} 👍
    • _posts/category/2010-02-02-hi.md and {% post_url 2010-02-02-hi %} 👍

Tests are missing for the two use cases referencing a post like category/_posts/2010-02-02-hi.md

@mlocher

This comment has been minimized.

Show comment
Hide comment
@mlocher

mlocher Jun 9, 2016

Contributor

@parkr, I rebased on the latest master and pushed the changes again.

Contributor

mlocher commented Jun 9, 2016

@parkr, I rebased on the latest master and pushed the changes again.

@ayastreb

This comment has been minimized.

Show comment
Hide comment
@ayastreb

ayastreb Jul 14, 2016

Contributor

Hi @mlocher, are you still working on this PR?
Could you please fix those failing Rubocop errors? Most of them could by fixed by Rubocop itself I guess 😄
This will also fix #2681
Thanks!

Contributor

ayastreb commented Jul 14, 2016

Hi @mlocher, are you still working on this PR?
Could you please fix those failing Rubocop errors? Most of them could by fixed by Rubocop itself I guess 😄
This will also fix #2681
Thanks!

mlocher added some commits May 7, 2016

Fix #3926 post_url helper with sub-directories
Instead of matching the the value provided to `post_url` against
the basename, test against the relative path.

Updated the regexp to match both
  * _posts/category
  * category/_posts
Add test case for deprecated post comparison
Checks that `post_url` works for nested posts even if the path isn't specified. Also checks that a deprecation message is shown in the build log.
@mlocher

This comment has been minimized.

Show comment
Hide comment
@mlocher

mlocher Jul 14, 2016

Contributor

@ayastreb thanks for pinging me :) I rebased the PR on the current master branch and fixed the Rubocop errors.

Let me know if this is ready now or if there is anything else I can help with.

Contributor

mlocher commented Jul 14, 2016

@ayastreb thanks for pinging me :) I rebased the PR on the current master branch and fixed the Rubocop errors.

Let me know if this is ready now or if there is anything else I can help with.

@ayastreb

This comment has been minimized.

Show comment
Hide comment
@ayastreb

ayastreb Jul 14, 2016

Contributor

Thanks, @mlocher!
I think @parkr needs to take a look once more and approve it, I just wanted to ping you 😄

Contributor

ayastreb commented Jul 14, 2016

Thanks, @mlocher!
I think @parkr needs to take a look once more and approve it, I just wanted to ping you 😄

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 15, 2016

Member

@mlocher When I said backwards-compatibility, that means we have to be able to just match the basename. If you say {% post_url 2016-01-01-hello-there %} it should match a category/_posts/2016-01-01-hello-there.md file as it does now. So you introduced a third way to match posts, which is arguably correct, but there is still a compatibility issue there.

Member

parkr commented Jul 15, 2016

@mlocher When I said backwards-compatibility, that means we have to be able to just match the basename. If you say {% post_url 2016-01-01-hello-there %} it should match a category/_posts/2016-01-01-hello-there.md file as it does now. So you introduced a third way to match posts, which is arguably correct, but there is still a compatibility issue there.

@parkr parkr added the enhancement label Jul 15, 2016

@mlocher

This comment has been minimized.

Show comment
Hide comment
@mlocher

mlocher Jul 15, 2016

Contributor

@parkr It does. If you call {% post_url 2016-01-01-hello-there %} the render method will first check for a post that matches via the regex I adjusted.

If that fails and no post is found it will use deprecated_equality method, which does a check based on equality of post slug, year, month and date. (This is a part of the software that I didn't touch. I only added a test for the deprecation warning, that is already being printed on standard out.

Contributor

mlocher commented Jul 15, 2016

@parkr It does. If you call {% post_url 2016-01-01-hello-there %} the render method will first check for a post that matches via the regex I adjusted.

If that fails and no post is found it will use deprecated_equality method, which does a check based on equality of post slug, year, month and date. (This is a part of the software that I didn't touch. I only added a test for the deprecation warning, that is already being printed on standard out.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 16, 2016

Member

@mlocher We added path-based equality checking because the date-based equality checking varied based on the timezone your workstation was in. Paths made way more sense, of course, than trying to parse times. I'm concerned that this simple basename-based equality won't work in the situation I mentioned and I'm not sure I communicated that properly. I can write further tests to elaborate if you'd like.

Member

parkr commented Jul 16, 2016

@mlocher We added path-based equality checking because the date-based equality checking varied based on the timezone your workstation was in. Paths made way more sense, of course, than trying to parse times. I'm concerned that this simple basename-based equality won't work in the situation I mentioned and I'm not sure I communicated that properly. I can write further tests to elaborate if you'd like.

@mlocher

This comment has been minimized.

Show comment
Hide comment
@mlocher

mlocher Jul 19, 2016

Contributor

@parkr, further tests or examples would be great. Right now, I get a deprecation warning for each use of post_url, that I can't get rid of other than excluding the path completely.

I fully understand the need to stay backwards compatible and not introduce breaking changes, so if you could provide me with either tests or a mapping of calls to post_url and filenames that should match, I'd be happy to make sure this PR conforms to those.

Contributor

mlocher commented Jul 19, 2016

@parkr, further tests or examples would be great. Right now, I get a deprecation warning for each use of post_url, that I can't get rid of other than excluding the path completely.

I fully understand the need to stay backwards compatible and not introduce breaking changes, so if you could provide me with either tests or a mapping of calls to post_url and filenames that should match, I'd be happy to make sure this PR conforms to those.

@parkr parkr added this to the 3.3 milestone Jul 25, 2016

@mlocher

This comment has been minimized.

Show comment
Hide comment
@mlocher

mlocher Sep 21, 2016

Contributor

I just noticed that the 3.3 milestone for this PR is coming up soon. I'd be happy to make further changes/add tests or anything else that's needed to get this ready.

And I wanted to say thank you for all the feedback and help with preparing this PR, you're doing an awesome job with maintaining Jekyll ❤️

Contributor

mlocher commented Sep 21, 2016

I just noticed that the 3.3 milestone for this PR is coming up soon. I'd be happy to make further changes/add tests or anything else that's needed to get this ready.

And I wanted to say thank you for all the feedback and help with preparing this PR, you're doing an awesome job with maintaining Jekyll ❤️

@parkr

Thanks, @mlocher! Please update the documentation in site/_docs/templates.md and indicate what arguments are no longer acceptable. IIRC, this contains a slightly breaking change.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Oct 4, 2016

Member

@mlocher Releasing 3.3 tomorrow or Thursday. Let me know if you need any assistance getting those docs updated. Can't merge until corresponding documentation is also updated. 😄

Member

parkr commented Oct 4, 2016

@mlocher Releasing 3.3 tomorrow or Thursday. Let me know if you need any assistance getting those docs updated. Can't merge until corresponding documentation is also updated. 😄

@mlocher

This comment has been minimized.

Show comment
Hide comment
@mlocher

mlocher Oct 5, 2016

Contributor

@parkr sorry, work got in the way :(

I just checked the documentation, and actually it's already up to date. https://jekyllrb.com/docs/templates/#post-url includes an example and mentions that users need to include the subdirectory in the call to post_url if posts on disk are organized in subdirectories

If you organize your posts in subdirectories, you need to include subdirectory path to the post:

Calls to post_url not including a subdirectory will match the same as without this PR. And the deprecated match based on dates & slug will be used if the regex based matcher doesn't find a post.

There are tests for all those situations as far as I can tell

Contributor

mlocher commented Oct 5, 2016

@parkr sorry, work got in the way :(

I just checked the documentation, and actually it's already up to date. https://jekyllrb.com/docs/templates/#post-url includes an example and mentions that users need to include the subdirectory in the call to post_url if posts on disk are organized in subdirectories

If you organize your posts in subdirectories, you need to include subdirectory path to the post:

Calls to post_url not including a subdirectory will match the same as without this PR. And the deprecated match based on dates & slug will be used if the regex based matcher doesn't find a post.

There are tests for all those situations as far as I can tell

@parkr

parkr approved these changes Oct 5, 2016

@parkr parkr changed the title from Fix #3926 post_url helper with sub-directories to Fix bug where `post_url` tag matched incorrect post with subdirectory Oct 5, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Oct 5, 2016

Member

@mlocher Thanks for that! I had it in my head that this was going to break sites.

@jekyllbot: merge +bug

Member

parkr commented Oct 5, 2016

@mlocher Thanks for that! I had it in my head that this was going to break sites.

@jekyllbot: merge +bug

@jekyllbot jekyllbot merged commit 3de7887 into jekyll:master Oct 5, 2016

1 check passed

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

@jekyllbot jekyllbot added bug fix labels Oct 5, 2016

jekyllbot added a commit that referenced this pull request Oct 5, 2016

@mlocher mlocher deleted the mlocher:bug-post-url branch Oct 6, 2016

@mlocher

This comment has been minimized.

Show comment
Hide comment
@mlocher

mlocher Oct 6, 2016

Contributor

@parkr Thanks for merging and thanks so much for all the help and getting this ready :)

Contributor

mlocher commented Oct 6, 2016

@parkr Thanks for merging and thanks so much for all the help and getting this ready :)

@dotnil

This comment has been minimized.

Show comment
Hide comment
@dotnil

dotnil Nov 25, 2016

What about paths like category/_posts/dir/date-slug.md? I know it might be rare but it is not forbidden by Jekyll. I've got several sites use this structure to organize posts by year, such as creatives/_posts/2016/2016-11-25-title.md

dotnil commented Nov 25, 2016

What about paths like category/_posts/dir/date-slug.md? I know it might be rare but it is not forbidden by Jekyll. I've got several sites use this structure to organize posts by year, such as creatives/_posts/2016/2016-11-25-title.md

@mlocher

This comment has been minimized.

Show comment
Hide comment
@mlocher

mlocher Nov 28, 2016

Contributor

@dotnil see the comment at #4873 (comment) for which URLs are tested.

Depending on who you link to that post, it might generate a deprecation warning, but the link will work.

Contributor

mlocher commented Nov 28, 2016

@dotnil see the comment at #4873 (comment) for which URLs are tested.

Depending on who you link to that post, it might generate a deprecation warning, but the link will work.

@dotnil

This comment has been minimized.

Show comment
Hide comment
@dotnil

dotnil Nov 29, 2016

Yes, my example isn't listed there. And I can confirm that with jekyll 3.3, there are deprecation warnings generated. So I'm wondering, should my example usage be allowed? If positive, I'm willing to suggest a pr.

dotnil commented Nov 29, 2016

Yes, my example isn't listed there. And I can confirm that with jekyll 3.3, there are deprecation warnings generated. So I'm wondering, should my example usage be allowed? If positive, I'm willing to suggest a pr.

@mlocher

This comment has been minimized.

Show comment
Hide comment
@mlocher

mlocher Nov 29, 2016

Contributor

Uh, sorry I missed the additional subdirectory in the path.

I can't tell for sure whether this structure is allowed or not. That said, Jekyll introduced a new link tag in #4624, which could at some point replace post_url completely and which works by specifying the file on disk instead.

Contributor

mlocher commented Nov 29, 2016

Uh, sorry I missed the additional subdirectory in the path.

I can't tell for sure whether this structure is allowed or not. That said, Jekyll introduced a new link tag in #4624, which could at some point replace post_url completely and which works by specifying the file on disk instead.

@dotnil

This comment has been minimized.

Show comment
Hide comment
@dotnil

dotnil Nov 30, 2016

Thanks @mlocher , link tag suffices :-)

dotnil commented Nov 30, 2016

Thanks @mlocher , link tag suffices :-)

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