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

Make DSN implementation more uniform #415

Merged
merged 5 commits into from
Feb 10, 2019
Merged

Make DSN implementation more uniform #415

merged 5 commits into from
Feb 10, 2019

Conversation

Log1x
Copy link
Sponsor Member

@Log1x Log1x commented Feb 8, 2019

Figured I'd propose this change as the implementation is still early so despite being a "breaking change" – very few would be affected.

  • Change DATABASE_URL to DB_URL to remain uniform with the other database variables
  • Improve phrasing on the README and .env.example DocBlock
  • Minor golfing / cleanup on how the DSN is handled in application.php
  • Apparently editorconfig found some trailing whitespace in the README 🙀

@swalkinshaw
Copy link
Member

While DB_URL is more consistent with the other constants, DATABASE_URL is actually kind of standard. Lots of hosting platforms use that name specifically and some other web frameworks have it integrated as well.

All the other changes look good though 👍

@Log1x
Copy link
Sponsor Member Author

Log1x commented Feb 8, 2019

Want to wait for more feedback or should I change it?

Drupal (lol) uses DB_URL – Laravel, etc. doesn't use DSN which isn't a surprise, it isn't user-friendly and there is no definitive standard (e.g. PDO: mysql:host=localhost;dbname=example, Perl: DBI:mysql:database=$database;host=$hostname;port=$port).

Otherwise, I mainly just see Rails/Django defining DATABASE_URL.

@swalkinshaw
Copy link
Member

Heroku and Doccu use DATABASE_URL. There's probably other hosts/infra related things which do as well.

@tristanbes
Copy link
Contributor

tristanbes commented Feb 8, 2019

I'm not in favor of changing DATABASE_URL to DB_URL

I would stick to DATABASE_URL since a lot of cloud providers are using this to expose their DSN. (Scalingo uses it, clever-cloud uses it and other international ones like @swalkinshaw reported).

PS: Frameworks also tends to use DATABASE_URL like for instance Symfony

.env.example Outdated Show resolved Hide resolved
.env.example Outdated Show resolved Hide resolved
config/application.php Outdated Show resolved Hide resolved
@austinpray
Copy link
Contributor

+1 for DATABASE_URL

Add null coalesce operator to DB_ variables to allow them to be removed
config/application.php Outdated Show resolved Hide resolved
config/application.php Outdated Show resolved Hide resolved
@QWp6t
Copy link
Sponsor Member

QWp6t commented Feb 10, 2019

lgtm @swalkinshaw

@swalkinshaw swalkinshaw merged commit cfb68e1 into roots:master Feb 10, 2019
@swalkinshaw
Copy link
Member

Thank you everyone

We also need the docs updated at https://github.com/roots/docs/blob/docs/bedrock/installing-bedrock.md if someone wants to help

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants