[TASK] set env variables based on $_SERVER

[lewisvoncken]

To run PWA Studio through Upward there are 2 variables which need to be set and this is an alternative method compared to the getenv which can't be correctly updated with putenv because the local env variable is not available when calling getenv.

This PR is related to the magento 2.3 core PR magento/magento2#23126

[tjwiebell]

@lewisvoncken - Do you have a bug or issue to support this change? It was several months ago I wrote this, and I do recall some debate went into getenv vs $_SERVER, but I don't recall why we decided on getenv. I believe it was easier to mock getenv in unit tests, and there was some issue with $_SERVER when using php -S for running the test spec (npm run test:spec).

Happy to discuss further if you could provide more context.

[lewisvoncken]

@tjwiebell

The issue is when you don't use commerce cloud you have to also be able to set these variables and that could be done through putenv but these won't be returned when you use getenv because they are set through a local putenv in the index.php as you can see in the other PR.

[tjwiebell]

In my local environment, I've injected Venia environment variables into index.php with putenv. I use the vagrant setup locally which uses Apache, if you could provide some more information about your local setup. All of my local testing has also used the connector module for M2, so maybe this setup is a bit more unique. Is your upward server separate from your M2 instance in this use case?

[lewisvoncken]

@tjwiebell

My setup is with nginx can you please test that also?

[tjwiebell]

@tjwiebell

My setup is with nginx can you please test that also?

Thank you, I am able to reproduce now. Appears we're running into the SAPI caveat when using getenv, which doesn't return locally set variables. There is a local_only flag, but it only works for single variable retrieval (eg. getenv('MAGENTO_BACKEND_URL', true) // works).

Outside of this caveat, there should be no difference between getenv and $_SERVER, so I have no issue accepting this change; but it has broken all tests. If you're interested in fixing the tests to be compatible with the $_SERVER superglobal, please proceed, otherwise I'll get this added to our next replenishment and we can fix up the tests.

Before committing to this change though, I'm going to research how our cloud handles environment variables, and see if that's an alternative to injecting via index.php.

[tjwiebell]

@lewisvoncken - Are you comfortable injecting variables via nginx config instead? This addition seems to work for me, and makes variables available with getenv.

fastcgi_param MAGENTO_BACKEND_URL http://magento2.test/;

[lewisvoncken]

@tjwiebell the reason I want to use $_SERVER variable is that you can fill the url with the HTTP_HOST so you eventually can start using multi domains but this can also be done in nginx but in my opinion that is extra unnecessary configuration because these can dynamically be set with the HTTP_HOST.

This dynamic setter I have added in the other PR but can also be set in the upward-php lib or module through the $_SERVER variables.

can be set as:

$_SERVER['MAGENTO_BACKEND_URL'] = $bootstrap->getMagentoBackendUrl();
$_SERVER['NODE_ENV'] = $bootstrap->getMode();

and then the getMagentoBackendUrl contains:

    public function getMagentoBackendUrl(): string
    {
        $protocol = isset($_SERVER['HTTP_HOST']) && $_SERVER['HTTP_HOST'] === 'on' ? 'https' : 'http';
        return "{$protocol}://{$_SERVER['HTTP_HOST']}";
    }

and the NODE_ENV has to still be set based on the mode which Magento is running in or else you have to configure that one twice because that variable is MAGE_MODE

    public function getMode()
    {
        $mode = 'default';
        if (isset($this->server[State::PARAM_MODE])) {
                $mode = $configMode;
            }
        }
        return $mode;
    }

What is the ideal situation? @dheesbeen

[tjwiebell]

@lewisvoncken - Magento core seems to have established that $_SERVER is the preferred way for interfacing with environment variables, so it seems reasonable to make this change to support FastCGI setups better (phpdotenv would be a better alternative, but doesn't help us now). Were you interested in fixing the tests, or did you want our team to look into that?

[lewisvoncken]

@tjwiebell

for the test the base url isn't used but mainly the UPWARD_PHP_UPWARD_PATH which is a global environment variable which can be set through several different ways but for the test it is possible to merge getenv into the $_SERVER variables. And for the url 127.0.0.1 is used

If you have a better idea let me know. Thank you in advance.

[tjwiebell]

@lewisvoncken - Made some minor adjustments in 2672341 to get all tests passing and cleanup some logic, but this change looks good to me. The team goes through replenishment every Monday where we commit to our weekly delivery, so I'll get this added to our queue so it can be reviewed and QA'd. Thanks for your contribution.

[lewisvoncken]

@tjwiebell do you have an update about the review of this pr?

[lewisvoncken]

@tjwiebell and @zetlen
Did you by any change had time to check this?

[zetlen]

Hi @lewisvoncken! Tommy has gone on parental leave, but he made sure to hand off these important tickets internally. I'm not the perfect one to review this as code, but I understand from the discussion that it's important to expedite. We'll make sure it falls into the right hands, and soon.

[paliarush]

Looks good from code point of view. Someone need to test.