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

added connection to service configuration of RepositorySchema #270

Merged
merged 1 commit into from
Jan 31, 2017
Merged

added connection to service configuration of RepositorySchema #270

merged 1 commit into from
Jan 31, 2017

Conversation

danrot
Copy link
Contributor

@danrot danrot commented Jan 27, 2017

The following issue in Sulu was reported: sulu/sulu#3106

Long story short: Currently utf8 emojis like "🎄" are not working with jackalope-doctrine-dbal, because it uses the limited utf8 charset instead of utf8mb4. I thought I can simply change the charset in doctrine dbal, but jackalope seemed to ignore it.

I found out that the service configuration didn't pass the Connection class, so it was using some default values instead of what has been defined in the DoctrineBundle. So the tables defined in the RepositorySchema did not use the default_table_options defined in the application configuration.

After changing this there was another error when creating the table, for which I will create another PR in jackalope-doctrine-dbal.

I hope simply injecting the default connection is good enough, although I can imagine that there are some usecases where this should be configurable. Maybe we should move that logic to the extension somehow?

@dbu
Copy link
Member

dbu commented Jan 27, 2017

interesting. i am looking forward to use tree icons as node names :-) its still not creating a forest, more a tree of trees :-)

i looked at the code a bit and i feel your solution seems reasonable. unless you configure something specific on the default connection, the behaviour does not change. if you did configure, chances are high you wanted to happen what you now make happen.

i think its correct to introduce this in the master branch which is aliased to 1.4. its more a bugfix than a real BC, but still might give some surprise to somebody that unknowingly relied on this oddity of not using the default connection configuration to set up the db.

can you please add a changelog notice telling that we now use the default connection for setting up the schema? and mention that this is only relevant when you configured connection options for dbal, and will not affect existing databases but only newly created ones?

@dbu dbu merged commit 9898c81 into doctrine:master Jan 31, 2017
@dbu
Copy link
Member

dbu commented Jan 31, 2017

indeed, this does not directly depend on the fix in jackalope as the parameter was accepted before already. thanks for the fix!

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.

2 participants