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

Allow to use a database DSN instead of all the database variables #414

Merged
merged 9 commits into from Feb 6, 2019

Conversation

Projects
None yet
5 participants
@tristanbes
Copy link
Contributor

tristanbes commented Jan 30, 2019

Hello,

As mentionned on the .README,

Bedrock is inspired by the Twelve-Factor App methodology

So I was surprised, during my 120+ Wordpress migration plan to add the same snippet over and over again to allow to use a DSN instead of all the random legacy Wordpress variables (DB_NAME, DB_USER...).

The 12Factor app is clearly in favor of using DSN: https://12factor.net/backing-services.

Also, when using a recent cloud provider, or PaaS provider, the often expose the database through a DSN (since they follow 12Factor App methodology too).

For those reading the PR and who don't know what a DSN is, it's a string containing the connection information. Ex:

mysql://database_user:database_password@database_host:database_port/database_name

So instead of breaking everything and force people to use DSN instead of legacy variables, this PR adds the support of DSN without breaking existing applications.

Basically, it you set a DATABASE_URL variable, then the code takes care of parsing the URL to set the correct variables.

I've been running tests on local (where DATABASE_URL was not set) and on production (where DATABASE_URL was set) with no issues.

I've updated the example and the documentation too.

Tell me what you think about it.

The algorithm takes care of the following DSN variations:

mysql://john_doe:password@127.0.0.1:3402/my_awesome_blog
mysql://john_doe:password@127.0.0.1/my_awesome_blog
mysql://john_doe@127.0.0.1/my_awesome_blog
@Kocal

This comment has been minimized.

Copy link

Kocal commented Jan 30, 2019

What happens if there is no password or no port in the DSN?

@swalkinshaw

This comment has been minimized.

Copy link
Member

swalkinshaw commented Jan 31, 2019

I'm not necessarily against this, but wanted to point out two things:

  1. that 12 factor page doesn't strictly recommend DSNs. Their image happens to use them but that doesn't mean much.

To the app, both are attached resources, accessed via a URL or other locator/credentials stored in the config. A deploy of the twelve-factor app should be able to swap out a local MySQL database with one managed by a third party (such as Amazon RDS) without any changes to the app’s code.

"other locator/credentials stored in the config" is the key part which is exactly what Bedrock is doing.

  1. the reason why we haven't supported the DSN option is because WordPress itself doesn't. This means we need to do URL parsing like this PR does.

I'm on the fence about this but open to opinions.

Is this something you'd consider trying to contribute to WP core? It should definitely be supported by default but contributing to WP isn't something I'd wish on anyone 😂

@tristanbes

This comment has been minimized.

Copy link
Contributor Author

tristanbes commented Jan 31, 2019

the reason why we haven't supported the DSN option is because WordPress itself doesn't.

They also doesn't support your structure, doesn't support composer, and dotenv. Your edition is "the right way" of doing Wordpress (yes Wordpress, we're not in 2004 anymore). So adding support is one step closer your goal. Also, it doesn't removes the actual way of doing :-), it just remove a little pain for cloud/paas users of bedrock

Also: Nope, I don't want to contribute to Wordpress, they are decades away of good practices when it comes to web development. I imagine PR like this would be left over like the composer support... 😄

@tristanbes

This comment has been minimized.

Copy link
Contributor Author

tristanbes commented Jan 31, 2019

Also, I fixed a case where the code couldn't support a passwordless database connexion. Good catch @Kocal.

@tristanbes

This comment has been minimized.

Copy link
Contributor Author

tristanbes commented Jan 31, 2019

The algorithm takes care of the following DSN variations:

mysql://john_doe:password@127.0.0.1:3402/my_awesome_blog
mysql://john_doe:password@127.0.0.1/my_awesome_blog
mysql://john_doe@127.0.0.1/my_awesome_blog

I think this PR is now ready if you don't have more comments to add.

@QWp6t
Copy link
Member

QWp6t left a comment

This also introduces a few new variables into the global scope. It's not a big deal, but I bring it up in case we want to discuss cleaning them up.

Show resolved Hide resolved config/application.php Outdated
Show resolved Hide resolved config/application.php Outdated
Show resolved Hide resolved config/application.php Outdated
CR
@tristanbes

This comment has been minimized.

Copy link
Contributor Author

tristanbes commented Feb 1, 2019

@QWp6t do you find this version better ?

@QWp6t

QWp6t approved these changes Feb 1, 2019

@tristanbes

This comment has been minimized.

Copy link
Contributor Author

tristanbes commented Feb 4, 2019

What is missing in order to merge and tag a new release of bedrock ? :D

@swalkinshaw

This comment has been minimized.

Copy link
Member

swalkinshaw commented Feb 4, 2019

My question about the required env vars is still valid I think. If you just use DATABASE_URL right now, wouldn't this fail validation?

@tristanbes

This comment has been minimized.

Copy link
Contributor Author

tristanbes commented Feb 4, 2019

@swalkinshaw I've updated the code to allow only DATABASE_URL to be defined

@swalkinshaw
Copy link
Member

swalkinshaw left a comment

Looks good overall.

@austinpray good with you?

Show resolved Hide resolved config/application.php Outdated
@austinpray

This comment has been minimized.

Copy link
Member

austinpray commented Feb 5, 2019

I'll give this a go tomorrow and merge.

@tristanbes

This comment has been minimized.

Copy link
Contributor Author

tristanbes commented Feb 5, 2019

Great news thanks :-), if possible tag a new release right away so we can all benefit from my 2 last PR 🤓

@tristanbes

This comment has been minimized.

Copy link
Contributor Author

tristanbes commented Feb 6, 2019

Is there something missing I can add that prevents this PR to get merged ? :)

@austinpray austinpray merged commit 718f5f7 into roots:master Feb 6, 2019

1 check passed

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

This comment has been minimized.

Copy link
Member

austinpray commented Feb 6, 2019

Sweet, works here!

@austinpray

This comment has been minimized.

Copy link
Member

austinpray commented Feb 6, 2019

Great news thanks :-), if possible tag a new release right away so we can all benefit from my 2 last P

@swalkinshaw @retlehs Let's get #412 in and then tag a new release? 1.12.0?

@tristanbes

This comment has been minimized.

Copy link
Contributor Author

tristanbes commented Feb 7, 2019

Thank you, @austinpray thanks for providing reactive feedback on my work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.