wecodemore / wpstarter

Easily bootstrap whole site Composer packages for WordPress.
https://wecodemore.github.io/wpstarter/
MIT License
246 stars 35 forks source link

Upgrade to PHP Dotenv v2 #14

Closed franz-josef-kaiser closed 9 years ago

franz-josef-kaiser commented 9 years ago

As mentioned by @Giuseppe-Mazzapica in #12 here, there's a newer version of PHP Dotenv available.

gmazzap commented 9 years ago

I've implemented Dotenv v2 and also added an option to change name of .env file directly from composer.json.

Changes are in dev branch, not merged & tagged because I would like some different pair of eyes look at that.

@franz-josef-kaiser and/or @GaryJones can you review / test dev branch?

GaryJones commented 9 years ago

I'll have a good excuse to try it out tomorrow.

GaryJones commented 9 years ago

I'm getting the following message:

Fatal error: Uncaught exception 'RuntimeException' with message ', , environment variables are required.' in .../htdocs/wp-content/vendor/wecodemore/wpstarter/wpstarter/src/Helpers.php:44 Stack trace: #0 .../htdocs/wp-config.php(28): WCM\WPStarter\Helpers::settings('/srv/www/future...', '.env') #1 .../htdocs/wp/wp-load.php(42): require_once('/srv/www/future...') #2 .../htdocs/wp/wp-blog-header.php(12): require_once('/srv/www/future...') #3 .../htdocs/index.php(17): require('/srv/www/future...') #4 {main} thrown in .../htdocs/wp-content/vendor/wecodemore/wpstarter/wpstarter/src/Helpers.php on line 44

Swapping line 41 of Helpers from:

$set = array_filter(array_intersect_key($required, $settings));

to:

$set = array_filter(array_intersect_key($settings, $required));

seems to work, as then the values from $settings for the three DB_* keys are maintained. As it currently stands, the keys are maintained, but with the empty values from the $required array a few lines up.

GaryJones commented 9 years ago

:x: Omitting the file name arg does not work, since Helpers::settings() default $file arg value is the empty string, despite the function making reference to DB_* settings that are expected to be in that environment file. Would it makes sense to change the default value, since it doesn't appear to be a generic function?

gmazzap commented 9 years ago

Would it makes sense to change the default value, since it doesn't appear to be a generic function?

Yes. I'll do that, thanks.

gmazzap commented 9 years ago

@GaryJones done with https://github.com/wecodemore/wpstarter/commit/34e0f2168584560aeabb33cef4d2bea33e85d2fe (forgot to refer the issue in commit message).

If it works now, I'll merge, thanks.

GaryJones commented 9 years ago

Still getting the same error message:

Fatal error: Uncaught exception 'RuntimeException' with message ', , environment variables are required.' in 
    .../htdocs/wp-content/vendor/wecodemore/wpstarter/wpstarter/src/Helpers.php:44 

Stack trace: 
    #0 .../htdocs/wp-config.php(28): WCM\WPStarter\Helpers::settings('/srv/www/genesi...', '.env') 
    #1 .../htdocs/wp/wp-load.php(42): require_once('/srv/www/genesi...') 
    #2 .../htdocs/wp/wp-blog-header.php(12): require_once('/srv/www/genesi...') 
    #3 .../htdocs/index.php(17): require('/srv/www/genesi...') 
    #4 {main} thrown in .../htdocs/wp-content/vendor/wecodemore/wpstarter/wpstarter/src/Helpers.php on line 44

I checked Helpers, and it definitely shows the new code with the foreach on line 41. You either need:

foreach ($required as $key => $value ) {

or

foreach (array_keys($required) as $key ) {
gmazzap commented 9 years ago

@GaryJones When moved to foreach I had to change $required to contain only variables names as array keys, not values... But I forgot that and left the array as it was when using array_intersect_key.

So no need to change the foreach, but the $required array.

Thanks, and sorry to make you waste your time on testing a so stupid error.

GaryJones commented 9 years ago

What's the merge-to-master timeline for this?

gmazzap commented 9 years ago

@GaryJones Sunday.

gmazzap commented 9 years ago

Finally finished an merged.