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

Adding support for dotenv using the -c config file option #1236

Closed
wants to merge 9 commits into from
Closed

Adding support for dotenv using the -c config file option #1236

wants to merge 9 commits into from

Conversation

phillipsharring
Copy link

@phillipsharring phillipsharring commented Nov 5, 2017

Idea

  1. josegonzalez/dotenv is required from composer.json. The 2.* version was preferred to have consistency with the cakephp/app composer requirements.
  2. And .env.example file is included with example config variables. This would be copied to .env and edited by the developer
  3. A phinx.config.php.dist file is included with an example config array. This would be copied to phinx.config.php and edited by the developer to include config arrays for various environments, along with the default database config. This file references the getenv() function, so the values are read from .env. The array keys in this config file are the same as in phinx.yml.
  4. .gitignore has been updated to include .env and *.config.php files. The name .env is a standard name for the dotenv package. The phinx.config.php file could be named and placed anywhere. The .config.php extension is just a suggestion to make ignoring the config files easier. This file could be phinx.config.php in the root, or configs/phinx.config.php or anywhere, really.

Usage

When running phinx at the command line, the -c or --configuration flag is used and the value is pointed at the phinx.config.php file, like so:

$ ./bin/phinx migrate  -e development -c phinx.config.php

The josegonzalez\Dotenv\Loader class is only loaded in the configuration file, so if the configuration file is not used, that class won't be loaded.

Again, the naming of the config file is merely a suggestion.

Note: Correct me if I'm wrong, but I don't think this needs any tests, since loading a config file is already tested. If it needs tests, I'll add them.

@codecov-io
Copy link

codecov-io commented Nov 5, 2017

Codecov Report

Merging #1236 into master will decrease coverage by 0.22%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1236      +/-   ##
============================================
- Coverage     74.84%   74.62%   -0.23%     
- Complexity        0     1965    +1965     
============================================
  Files            35       54      +19     
  Lines          4783     5817    +1034     
============================================
+ Hits           3580     4341     +761     
- Misses         1203     1476     +273
Impacted Files Coverage Δ Complexity Δ
src/Phinx/Db/Adapter/TimedOutputAdapter.php 18.88% <0%> (-34.72%) 46% <0%> (+46%)
src/Phinx/Db/Adapter/MysqlAdapter.php 90.04% <0%> (-8.68%) 221% <0%> (+221%)
src/Phinx/Db/Adapter/AdapterWrapper.php 61.6% <0%> (-4.8%) 53% <0%> (+53%)
src/Phinx/Db/Adapter/PdoAdapter.php 87.72% <0%> (-4.3%) 90% <0%> (+90%)
src/Phinx/Migration/AbstractMigration.php 96.1% <0%> (-3.9%) 35% <0%> (+35%)
src/Phinx/Db/Table/Column.php 85.21% <0%> (-3.2%) 48% <0%> (+48%)
src/Phinx/Migration/Manager.php 88.04% <0%> (-2.28%) 144% <0%> (+144%)
src/Phinx/Db/Adapter/PostgresAdapter.php 92.44% <0%> (-1.31%) 223% <0%> (+223%)
src/Phinx/Db/Adapter/AbstractAdapter.php 79.68% <0%> (-0.65%) 26% <0%> (+26%)
src/Phinx/Db/Adapter/SqlServerAdapter.php 0% <0%> (ø) 190% <0%> (+190%) ⬆️
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3f3a0a...435210f. Read the comment docs.

composer.json Outdated
@@ -27,7 +27,8 @@
"php": ">=5.4",
"symfony/console": "~2.8|~3.0|~4.0",
"symfony/config": "~2.8|~3.0|~4.0",
"symfony/yaml": "~2.8|~3.0|~4.0"
"symfony/yaml": "~2.8|~3.0|~4.0",
"vlucas/phpdotenv": "^2.4"
Copy link
Member

@ADmad ADmad Nov 5, 2017

Choose a reason for hiding this comment

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

The cakephp skeleton app uses josegonzalez/dotenv for providing dotenv support. While phinx is standalone lib it would be better if projects under the cakephp family used same external libs when possible.

Copy link
Author

Choose a reason for hiding this comment

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

Good idea. I didn't know that. I'll change this to use the josegonzalez/dotenv library soon today.

Copy link
Author

Choose a reason for hiding this comment

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

I made this change. Went with "2.*" to be consistent with cakephp.

composer.json Outdated
@@ -28,7 +28,7 @@
"symfony/console": "~2.8|~3.0|~4.0",
"symfony/config": "~2.8|~3.0|~4.0",
"symfony/yaml": "~2.8|~3.0|~4.0",
"vlucas/phpdotenv": "^2.4"
"josegonzalez/dotenv": "2.*"
Copy link
Member

Choose a reason for hiding this comment

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

Why not semver?

Copy link
Author

Choose a reason for hiding this comment

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

I picked this version to be consistent with the version used in cakephp/app.

@chinpei215
Copy link
Contributor

Relates to #1085

@dereuromark
Copy link
Member

Looks like we can move forward once the conflict is resolved 👍

@@ -36,3 +36,9 @@ phpunit.xml

# sqlite test database
phinx_testing.sqlite3

# ignore dotenv .env file
.env
Copy link
Member

Choose a reason for hiding this comment

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

why is this needed? shouldnt this only be relevant for project's config files, not for inside vendor?

Copy link
Author

Choose a reason for hiding this comment

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

The dotenv file (which is a config file) is typically placed in the root, and is ignored, because it will most likely be different in every environment an application is deployed to.

The .env.example is committed to give people a starting point. The naming of this file is also fairly standard across various project.

I'm not sure what you're asking about the vendor directory.

Copy link
Member

@dereuromark dereuromark Mar 1, 2018

Choose a reason for hiding this comment

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

vendor/cakephp/phinx/.gitignore => this file
How does that relate to .env files that are in root dir?

Does this PR create a tmp .env file here in the plugin directly for testing etc?

Copy link
Member

Choose a reason for hiding this comment

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

This is for development of the project. if someone is working with phinx directly and using .env files, then this is useful for them not accidentally committing local items.

The bit where cakephp puts it in a config directory is kinda silly - but hey, we listen to kooks all the time!

As far as users ignoring it in their projects, we should almost certainly just document that the file should be gitignored as a best practice.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good.

composer.json Outdated
@@ -28,7 +28,8 @@
"php": ">=5.4",
"symfony/console": "^2.8|^3.0|^4.0",
"symfony/config": "^2.8|^3.0|^4.0",
"symfony/yaml": "^2.8|^3.0|^4.0"
"symfony/yaml": "^2.8|^3.0|^4.0",
"josegonzalez/dotenv": "2.*"
Copy link
Member

Choose a reason for hiding this comment

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

Does anyone think this should be require-dev as optional one for those that do not use/want it?

Copy link
Author

Choose a reason for hiding this comment

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

The migrations will probably be run in every environment, not just while developing, right? In which case, I think this should be a requirement, not a dev requirement. Just my 2 cents but I maybe wrong in my understanding.

Copy link
Member

Choose a reason for hiding this comment

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

A dev requirement (along with suggest text) means that the functionality is there but dormant.
Once you require it in your root level (project application), it then becomes a real functionality.
So no, this should be up to the dev IMO. Along with a documentation on how to enable this of course.

Copy link
Author

Choose a reason for hiding this comment

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

OK :)

Copy link
Author

Choose a reason for hiding this comment

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

Changed.

@@ -0,0 +1,38 @@
<?php

// copy this file to phinx.config.php, or your preferred name/location
Copy link
Member

Choose a reason for hiding this comment

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

Should we add the note here? composer require ... ?

Copy link
Member

Choose a reason for hiding this comment

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

We can probably add a note when installing the library somehow, though not sure where the best place for that would be.

@@ -33,7 +33,8 @@
"require-dev": {
"phpunit/phpunit": "^4.8.35|^5.7|^6.5",
"sebastian/comparator": ">=1.2.3",
"cakephp/cakephp-codesniffer": "^3.0"
"cakephp/cakephp-codesniffer": "^3.0",
"josegonzalez/dotenv": "2.*"
Copy link
Member

Choose a reason for hiding this comment

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

This can be 3.x.

@josegonzalez
Copy link
Member

Needs docs.

'environments' => [
'default_migration_table' => 'phinxlog',
'default_database' => getenv('APP_ENV'),
'develop' => [

Choose a reason for hiding this comment

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

I think this should match the APP_ENV value in .env.example - "development"

Copy link
Author

Choose a reason for hiding this comment

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

Good catch. Thanks!

@lorenzo lorenzo self-assigned this Oct 22, 2018
Copy link

@williamdes williamdes left a comment

Choose a reason for hiding this comment

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

🏓

@othercorey
Copy link
Member

Closing due to no activity.

@othercorey othercorey closed this May 8, 2020
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.