Lullabot / drainpipe

GNU General Public License v3.0
30 stars 14 forks source link

phpdotenv support in autoload, not just autoload-dev #165

Closed hawkeyetwolf closed 1 year ago

hawkeyetwolf commented 1 year ago

The Drainpipe README instructions and composer install warning say to add drainpipe's phpdotenv to autoload-dev in composer.json. Is there a reason it can't/shouldn't go in autoload, making it available in all environments?

I'm using .env.defaults to set the private-files directory path (so that it's available to both shell scripts and php--maybe there's a better way to do that?), but that means it's needed in all environments, not just dev ones.

Also how about if Drainpipe just added the autoload entry to its own composer file? Then support for .env.defaults would get inherited by the "root" project automatically.

The docs say autoload-dev helps avoid "polluting" the root project, but in this case I think it's mostly desirable.

Classes needed to run the test suite should not be included in the main autoload rules to avoid polluting the autoloader in production and when other people use your package as a dependency.

hawkeyetwolf commented 1 year ago

cc @deviantintegral @justafish

deviantintegral commented 1 year ago

I guess it's a question as to if you want environment variables in composer and not just Drupal bootstrap, in which case it would make more sense to load via settings.php.

I also think part of the assumption is that the hosting provider supports environment variables, of which:

hawkeyetwolf commented 1 year ago

I also think part of the assumption is that the hosting provider supports environment variables

Do you mean "supports environment variables" as configured from a UI? I don't follow what's being assumed 😆

deviantintegral commented 1 year ago

In the world of https://12factor.net, it's assumed hosting providers support environment variables like how CI services do. However, the two biggest Drupal hosting providers don't.

deviantintegral commented 1 year ago

We're going to update the readme to make it clear environment variable support is for local / ddev / CI usage only.

justafish commented 1 year ago

Updated in #201