404 and around

[digitalinferno]

If I create a plugin that serves a page when I request site/fakepage, for example using onPageRendering with $twigTemplate = 'fakepage.twig', Pico still considers this as 404 content. On the user side, everything works fine, but is404Content() returns true (similarly, on404ContentLoaded(&$rawContent) is also triggered). Is there a way to work around this? For instance, if I want to log all the requests that result in a 404, calls to site/fakepage would also be logged as 404s, but they shouldn't be.

Another question about 404 behavior: If I create a 404.md page and point to mysite/404, Pico serves the 404 page, but with a 200 OK response, because the page actually exists. Is it an expected behavior? A better practice would be to only allow a _404.md file, which is not accessible directly but is used by Pico to serve a 404 message when there is a bad request. This approach, like before, avoids false positives when plugins check for is404Content().

[PhrozenByte]

If I create a plugin that serves a page when I request site/fakepage, for example using onPageRendering with $twigTemplate = 'fakepage.twig', Pico still considers this as 404 content. […] Is there a way to work around this?

I'm afraid there isn't. There were plans for Pico 4.0 to fix that (by adding "virtual pages"), but that never materialized. Since your plugin is taking control anyway, you don't really have to care about is404Content() yielding true; the only effect it has is that you have to change the HTTP status code back to 200 OK.

Another question about 404 behavior: If I create a 404.md page and point to mysite/404, Pico serves the 404 page, but with a 200 OK response, because the page actually exists. Is it an expected behavior?

No, 404.md files are considered hidden and should therefore follow the is404Content path. Please check again (because the code looks right to me) and if Pico truly responds with 200 OK (check that no plugin is changing the status code!) please open a separate issue. However, as a disclaimer, since Pico's development has stopped, it's unlikely that it will be fixed.

[digitalinferno]

I'm afraid there isn't. There were plans for Pico 4.0 to fix that (by adding "virtual pages"), but that never materialized.

Ok, thanks.

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in two days if no further activity occurs. Thank you for your contributions! 👍

[digitalinferno]

There were plans for Pico 4.0 to fix that (by adding "virtual pages"), but that never materialized.

@PhrozenByte I'm currently investigating this issue. At the moment, I can hook into on404ContentLoaded and inject my own rawContent, which allows the content to be displayed, but it's not accessible to other plugins, nor does it integrate with Pico's page system.

Alternatively, I can hook into onPagesDiscovered and add a virtual page to the $pages array. This way the page is included in Pico's structure (and maybe accessible to plugins loaded afterwards). However Pico itself doesn't handle this data correctly during rendering (missing md).

I believe it's possible (without breaking anything) to modify readPages() in Pico and supply raw content, something like if $this->is404Content $rawContent = from a plugin, effectively generating a page without relying on .md files.

Is this the right approach, or would a major refactoring of the Pico system be necessary?

[PhrozenByte]

@digitalinferno, you should do both, i.e. hook into both on404ContentLoaded (or onContentLoaded for that matter, but since you want to do header('HTTP/1.1 200 OK'); anyway, on404ContentLoaded is indeed better) and replace the rawContent accordingly, and hook into onPagesDiscovered and add your virtual page there. You could (and most of the time should) use references for the rawContent variable, i.e. first create your content with e.g. $this->rawContent = "My awesome page contents";, then $rawContent = &$this->rawContent; in on404ContentLoaded, and lastly &$pages['my_virtual_page'] = [ …, 'rawContent' = &$this->rawContent, … ]; in onPagesDiscovered.

Optionally you can also hook into onContentParsed to get the parsed $content, and into onMetaParsed to get $meta as references (e.g. $this->content = &$content; resp. $this->meta = &$meta;) to add them in onPagesDiscovered as well.

It's without any doubt extremely cumbersome and more feels like a workaround than a proper solution, but it's actually how it was intended with Pico 3.0 and earlier. This unnecessary complexity and "hackiness" was the main reason why I wanted to change that with Pico 4.0.

[digitalinferno]

Yes, it's a bit cumbersome, so I'm also looking into possible small core-level changes.

Currently, Pico follows this flow:

Pico reads the URL  
Searches for the corresponding .md file  
If found displays the raw content  
Else returns a 404  
After that builds the complete pages array

I assume this order is primarily for performance reasons, so Pico can serve the current page as quickly as possible.

My idea is to introduce a new event (like onLoadingVirtualContent) to extend this behavior with no breaks:

Pico reads the URL  
Searches for the corresponding .md file  
If found displays the raw content  
Else if a matching virtual ID exists, display raw content from the virtual page  
Else return 404  
After that merge the virtual pages array with the regular pages array

Out of curiosity, which data flow would be ideally better from a design standpoint?
Something like this is a good start?

Pico reads the URL  
Checks for a cached/JSON page/array  
If the page is cached serve it  
Else if the .md file exists serve it  and make cache
Else return 404  

[PhrozenByte]

I assume this order is primarily for performance reasons, so Pico can serve the current page as quickly as possible.

I'd love to say that this is the case, but in reality it's just historic reasons: In Gilbert's first versions of Pico there wasn't even a pages array. It was added later and since then we just never changed the processing order. In a complete rewrite of Pico I'd probably just switch the order, i.e. discover pages first and then decide what to render. However, this will likely break compatibility with many plugins (not sure whether there's even a way to implement this in a BC way).

If I'd want to improve support for virtual pages without breaking too many things, I'd probably just add a parameter to onContentLoading that tells Pico what to expect: Such $pageType parameter could distinguish between (1) "Markdown file exists and is a regular page", (2) "Markdown file exists but is inaccessible, therefore Pico should serve a 404 page" (i.e. the basename starts with a _ (like _meta.md) or is 404.md), (3) "No Markdown file exists, but a plugin will provide contents" (i.e. a virtual page), and (4) "No Markdown file exists and Pico should serve a 404 page". The $pageType could be pre-populated with the result of these checks here, followed by either just the onContentLoaded event in case of (1), on404Content… events in case of (2) and (4), or new onVirtualContent… events in case of (3) (i.e. similar to on404Content…; I'd also take things like is404Content into account). You'd still have to hook into onPagesDiscovered though. I don't think it's reasonably possible to change that without also changing processing order.

Caching is a whole different story. Caching has some advantages, but also disadvantages, like the necessity to detect an outdated cache - for both the requested page, and any other page (think about the site's nav menu). See #317 for some of my ideas for caching in Pico 4.0.

[digitalinferno]

Thanks for the hints. Discovering all pages first does make sense architecturally, but isn’t it a heavy task to perform at the very beginning of each request? Or would this kind of rewrite go hand in hand with some form of caching?

[PhrozenByte]

I don't think it really make any difference in which order we execute the tasks ("page discovery" and "page parsing") if the tasks itself don't change. I might totally miss something here (I'm afraid I haven't worked on Pico for ages…), but as far as I can reconstruct things in my mind right now the logic within the task doesn't really change (I don't mean "just call readPages() earlier in Pico::run()" by that, the code definitely needs more changes, e.g. making sure that this shortcut still works, but from a "what needs to be done" standpoint, like getting directory listings and reading file contents), but I don't see how the tasks itself change.

However, I once again want to stress that this will likely fundamentally break many plugins and I'm not sure whether it's even possible to preserve BC with this. It's the better approach architecturally though, I'm just not sure whether it's still Pico then or more like a fork. IMHO such radical change should come with other major improvements (like the things outlined in #317), but that's really no longer up to me to decide, but anyone (or group of people) who wants to take over Pico's development.

[digitalinferno]

I understand the decision is no longer in your hands, but I’d still value your perspective on this. For fun, I started by moving 404 handling into a plugin instead of keeping it hardcoded in the core. Now, I’m experimenting to see how far I can push things based on #317 without breaking things. Restructuring the flow and potentially rebuilding the pages array with OOP is the next step... if it stays fun and I can learn along the way. :)