Skip to content
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

404 on an _page replaces that page in the Pages Array with 404's contents #602

Open
mayamcdougall opened this issue Aug 24, 2021 · 2 comments · May be fixed by #535
Open

404 on an _page replaces that page in the Pages Array with 404's contents #602

mayamcdougall opened this issue Aug 24, 2021 · 2 comments · May be fixed by #535

Comments

@mayamcdougall
Copy link
Collaborator

Mentioned in #600

When you attempt to navigate to an underscore page (eg _meta), the entry for that page in the pages array is replaced with the contents of your 404 page (either the internal one or 404.md).

This can lead to some odd behavior if your theme is trying to access that underscore page, and also overall doesn't seem like it should be happening. 😉

(Ugh, and yes, that 👆🏻was the most readable issue title I could come up with... it took me a whole five minutes. 🤦🏻‍♀️)

PhrozenByte added a commit that referenced this issue Mar 3, 2022
This is a workaround for meta pages (i.e. pages starting with a '_'): If a user attempts to request such a page, Pico won't respond with the contents of this meta page, but with a 404 page. This is expected behavior. However, we also have a shortcut in Pico::readPages() attempting to skip reading the contents of the requested page twice. Since we're not serving the contents of the meta page, but of the 404 page, we accidentally overwrite the contents of the meta page by Pico's 404 page. This is unexpected behavior. Even though this commit fixes this particular issue, it doesn't fix its major cause, as the shortcut still exists and can still be triggered by plugin authors by simply overwriting the contents of an existing page. Even though a plugin author might want this to happen, we can't really tell whether it is intended or not. The solution is to remove the shortcut, but we don't want that either, it's a useful performance optimization. The only real solution to this is to switch to page objects, allowing us to handle such situations more verbose. This feature is expected for Pico 4.0. For now we leave this partially fixed...

Fixes #602
@PhrozenByte
Copy link
Collaborator

Fixed in 71c0dfb

Some explanation:

This is a workaround for meta pages (i.e. pages starting with a '_'): If a user attempts to request such a page, Pico won't respond with the contents of this meta page, but with a 404 page. This is expected behavior. However, we also have a shortcut in Pico::readPages() attempting to skip reading the contents of the requested page twice. Since we're not serving the contents of the meta page, but of the 404 page, we accidentally overwrite the contents of the meta page by Pico's 404 page. This is unexpected behavior. Even though this commit fixes this particular issue, it doesn't fix its major cause, as the shortcut still exists and can still be triggered by plugin authors by simply overwriting the contents of an existing page. Even though a plugin author might want this to happen, we can't really tell whether it is intended or not. The solution is to remove the shortcut, but we don't want that either, it's a useful performance optimization. The only real solution to this is to switch to page objects, allowing us to handle such situations more verbose. This feature is expected for Pico 4.0. For now we leave this partially fixed...

@PhrozenByte PhrozenByte linked a pull request Mar 6, 2022 that will close this issue
@mayamcdougall
Copy link
Collaborator Author

Seems like you've got it under control. 👍🏻

It's a minor issue, for sure. Technically though, even the Default Theme was affected by it. Trying to visit _meta makes your logo disappear. 😉

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

Successfully merging a pull request may close this issue.

2 participants