Remove charset config for doctrine-bundle v2 #693
Conversation
ghost
deployed
to
| # IMPORTANT: You MUST configure your db driver and server version, | ||
| # either here or in the DATABASE_URL env var (see .env file) | ||
| #driver: 'mysql' | ||
| #server_version: '5.7' |
There was a problem hiding this comment.
What about removing everything here? Or put another way: why would one want to configure the driver and version here instead of in the DATABASE_URL env var?
There was a problem hiding this comment.
Also, the DSN are like: mysql://..., postgresql://..., sqlite://... So, why do I have to define the driver explicitly? It's just the first word of the DSN, right?
There was a problem hiding this comment.
you might want to configure the version here in case your PaaS does not expose a serverVersion parameter in the query string (which is a Doctrine DBAL convention). This happens on Heroku for instance.
There was a problem hiding this comment.
@dzuelke WDYT about this? Could Heroku include the serverVersion in the DATABASE_URL?
There was a problem hiding this comment.
I removed the "driver" line but kept "server_version" for now.
There was a problem hiding this comment.
We can't, since we don't offer MySQL, @nicolas-grekas ;) That's probably a question for the folks at ClearDB and JawsDB. I generally think that the server version is more of a "static" environment thing, since your code is probably also written against the specific capabilities of a version series, so it shouldn't differ between environments, unlike the DSN.
There was a problem hiding this comment.
That's not only for MySQL - the same is needed for PG :)
But I get your point. Actually, DoctrineBundle/DBAL could throw an exception when both settings are found but don't match (in DSN & config). That would be even more useful as it would help spot mistakes.
/cc @alcaeus in case you agree and want to create an issue there :)
|
@nicolas-grekas does this need a rebase after the changes in #695? |
|
No rebase needed,this is ready to go. |
Requires doctrine/DoctrineBundle#1070 before merging.