automad icon indicating copy to clipboard operation
automad copied to clipboard

PATHINFO (even when empty) prioritized above REQUEST_URI (even when non-empty)

Open barryhughes opened this issue 5 years ago • 5 comments
trafficstars

Hullo, Automad noob here.

When setting up for the first time (via composer create-project) I found I was unable to access anything except the homepage. Following links to /dashboard or to demo content such as /work/a-project-page had no effect, they just caused the homepage to reload.

On doing some debugging I noticed that, regardless of the actual URL path, AM_REQUEST was being set to (string) '/', which explains the behaviour I was seeing. From there I took a look at Automad\Core\Request::page() and I think the problem is in the order of operations. The way it works reads to me like this:

  • If the query string is used to contain the path, that is processed first.
  • Otherwise, it looks at different $_SERVER values to discover the path.

In the second of those cases, it will check if PATHINFO is set first and will use that value, otherwise it looks at ORIG_PATH_INFO and so on. The problem for me was that PATHINFO was set, but to an empty string. REQUEST_URI was also set, and that is where the path string was living. Hopefully that all makes sense so far.

Of course, others may not see this problem: a lot depends on how things outside of Automad (ie, at Nginx or Apache level) are configured. However, it seems like it would be a shame to let this potentially stop others from using and experiencing Automad as it seems like a great tool. I was curious on your thoughts on this ... would you be open to considering either of the following approaches?

  • Prioritizing REQUEST_URI above PATHINFO (on the basis that the latter is a bit of an edge-case scenario these days)?
  • Or, keeping the existing order but changing the logic so that instead of testing if PATHINFO, ORIG_PATH_INFO, REQUEST_URI, ... are set it tests if they are non-empty.

barryhughes avatar Aug 31 '20 15:08 barryhughes

:bulb: Just noting in case useful for others...if you experience the same thing (ie, you click on the /dashboard link but still see the homepage) an easy fix is to add the following line to the top of index.php (of course, after the opening PHP tag):

unset($_SERVER['PATH_INFO']);

barryhughes avatar Aug 31 '20 15:08 barryhughes

Hi, thanks for your input. What kind of server are you using? In case you use Apache, the included .htaccess file should do fine.

I think in general the most stable solution is to use the query string to communicate the requested page.

Regarding the second option, maybe the order is not so important, but I agree that checking for non-empty values is a good idea.

marcantondahmen avatar Aug 31 '20 22:08 marcantondahmen

What kind of server are you using?

Locally I'm using the official Nginx and PHP (FPM) images, orchestrated using Docker Compose.

I agree that checking for non-empty values is a good idea.

If you're open to it and I have the time, I'd be happy to submit a pull request for this. Query strings might indeed be the most stable way, it just occurs that we could make it so that Automad supports configurations like mine out of the box, and in doing so remove a potential source of friction for new users without adversely impacting existing users :-)

barryhughes avatar Aug 31 '20 22:08 barryhughes

Hey, thanks! Unfortunately due to licensing issues I can't accept pull requests to this repository. Check out the readme file for more detail. Sorry! 😕 I hope you understand this.

However — I take your issue very serious. I will take a look at it.

There's also a guide for setting up rewriting in a Nginx/fpm environment on the "Getting Started" page in the docs including a snippet of a nginx.conf. I didn't test it within the exact setup like yours, but that one should define PATH_INFO correctly. Maybe you can quickly try it.

Thank you for digging into this!

marcantondahmen avatar Sep 01 '20 08:09 marcantondahmen

No problem, I can understand your policy on pull requests...and yep, it looks like the sample nginx.conf would be another way to avoid this issue. Thanks!

barryhughes avatar Sep 01 '20 13:09 barryhughes