Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use putenv() in InitialisationMiddleware #61

Merged
merged 5 commits into from
Mar 11, 2019

Conversation

ichaber
Copy link
Contributor

@ichaber ichaber commented Mar 8, 2019

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 laravel/framework#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
Copy link
Contributor

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
Copy link
Contributor Author

ichaber commented Mar 8, 2019

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

@robbieaverill
Copy link
Contributor

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

@ichaber
Copy link
Contributor Author

ichaber commented Mar 8, 2019

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
Copy link
Member

chillu commented Mar 11, 2019

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

@robbieaverill
Copy link
Contributor

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 silverstripe/cwp#171.

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

@robbieaverill robbieaverill changed the base branch from master to 2.0 March 11, 2019 22:52
ichaber and others added 4 commits March 12, 2019 11:53
@robbieaverill robbieaverill force-pushed the fix/curlexec_proxyenvs branch from d278a74 to 03f32c8 Compare March 11, 2019 22:54
@robbieaverill robbieaverill merged commit 3b4e158 into silverstripe:2.0 Mar 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants