search

silverstripe / cwp-core

CWP basic compatibility module
BSD 3-Clause "New" or "Revised" License
3 stars 12 forks source link

Use putenv() in InitialisationMiddleware #61

Closed ichaber closed 5 years ago

ichaber commented 5 years ago

This commit it based on changing the order of execution in InitialisationMiddleware. It proposes to change the setting of the environment variables for http_proxy, https_proxy and NO_PROXY from Environment::setEnv() back to putenv() as it was in CWP1.

As described in the documentation of InitialisationMiddleware its main purpose is to configure the proxy so it is automatically picked up when calling curl in an apache context. I'm aware of the multithreaded issue described in https://github.com/laravel/framework/issues/7354, but considering that these variables should be present anyway, I don't consider that to be an issue in context of CWP. Maybe @tractorcow has some insights into that?

Otherwise the variables are redundant as SS_OUTBOUND_PROXY, SS_OUTBOUND_PROXY_PORT and NO_PROXY already exist.

robbieaverill commented 5 years ago

I assume the tests are failing since Environment::getEnv('http_proxy' has been called once already in the unit test class, so subsequent requests no longer read from getenv()

ichaber commented 5 years ago

Yeah, it looks like the testConfigureEgressProxyDomainExclusions fails on re-setting the environment, because it's not using the Environment class anymore. 🤔

robbieaverill commented 5 years ago

You could maybe putenv('https_...', '') in setUp() of that class to work around it

ichaber commented 5 years ago

That wouldn't actually resolve the issue. If we use Environment::setEnv() with a key, there will always be a key in static::$env and the getEnv will never fall through to the getenv() call.

chillu commented 5 years ago

Do we need to add this to the CWP upgrading guide?

robbieaverill commented 5 years ago

It's technically a regression - I think we'll retarget this at 2.0 and merge it up so future releases will configure the proxy automatically. For now though we have the External HTTP requests with proxy docs for configuring this, and our supported modules implement the proxy explicitly when using Guzzle for example.

@indygriffiths previously removed the mention of auto-configured cURL requests in https://github.com/silverstripe/cwp/pull/171.

I'd like to get this in as soon as CWP 2.2.2 is released.