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

Always set Doctrine server version & platform #258

Merged
7 commits merged into from
Nov 23, 2017
Merged

Always set Doctrine server version & platform #258

7 commits merged into from
Nov 23, 2017

Conversation

weaverryan
Copy link
Member

@weaverryan weaverryan commented Nov 16, 2017

Q A
License MIT

In order to run cache:warmup, you need to set the server platform and server version. I believe this can be accomplished by using some fake string like DATABASE_URL: 'mysql://?server_version=5.7

In this PR, I attempt to make the DATABASE_URL truly optional during cache:warmup. I do this by committing the server (e.g. mysql) and version (e.g. 5.7) into a config file. This allows us to create a fallback when needed.

On the downside, doctrine.yaml looks a little strange. Also, I suppose it might be less obvious if you forget to specify DATABASE_URL on production (because your connection will fail vs a Missing DATABASE_URL environment variable error).

On the upside, you can once again run cache:warmup with no environment variables at all (very nice for PAAS).

@symfony symfony deleted a comment Nov 16, 2017
# this is useful to allow you to run cache:warmup
# even when your environment variables don't yet exist
# you should not need to change this value
env(DATABASE_URL): '%doctrine.database_server%://'
Copy link
Member Author

Choose a reason for hiding this comment

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

Is it possible to specify a default value for the DATABASE_URL environment value in some other way (i.e. not below parameters)? I could move this to services.yaml as @symfony-flex-server suggests, but it's really better here, as the user should not really configure this.

@symfony symfony deleted a comment Nov 17, 2017
We can do this by using the driver and server_version config. Then, we
can just set DATABASE_URL to blank, to supress the error that it does
not exist. If it *is* set, it overrides the driver option, but they
should match anyways.
@weaverryan
Copy link
Member Author

I've just simplified this a bunch:

A) Things like driver and server_version would now be committed in doctrine.yaml instead of embedded in the env var. I think this makes sense: you really don't want to vary your db driver or version from server to server (but you still could be adding the options to DATABASE_URL).

B) Thanks to this, DATABASE_URL no longer needs to be set for cache warmup. Actually, it does need to be set, but we can safely default it to an empty string.

@@ -10,6 +10,6 @@
"#1": "Format described at http://docs.doctrine-project.org/projects/doctrine-dbal/en/latest/reference/configuration.html#connecting-using-a-url",
"#2": "For an SQLite database, use: \"sqlite:///%kernel.project_dir%/var/data.db\"",
"#3": "Set \"serverVersion\" to your server version to avoid edge-case exceptions and extra database calls",
"DATABASE_URL": "mysql://db_user:[email protected]:3306/db_name?charset=utf8mb4&serverVersion=5.7"
"DATABASE_URL": "mysql://db_user:[email protected]:3306/db_name"
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to mention that one should also not forget to change the doctrine.yaml file? And vice-versa?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. I just changed the #3 comment line to mention doctrine.yaml. But I didn't add a note in doctrine.yaml about .env: it seemed weird, because DATABASE_URL is an env variable, so saying "edit it in .env" is only true in dev mode.... I wasn't convinced if it should be added there or not.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request does not pass validation.

@@ -1,5 +1,17 @@
parameters:
Copy link

Choose a reason for hiding this comment

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

"parameters" should be defined via the "container" configurator instead

@symfony symfony deleted a comment Nov 20, 2017
@symfony symfony deleted a comment Nov 20, 2017
@weaverryan
Copy link
Member Author

Note to self: if this is merged, https://github.com/symfony/symfony-docs/pull/8705/files#diff-d796577eeafc4ece119cf5df3492e5dbR124 can be simplified slightly.

@rosier
Copy link
Contributor

rosier commented Nov 21, 2017

I'm not sure if the pros outweigh the cons here. The use case also looks a bit too specific.

Maybe it's better to add this to the docs as an example. WDYT?

@weaverryan
Copy link
Member Author

The use-case is a little bit specific, but I think it'll be more and more common (it's standard practice on a PAAS like platform.sh or Heroku to warm up the cache during a build stage where no env parameters are added). This would make it work for everyone automatically instead of PAAS users needing to do extra.

I don't think there's much downside:

  • For driver, if you don't realize the config is present and choose a conflicting DATABASE_URL, (e.g. one that uses pgsql) the DATABASE_URL wins

  • For server_version, if you don't realize this is here, you could possibly get some weird behavior if you connect to a DB with a different version. That is also true of the current recipe, though it would be a bit easier to miss with this change.

  • Putting the charset in config is a homerun I think :)

Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

merge now

@ghost ghost merged commit 2efa651 into symfony:master Nov 23, 2017
ghost pushed a commit that referenced this pull request Nov 23, 2017
This pull request was closed.
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.

5 participants