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 defaultTableOptions to be configured #420

Merged
merged 11 commits into from
Nov 4, 2015
Merged

Allow defaultTableOptions to be configured #420

merged 11 commits into from
Nov 4, 2015

Conversation

DHager
Copy link
Contributor

@DHager DHager commented May 5, 2015

Change configuration metadata to unlock the use of defaultTableOptions, so that defaults can be passed to the schema-tool. In the case of MySQL, possible defaults include charset, collation, and engine.

The array-data is taken up here and then applied by individual platforms.

@DHager
Copy link
Contributor Author

DHager commented May 21, 2015

I'm assuming that documentation for this option will involve a separate, future PR against the separate symfony-docs repo.

So besides that, is there anything else I can do to help move this feature forward?

@stof
Copy link
Member

stof commented May 22, 2015

@DHager Please add the documentation in the Resources/doc folder

@DHager
Copy link
Contributor Author

DHager commented May 22, 2015

@stof OK, I added some samples to the documentation folder. I can't enumerate all the possible options because they are situation-dependent on some DBAL platform internals.

Stylistically, default_table_options (in YAML) and default-table-option (in XML) would look a lot nicer, but the DBAL layer would still be looking for defaultTableOptions, and I'm not certain what the best mechanism would be for doing that conversion.

@stof
Copy link
Member

stof commented May 28, 2015

@DHager we already have such a mechanism in the DoctrineExtension to rename keys to camelCase

@@ -153,6 +153,11 @@ private function getDbalConnectionsNode()
->useAttributeAsKey('name')
->prototype('scalar')->end()
->end()
->arrayNode('defaultTableOptions')
Copy link
Member

Choose a reason for hiding this comment

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

you are missing the ->fixXmlConfig() on the parent node for the XML to work properly

@DHager
Copy link
Contributor Author

DHager commented Jun 6, 2015

@stof : The pluralization is now fixed, but what I meant is that I'm not sure if there's a worthwhile way to let the user specify default-table-option (XML tag) and default_table_option (YAML key) while still putting something called defaultTableOptions into Doctrine-DBAL.

@DHager
Copy link
Contributor Author

DHager commented Jun 9, 2015

@stof: I think this one is done. Personally, it removes the need for a lot of <options> in every entity stating the desired character set and collation.

@DHager
Copy link
Contributor Author

DHager commented Jul 16, 2015

Are there any changes or checks I can do to help move this one along?

@stof
Copy link
Member

stof commented Jul 20, 2015

the XML conventions are to use dash-separated words in keys, and the Yaml convention is to use snake_case. So yes, I would use such names in the config (we already have some logic in the DI extension converting to camelCase for Doctrine names; you just need to add it in the list)

@DHager
Copy link
Contributor Author

DHager commented Jul 22, 2015

OK, so you're saying that the meta-configuration layer does not handle all three varieties, only the dash and underscore variety, and any conversion to camelCase must be done with some ad-hoc code, correct? Perhaps in DoctrineExtension::dbalLoad()?

Are you sure all this convention-juggling is necessary? To some extent this is not structured configuration for the bundle to consume, but a complex payload we just want to pass-through so that DBAL can consume and interpret. (There are also several documented sibling-keys which break the pattern, such as sessionMode, sslmode, and MultipleActiveResultSets.)

…table_options (or dashed, if XML)

Add renaming step to the extension so that DBAL layer is still satisfied seeing defaultTableOptions
@DHager
Copy link
Contributor Author

DHager commented Jul 22, 2015

OK, I've added some code that--after loading configurations--renames/replaces them as camelCase. The rename has to occur in order to satisfy DBAL code in AbstractSchemaManager which looks specifically for that string.

@DHager
Copy link
Contributor Author

DHager commented Jul 30, 2015

@stof : It ought to match the XML/YAML conventions now. Anything else?

* satisfy DBAL, which is expecting camelCasing.
*
*/
$conversions = array('default_table_options' => 'defaultTableOptions');
Copy link
Member

Choose a reason for hiding this comment

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

we already have code doing this. See getConnectionOptions. Just add this option there

@DHager
Copy link
Contributor Author

DHager commented Aug 15, 2015

@stof : Fixed the conversion step to use the existing mechanism. Thanks, didn't see it there.

@DHager
Copy link
Contributor Author

DHager commented Sep 1, 2015

@stof Is there anything else?

@DHager
Copy link
Contributor Author

DHager commented Sep 8, 2015

Ping

@DHager
Copy link
Contributor Author

DHager commented Sep 16, 2015

@stof: Ping

@DHager
Copy link
Contributor Author

DHager commented Sep 25, 2015

@stof: Ping

@DHager
Copy link
Contributor Author

DHager commented Oct 2, 2015

@stof : Weekly checkin.

@DHager
Copy link
Contributor Author

DHager commented Oct 16, 2015

@stof Anything else I can do to make this merge-ready? Personally speaking, it should let me avoid putting

    <options>
        <option name="charset">utf8mb4</option>
        <option name="collation">utf8mb4_unicode_ci</option>
    </options> 

across hundreds of <Entity> definitions.

@teohhanhui
Copy link

Since @stof is not giving any response, perhaps it's time to ask the other maintainers...

@fabpot @beberlei

guilhermeblanco added a commit that referenced this pull request Nov 4, 2015
Allow defaultTableOptions to be configured
@guilhermeblanco guilhermeblanco merged commit f3782a6 into doctrine:master Nov 4, 2015
mcfedr added a commit to mcfedr/symfony-docs that referenced this pull request Feb 15, 2016
Since its now possible (doctrine/DoctrineBundle#420) to set the default character set for all tables using DoctrineBundle it can be mentioned in the docs
mcfedr added a commit to mcfedr/symfony-docs that referenced this pull request Feb 15, 2016
Since its now possible (doctrine/DoctrineBundle#420) to set the default character set for all tables using DoctrineBundle it can be mentioned in the docs
mcfedr added a commit to mcfedr/symfony-docs that referenced this pull request Feb 15, 2016
Since its now possible (doctrine/DoctrineBundle#420) to set the default character set for all tables using DoctrineBundle it can be mentioned in the docs
mcfedr added a commit to mcfedr/symfony-docs that referenced this pull request Mar 8, 2016
Since its now possible (doctrine/DoctrineBundle#420) to set the default character set for all tables using DoctrineBundle it can be mentioned in the docs
weaverryan added a commit to symfony/symfony-docs that referenced this pull request Mar 17, 2016
This PR was merged into the 3.0 branch.

Discussion
----------

Update Doctrine UTF8 docs

Since its now possible (doctrine/DoctrineBundle#420) to set the default character set for all tables using DoctrineBundle it can be mentioned in the docs

Commits
-------

dcd1126 Update Doctrine UTF8 docs
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.

4 participants