Fix undefined index issue with HTTP_HOST

[fastslack]

Fix undefined index issue with HTTP_HOST. Look #7716

[nikosdion]

While this code is technically correct I don't agree that the issue is an issue at all or that this code is a fix. The actual fix should be made in your code calling JUri. Most likely you need to save the JUri::base() in a hidden config value of your component / plugin / module when it's called up in the front- or back-end and use that in your CLI script. Otherwise you're relying on live_site being populated which is not and MUST NOT be the typical site case as live_site is only meant to be populated on bad hosts which don't report the server's address correctly.

[btoplak]

I don't see any mention of the $live_site variable in the commit code... The commit author and referenced issue #7716 OP talk only about preventing PHP throwing an E_NOTICE because the HTTP Host header wasn't set. Which is a totally valid check. The same type of check for existence of other indexes in $_SERVER superglobal, so why not this too.
I noticed those E_NOTICE error log entries produced by Joomla, which only fill up the server logs.

I agree with the rest of your post, authors must take care of the Host variable when running in CLI.

[mbabker]

This PR as is isn't just doing a check for existence on the index in the superglobal, it's setting a value if the index isn't present. That's worse in my opinion, we shouldn't be altering the globals ever (and ya, I know there's probably some case somewhere outside our unit test suite doing just that). I can live with checking for existence, I will not support merging any patch which is overwriting global values in ANY condition.

[btoplak]

Your point of view is generally valid. And yes, I know there are some places in existing code doing that already, and many worse things ...
But, I'll just add that in this particular case I don't see any issue arising from setting the Host value to a Null. As a matter of fact, AFAIK by HTTP specs Host header must always be set, or else it brakes named vhosts (Apache and Co) functionality when more than one website is hosted on the same IP.

But if someone is willing to add checks in all files that use this variable, that would be best.

[fastslack]

@nikosdion @mbabker The issue that i has is when i run an script using JApplicationWeb over CLI internally to return some websocket response, take in mind that is not good to receive the notice every time that you call the script, some 3 times every second. The notice that i receive is this:

PHP Notice:  Undefined index: HTTP_HOST in /www/libraries/joomla/application/web.php on line 869

In my opinion this notice should be fixed into libraries to prevent this ugly code on every CLI scripts that extends JApplicationWeb:

        // TODO: Fix prevent Joomla! libraries error
        // PHP Notice:  Undefined index: HTTP_HOST
        $_SERVER['HTTP_HOST'] = null;

I understand that is not good to 'overwrite' a superglobal var, but is not an exactly overwrite because it do not exists at the moment to set it null. Its just to prevent the notice error, this do not affect nothing more.

[fastslack]

Other possible fix without overwritting superglobal var:

diff --git a/libraries/joomla/application/web.php b/libraries/joomla/application/web.php
index 735adac..363c34d 100644
--- a/libraries/joomla/application/web.php
+++ b/libraries/joomla/application/web.php
@@ -855,6 +855,8 @@ class JApplicationWeb extends JApplicationBase
                 * properly detect the requested URI we need to adjust our algorithm based on whether or not we are getting
                 * information from Apache or IIS.
                 */
+               // Define variable to return
+               $uri = '';

                // If PHP_SELF and REQUEST_URI are both populated then we will assume "Apache Mode".
                if (!empty($_SERVER['PHP_SELF']) && !empty($_SERVER['REQUEST_URI']))
@@ -863,7 +865,7 @@ class JApplicationWeb extends JApplicationBase
                        $uri = $scheme . $_SERVER['HTTP_HOST'] . $_SERVER['REQUEST_URI'];
                }
                // If not in "Apache Mode" we will assume that we are in an IIS environment and proceed.
-               else
+               else if (isset($_SERVER['HTTP_HOST']))
                {
                        // IIS uses the SCRIPT_NAME variable instead of a REQUEST_URI variable... thanks, MS
                        $uri = $scheme . $_SERVER['HTTP_HOST'] . $_SERVER['SCRIPT_NAME'];

[mbabker]

It still changes global state. Ugly or not, we shouldn't be messing with it, period. Like I said, I can live with checking the key exists, I can also live with just telling developers to set the value themselves if they really need it, but if we accept a patch that is explicitly changing a superglobal value then to me we #^(&ed up royally.

[fastslack]

I understand your point but where the second patch changes global state?

You are forcing all CLI script developers to have to patch this bug themselves. The HTTP_HOST supervariable is included only in Web browsers and not in CLI. They do not need HTTP_HOST at all but will receive this notice.

[Hackwar]

No, we are forcing CLI script devs to use JApplicationCli instead of the web application.

[mbabker]

No, we are forcing CLI script devs to use JApplicationCli instead of the web application.

There are valid cases to instantiate JApplicationWeb objects on CLI (unit tests for one). But the CLI environment definitely cannot be expected to have set all environmental settings as a normal web request would.

The isset check's fine honestly. But this PR in the state it's in now (modifying $_SERVER) should not be accepted as I said before.

[brianteeman]

Set Needs Review so the maintainers can make a decision

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/7757.}

[fastslack]

@mbabker updated this PR to isset fix

[wojsmol]

@fastslack see fastslack#1

[roland-d]

As this PR now no longer changes the super global, it is fine to be merged. Thank you all for your contribution.