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

#473 Add feature to load server environment variables from special file #474

Merged
merged 3 commits into from Jan 2, 2019

Conversation

Projects
None yet
7 participants
@MartinPeverelli
Copy link
Contributor

MartinPeverelli commented Nov 24, 2017

Proposal for #473

Closes #473
Closes #146

@Lloople

This comment has been minimized.

Copy link

Lloople commented Feb 13, 2018

Hi @adamwathan @mattstauffer, is there any reason for not merge this pull request? I found that I need to set env variables per site and can't find a proper way. Adding this .env.valet file into the project looks pretty neat.

Thank you

EDIT: I'm talking about not-laravel projects

@mattstauffer

This comment has been minimized.

Copy link
Contributor

mattstauffer commented Feb 14, 2018

@Lloople I don't see anything particularly wrong with the PR. What I did was close obvious "no" PRs, shut down all driver requests for now, and leave PR's like these open for Adam and Taylor to merge when they have some time to look over them and decide whether they fit within the spirit/intent of Valet. I think this being open is not a "no" but just a "taylor and adam haven't had time to dig into it yet".

@iboldurev

This comment has been minimized.

Copy link

iboldurev commented Mar 6, 2018

Please add this implementing

@osrecio

This comment has been minimized.

Copy link

osrecio commented Jun 5, 2018

Hi, I think is a good approach for some kind of projects to have env variables and load it inside a project.

Totally agree to merge it @mattstauffer

@bramus

This comment has been minimized.

Copy link

bramus commented Jul 6, 2018

Nice work! Would love to see this one get merged.

@drbyte

This comment has been minimized.

Copy link
Contributor

drbyte commented Sep 18, 2018

Notwithstanding the merge conflict here (which I think is a simple fix), and that this PR touches most drivers, I like this.
I think it provides an easy-to-use-and-understand way of providing ENV variables on a per-project basis, beneficial specifically to those using Valet.

One concern: naming the file .env.valet could be confusing since .env files are typically expected to contain settings in a prescribed VAR=setting format, but this PR expects the file to be a PHP file containing an array declaration.
Thus I propose calling it .valet-config.php or maybe .valet-env.php

@@ -44,10 +44,12 @@ public function isStaticFile($sitePath, $siteName, $uri)
*/
public function frontControllerPath($sitePath, $siteName, $uri)
{
$this->loadServerEnvironmentVariables($sitePath, $siteName);

This comment has been minimized.

@mattstauffer

mattstauffer Sep 19, 2018

Contributor

Since this seems to be happening in every frontControllerPath method, is it possible to instead just run this in server.php right before it calls frontControllerPath? (I don't know the code super well so there may be a good reason why not...)

This comment has been minimized.

@drbyte

drbyte Sep 19, 2018

Contributor

Technically it probably could.

valet/server.php

Lines 134 to 147 in 0323607

/**
* Attempt to dispatch to a front controller.
*/
$frontControllerPath = $valetDriver->frontControllerPath(
$valetSitePath, $siteName, $uri
);
if (! $frontControllerPath) {
show_valet_404();
}
chdir(dirname($frontControllerPath));
require $frontControllerPath;

@MartinPeverelli MartinPeverelli force-pushed the MartinPeverelli:server_environment_variables branch from fea72f9 to a603114 Sep 24, 2018

@MartinPeverelli

This comment has been minimized.

Copy link
Contributor Author

MartinPeverelli commented Sep 24, 2018

@mattstauffer @drbyte I took both your advices and refactored this idea.
The file is now .valet-env.php, which makes it obvious that it should contain a PHP array (instead of the key=value pairs of regular dotfiles).
And I changed the method to be public and called it from server.php as requested, which allowed me to revert all the changes to all the Drivers.

Let me know what you think!

If this gets merged into master, I would also love to help with the documentation of this feature, how can I do that?

}
foreach ($variables[$siteName] as $key => $value) {
$_SERVER[$key] = $value;

This comment has been minimized.

@Lloople

Lloople Sep 26, 2018

Why not using putenv($key.'='.$value) here along with the already existing $_SERVER? 🤔

This comment has been minimized.

@MartinPeverelli

MartinPeverelli Sep 26, 2018

Author Contributor

I was only trying to mimic Apache's way of loading env vars on the vhosts files, and if I'm not mistaken, that only sets values on$_SERVER.
I don't know if duplicating the values on both $_SERVER and $_ENV is useful or desirable, but as you say, we can add that single line to the loop if we do.

What say the rest of us?

This comment has been minimized.

@drbyte

drbyte Sep 27, 2018

Contributor

I have no objection.

@drbyte

This comment has been minimized.

Copy link
Contributor

drbyte commented Sep 27, 2018

@MartinPeverelli have you considered a way to allow for wildcard domain-names with this?

I have a use-case where simply dropping in a non-domain-specific .valet-env.php file would make environment overrides easier. But I don't want to have to edit the file to set domain name/s inside it.
It's admittedly a little different use-case than your original need which was to do domain-specific custom overrides.

@drbyte

This comment has been minimized.

Copy link
Contributor

drbyte commented Nov 7, 2018

I've used this proposed PR on a project. It worked well.

I do think altering it to allow for being domain-agnostic would be nice too.

Updated loading of server environment variables
- added `putenv()` for Laravel compatibility
- added `$_ENV` for generic compatibility
- added wildcard processing, so site array named `*` gets processed always (if present), and then site-specific entries are added and will override the wildcard.

Sample `.valet-env.php`:
```php
<?php

return [
    '*' => [
        'USER' => 'vagrant',
    ],
    'demo' => [
        'MY_CUSTOM_VAR' => 'special_value',
        'USER' => 'travis',
    ],
];
```
(Note: order of entries in the array is irrelevant, as the parser reads `*` first, followed by site-specific entries.)
@drbyte

This comment has been minimized.

Copy link
Contributor

drbyte commented Dec 28, 2018

Waiting for @MartinPeverelli to merge my PR to his valet fork, which will update this PR, and then this could be merged.

Merge pull request #1 from drbyte/patch-1
Updated loading of server environment variables
@MartinPeverelli

This comment has been minimized.

Copy link
Contributor Author

MartinPeverelli commented Dec 28, 2018

@drbyte merged your PR into my fork! Sorry for the delay, hectic RL.

@drbyte

This comment has been minimized.

Copy link
Contributor

drbyte commented Dec 28, 2018

Thanks @MartinPeverelli

I've been using this code for the past 3 weeks, and haven't run into any problems. It's definitely been helpful to keep project-specific environment variable configs in a separate .valet-env.php file in my app, instead of having to touch the valet-generated nginx configs.

@mattstauffer IMO this is safe to merge.

@mattstauffer

This comment has been minimized.

Copy link
Contributor

mattstauffer commented Jan 2, 2019

Should we possibly add a PR to the docs while we're at it?

@mattstauffer mattstauffer merged commit 67546f1 into laravel:master Jan 2, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mattstauffer

This comment has been minimized.

Copy link
Contributor

mattstauffer commented Jan 16, 2019

I think I got mixed up and thought this was adding .env.valet to each application, not env-valet.php to the root. Is there a reason we went this way? I was about to release but this feels weird.

@MartinPeverelli

This comment has been minimized.

Copy link
Contributor Author

MartinPeverelli commented Jan 16, 2019

The change to the file name was because the contents are a PHP array, instead of the key=value pair on the other .env files.

@mattstauffer

This comment has been minimized.

Copy link
Contributor

mattstauffer commented Jan 16, 2019

@MartinPeverelli sure... and you had to do PHP so you can differentiate them per domain. I remember now. Thanks!

@drbyte

This comment has been minimized.

Copy link
Contributor

drbyte commented Jan 16, 2019

Note: The filename is .valet-env.php

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment