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

Pass ENV to SymfonyProcess via getenv() call. #3734

Merged
merged 2 commits into from
May 21, 2021
Merged

Pass ENV to SymfonyProcess via getenv() call. #3734

merged 2 commits into from
May 21, 2021

Conversation

michaelr0
Copy link
Contributor

Aim's to fix issue with installing/updating Addons or Statamic via the Statamic Control Panel.

For those who get the error:

In Factory.php line 677:
                                                                               
  The HOME or COMPOSER_HOME environment variable must be set for composer to   
  run correctly   

As referenced in #3383

Addon installation is currently now working.
However there are issues with the Statamic Core updates, which appear to remove the contents of vendor/statamic/cms

I'm not sure where to start in regards to the removal issue, so feel free to chime in.

Thanks.

@jasonvarga
Copy link
Member

What do you mean it removes the contents?

@michaelr0
Copy link
Contributor Author

What do you mean it removes the contents?

My guess is that during the update process of statamic/cms, composer was removing the contents of the vendor/statamic/cms folder in preperation to replace it with requested version, but this caused the update process and php artisan/php please to malfunction due to the missing statamic/cms dependency.

However it appears that I'm now unable to replicate that issue, very odd.

I've tested on a M1 Mac Mini (PHP 8.0 via Homebrew) and a Laptop (Ubuntu 20.04, PHP 7.4) both using php artisan serve and the above change to push the ENV to SymfonyProcess has resolved the error below (for me)

In Factory.php line 677:
                                                                               
  The HOME or COMPOSER_HOME environment variable must be set for composer to   
  run correctly  

@jasonvarga
Copy link
Member

This change looks like it works, but from what I read on the Symfony docs, it should inherit all the env vars by default.

https://symfony.com/doc/current/components/process.html#setting-environment-variables-for-processes

Before merging this, I'd like to get a better understanding of why it's not automatically inheriting the vars, and if there's a security concern with just passing getenv() into it. It's probably fine, but feels mildly scary.

@michaelr0
Copy link
Contributor Author

michaelr0 commented May 19, 2021

Thanks Jason, yes that's a solid point.
I actually wondered if it would be passing API keys etc from the .Env file, but had assumed that it would have normally been doing that anyway.

Agreed, it says it should automatically get it, but for whatever reason it isn't.

It looks like SymfonyProcess generates the default ENV via: https://github.com/symfony/process/blob/5.4/Process.php#L1656

    private function getDefaultEnv(): array
    {
        $env = [];

        foreach ($_SERVER as $k => $v) {
            if (\is_string($v) && false !== $v = getenv($k)) {
                $env[$k] = $v;
            }
        }

        foreach ($_ENV as $k => $v) {
            if (\is_string($v)) {
                $env[$k] = $v;
            }
        }

        return $env;
    }

And merging it with the passed $env https://github.com/symfony/process/blob/5.4/Process.php#L308

public function start(callable $callback = null, array $env = [])
        .......
        if ($this->env) {
            $env += $this->env;
        }

        $env += $this->getDefaultEnv();

@jasonvarga
Copy link
Member

I saw that you suggested passing in an array with just HOME in it, but you removed it from your comment. Why? That sounds like a safer thing and I'd be happy to merge that right in.

@michaelr0
Copy link
Contributor Author

I saw that you suggested passing in an array with just HOME in it, but you removed it from your comment. Why? That sounds like a safer thing and I'd be happy to merge that right in.

Hi Jason, yeah sorry, I haven't had a chance to test that yet, I'm unsure at this stage if anything else is needed to be passed as well, I'll test it shortly and come back to you.

I had looked into the Symfony Process code as above and I wondered if it would normally have access to the contents of the .env file ? But it looks like composer doesn't use the .env file, but rather the contents are loaded outside of composer via Laravel https://github.com/laravel/framework/blob/8.x/src/Illuminate/Foundation/Bootstrap/LoadEnvironmentVariables.php

@michaelr0
Copy link
Contributor Author

Hi Jason,

I have changed it to

   ['HOME' => getenv('HOME')]

I have tested upgrading and downgrading Statamic and installing, upgrading, downgrading and uninstalling Addon's

All appears functional with php artisan serve.

Thanks.

Copy link
Member

@jasonvarga jasonvarga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants