readthedocs / sphinx-notfound-page

Create a custom 404 page with absolute URLs hardcoded
https://sphinx-notfound-page.readthedocs.io/
MIT License
48 stars 32 forks source link

Read the Docs ignores custom 404 at /404/index.html with dirhtml #215

Closed hugovk closed 1 year ago

hugovk commented 1 year ago

Steps to reproduce

  1. Build docs using dirhtml with notfound_urls_prefix = "/" for a single version site that is deployed at the root (i.e. no /en/latest/)
  2. Visit some page that doesn't exist: https://cpython-devguide--1042.org.readthedocs.build/notfound

Actual result

Get the default maze:

image

Expected result

Get the custom 404 page.

More info

It seems that Read the Docs only supports a custom 404 page at 404.html?

https://docs.readthedocs.io/en/stable/hosting.html#custom-not-found-404-pages

And it looks like, with dirhtml, this extension builds the custom 404 page at 404/index.html, which Read the Docs ignores?

For example, I can see custom 404 page (returning a 200) at:

https://cpython-devguide--1042.org.readthedocs.build/404/index.html

image

See also

https://github.com/python/devguide/pull/1042

humitos commented 1 year ago

Thanks for reporting this issue.

It seems it's a bug in Read the Docs itself. We do have a check in place for 404/index.html but for some reason it doesn't find it in your project.

https://github.com/readthedocs/readthedocs.org/blob/77781b122cbdc5c70f56d44722ef83ed1abd9ce6/readthedocs/proxito/views/serve.py#L380-L381

I guess it's because it's not detected as HTMLDIR. I'll research a little and come back with the solution... Hopefully 😅

humitos commented 1 year ago

My guess was correct. Since you are using build.commands (in beta), we don't really know what's the "documentation type" of that version of your documentation. Because of that, we set it as generic. Then, that line that I linked is skipped and 404/index.html file is not checked.

In [1]: p = Project.objects.get(slug='cpython-devguide')

In [4]: v = p.versions.get(slug='1042')

In [5]: v.documentation_type
Out[5]: 'generic'

I are not checking for that file always to avoid hitting S3 API and extra time for all the requests. However, I suppose we may need to re-consider this since we have support for more doctools now, and I don't think we can keep making the same assumption. @ericholscher what do you think?

ericholscher commented 1 year ago

@humitos Yea, agreed. We likely need to be smarter here, but I really dislike adding more checks for this.

humitos commented 1 year ago

This is already solved in https://github.com/readthedocs/readthedocs.org/pull/9983 and it's going to be deploy today. Please, confirm this is working as you expected later today, or tomorrow 😄

humitos commented 1 year ago

It works now! 🥳

Screenshot_2023-02-07_17-47-27

hugovk commented 1 year ago

Brilliant, thank you for the quick fix!