mkdocs / mkdocs-redirects

Open source plugin for Mkdocs page redirects
MIT License
176 stars 25 forks source link

Fixed redirects for computed destinations and URLs #45

Closed squidfunk closed 1 year ago

squidfunk commented 1 year ago

Fixes #46

When defining redirects for Material for MkDocs' new built-in blog plugin, I stumbled upon a problem where computed URLs are not taken into account. The blog plugin recursively enumerates all Markdown files in the folder docs/blog/posts and then rewrites the File objects to have the correct dest_path and url values in the on_files hook. This is done so MkDocs and other plugins know the correct destinations for all blog posts. The result looks like this:

File(
  src_path='blog/posts/blog-support-just-landed.md', 
  dest_path='blog/2022/09/12/blog-support-just-landed/index.html', 
  name='blog-support-just-landed', 
  url='blog/2022/09/12/blog-support-just-landed/'
)

Now, when I map the old paths to the new src_path, the redirects plugin will ignore the computed, file.url and compute the target URL by itself, resulting in blog/posts/blog-support-just-landed/, which mismatches the destination URL.

This PR fixes the problem by using the computed URL, so the following configuration is correct:

blog/2022/chinese-search-support.md: blog/posts/chinese-search-support.md
blog/2021/the-past-present-and-future.md: blog/posts/the-past-present-and-future.md
blog/2021/excluding-content-from-search.md: blog/posts/excluding-content-from-search.md
blog/2021/search-better-faster-smaller.md: blog/posts/search-better-faster-smaller.md
squidfunk commented 1 year ago

I'm not sure whether the failing tests are false positives or not, due to the changed input of the function.

squidfunk commented 1 year ago

The fact that the tests only check get_relative_html_path made them fail, as this function now receives URL inputs (previously .md inputs). For this reasons, we now create the File objects that MkDocs will create, and pass them to the function. I guess there are several opportunities for refactoring since the logic should be much simpler now, but I'm not confident enough to do the refactoring as I just touched the code base for the first time.

Any thoughts on why Python 2.7 fails?

oprypin commented 1 year ago

This is a great implementation! I am able to reproduce locally with Python 2.7, looking into it. Also I suspect we will be able to throw away a bit more code after this.

oprypin commented 1 year ago

Uuh Python 2.7 fails because it's forced to install MkDocs 1.0.4 and that was prior to this very old bugfix...

And these failures are real with MkDocs 1.0.4 :grimacing:

squidfunk commented 1 year ago

Yeah, I'm not sure how to proceed here. Is Python 2.7 support still necessary? I'm not familiar enough with the ecosystem to judge. Could we issue a new release without Python 2.7 support, locking it into using the version before the merge?

oprypin commented 1 year ago

I think I'll drop it. And I'm also working on some change to tests so I can be more confident in them

oprypin commented 1 year ago

So I indeed dropped Python 2.7.

We're fine to merge this as is, just with the changes I suggested.

Afterwards I'll apply my refactoring of tests: https://github.com/mkdocs/mkdocs-redirects/compare/bb2ee628e2c064bd7ebc7d72e4426fa114ae7e1a With these tests I am already very confident that this is good.

squidfunk commented 1 year ago

All good, thanks for looking at this!