[aur-dev] [PATCH] Add check for if PATH_INFO exists
When running aurweb with the command line php server and / is visited, it will give an undefined index notice. This will add a check if it exists and will prevent the notice. Signed-off-by: Mark Weiman <mark.weiman@markzz.com> --- web/html/index.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/html/index.php b/web/html/index.php index 78ab6ad..3bdf281 100644 --- a/web/html/index.php +++ b/web/html/index.php @@ -4,7 +4,7 @@ set_include_path(get_include_path() . PATH_SEPARATOR . '../lib'); include_once("aur.inc.php"); include_once("pkgfuncs.inc.php"); -$path = $_SERVER['PATH_INFO']; +$path = isset($_SERVER['PATH_INFO']) ? $_SERVER['PATH_INFO'] : "/"; $tokens = explode('/', $path); if (config_get_bool('options', 'enable-maintenance') && (empty($tokens[1]) || ($tokens[1] != "css" && $tokens[1] != "images"))) { -- 2.11.0
On Sun, 05 Feb 2017 at 00:35:57, Mark Weiman wrote:
When running aurweb with the command line php server and / is visited, it will give an undefined index notice. This will add a check if it exists and will prevent the notice.
Signed-off-by: Mark Weiman <mark.weiman@markzz.com> --- web/html/index.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/web/html/index.php b/web/html/index.php index 78ab6ad..3bdf281 100644 --- a/web/html/index.php +++ b/web/html/index.php @@ -4,7 +4,7 @@ set_include_path(get_include_path() . PATH_SEPARATOR . '../lib'); include_once("aur.inc.php"); include_once("pkgfuncs.inc.php");
-$path = $_SERVER['PATH_INFO']; +$path = isset($_SERVER['PATH_INFO']) ? $_SERVER['PATH_INFO'] : "/";
I am not 100% convinced that this is a good idea. If somebody wants to setup a "proper" web server running aurweb, this workaround makes it harder to figure out what went wrong if PATH_INFO is unset due to some misconfiguration. On the other hand, being able to run a quick test using `php -S` is quite nice. Would you might adding a notice to the commit message that this is specifically for `php -S`? I also think that a note on how to test aurweb patches with `php -S` should be added to the documentation. Maybe in INSTALL, before the "real" setup instructions? Something like: For testing aurweb patches before submission, you can use [...] Note that you can only do limited testing using the PHP built-in web server. In particular, the SSH/Git interface will be unusable. For a detailed description on how to setup a full aurweb server, read the instructions below. Opinions?
$tokens = explode('/', $path);
if (config_get_bool('options', 'enable-maintenance') && (empty($tokens[1]) || ($tokens[1] != "css" && $tokens[1] != "images"))) { -- 2.11.0
On Tue, 2017-02-07 at 21:43 +0100, Lukas Fleischer wrote:
I am not 100% convinced that this is a good idea. If somebody wants to setup a "proper" web server running aurweb, this workaround makes it harder to figure out what went wrong if PATH_INFO is unset due to some misconfiguration. On the other hand, being able to run a quick test using `php -S` is quite nice. Would you might adding a notice to the commit message that this is specifically for `php -S`?
If PATH_INFO is unset, then it will always return "/", so it would be fairly obvious that something's wrong if you are only getting the root page when you visit other pages.
I also think that a note on how to test aurweb patches with `php -S` should be added to the documentation. Maybe in INSTALL, before the "real" setup instructions? Something like:
For testing aurweb patches before submission, you can use [...]
Note that you can only do limited testing using the PHP built-in web server. In particular, the SSH/Git interface will be unusable. For a detailed description on how to setup a full aurweb server, read the instructions below.
Opinions?
That second paragraph is not entirely true, you can still set up an SSH/Git interface separate from the web interface. There's nothing too special about the web interface that the SSH/Git interface depends on. I think that while we are talking about testing installations, perhaps some information on how to populate a database using sqlite instead of MySQL should be added.
On Wed, 08 Feb 2017 at 02:33:48, Mark Weiman wrote:
On Tue, 2017-02-07 at 21:43 +0100, Lukas Fleischer wrote:
I am not 100% convinced that this is a good idea. If somebody wants to setup a "proper" web server running aurweb, this workaround makes it harder to figure out what went wrong if PATH_INFO is unset due to some misconfiguration. On the other hand, being able to run a quick test using `php -S` is quite nice. Would you might adding a notice to the commit message that this is specifically for `php -S`?
If PATH_INFO is unset, then it will always return "/", so it would be fairly obvious that something's wrong if you are only getting the root page when you visit other pages.
True, it would be obvious that something is wrong. However, it would be harder to pinpoint the actual issue. Right now, if the server does not set PATH_INFO, the user gets a warning explicitly stating that. After your patch, there won't be such a notice and the user does not get any hint. Maybe it's not too bad, though. And I cannot think of a way to make the notice disappear when using `php -S` while keeping it for "real" web servers. Or maybe there is an option to php which tells it to redirect everything to index.php unconditionally and always specify PATH_INFO?
I also think that a note on how to test aurweb patches with `php -S` should be added to the documentation. Maybe in INSTALL, before the "real" setup instructions? Something like:
For testing aurweb patches before submission, you can use [...]
Note that you can only do limited testing using the PHP built-in web server. In particular, the SSH/Git interface will be unusable. For a detailed description on how to setup a full aurweb server, read the instructions below.
Opinions?
That second paragraph is not entirely true, you can still set up an SSH/Git interface separate from the web interface. There's nothing too special about the web interface that the SSH/Git interface depends on.
True, let's replace "SSH/Git interface" with "cgit interface" then.
I think that while we are talking about testing installations, perhaps some information on how to populate a database using sqlite instead of MySQL should be added.
Good idea, maybe some TESTING document in addition to INSTALL (and a reference to TESTING before the actual instructions in INSTALL)? Regards, Lukas
On Wed, 2017-02-08 at 07:30 +0100, Lukas Fleischer wrote:
On Wed, 08 Feb 2017 at 02:33:48, Mark Weiman wrote:
If PATH_INFO is unset, then it will always return "/", so it would be fairly obvious that something's wrong if you are only getting the root page when you visit other pages.
True, it would be obvious that something is wrong. However, it would be harder to pinpoint the actual issue. Right now, if the server does not set PATH_INFO, the user gets a warning explicitly stating that. After your patch, there won't be such a notice and the user does not get any hint.
Can you think of a way where you could have an instance where a rewrite to /index.php/* is done and PATH_INFO is unset? If it isn't done correctly, you would just get your web server's generic 404 page with no notice either (other than this file doesn't exist), so I really don't see why this is important to consider.
Maybe it's not too bad, though. And I cannot think of a way to make the notice disappear when using `php -S` while keeping it for "real" web servers. Or maybe there is an option to php which tells it to redirect everything to index.php unconditionally and always specify PATH_INFO?
To extend, QUERY_STRING is unset for many pages as well when using `php -S` and I keep meaning to see if this happens with a setup on Nginx. This one makes sense because on some pages, there really isn't a QUERY_STRING.
That second paragraph is not entirely true, you can still set up an SSH/Git interface separate from the web interface. There's nothing too special about the web interface that the SSH/Git interface depends on.
True, let's replace "SSH/Git interface" with "cgit interface" then.
Even right now, INSTALL does not mention how to set up cgit either. Perhaps some instruction on that should be added?
I think that while we are talking about testing installations, perhaps some information on how to populate a database using sqlite instead of MySQL should be added.
Good idea, maybe some TESTING document in addition to INSTALL (and a reference to TESTING before the actual instructions in INSTALL)?
I can get to work on writing such a document.
On Wed, 08 Feb 2017 at 18:22:20, Mark Weiman wrote:
Can you think of a way where you could have an instance where a rewrite to /index.php/* is done and PATH_INFO is unset? If it isn't done correctly, you would just get your web server's generic 404 page with no notice either (other than this file doesn't exist), so I really don't see why this is important to consider.
If somebody forgets to setup rewriting and browses the home page, he immediately seems the PATH_INFO warning and knows that something with the PATH_INFO setup might be wrong (just like the current situation with PHP's built-in web server, except that PHP magically fixes the setup itself). Clicking any other link will then result in the 404 page being displayed but at this point, the user has already seen the PATH_INFO warning. However, as I said, getting rid of the warning where it is desired might be a small price to pay for having a much simpler test setup. I am fine with this patch if it is accompanied by a document that describes this setup.
To extend, QUERY_STRING is unset for many pages as well when using `php -S` and I keep meaning to see if this happens with a setup on Nginx. This one makes sense because on some pages, there really isn't a QUERY_STRING.
Yes, QUERY_STRING should be unproblematic, though, because we never rely on it being defined, right?
Even right now, INSTALL does not mention how to set up cgit either. Perhaps some instruction on that should be added?
Sounds like a good idea!
I can get to work on writing such a document.
Awesome, thanks!
participants (2)
-
Lukas Fleischer
-
Mark Weiman