Apply default collation on tables as per documentation (new #1165) #1169
Conversation
nebez
force-pushed
the
default-collation
branch
from
7576725
to
e8d3bd1
Compare
| @@ -300,6 +300,32 @@ public function testCreateTableWithMyISAMEngine() | |||
| $this->assertEquals('MyISAM', $row['Engine']); | |||
| } | |||
|
|
|||
| public function testCreateTableAndInheritDefaultCollation() | |||
| { | |||
| $options = array( | |||
There was a problem hiding this comment.
Short array syntax must be used to define arrays
| $adapter->createDatabase($options['name']); | ||
| $adapter->disconnect(); | ||
|
|
||
| $table = new \Phinx\Db\Table('table_with_default_collation', array(), $adapter); |
There was a problem hiding this comment.
Short array syntax must be used to define arrays
nebez
force-pushed
the
default-collation
branch
from
1f89bed
to
0ab243d
Compare
|
Any chance this gets merged? Just fixed the merge conflicts and new stickler-ci linter errors. |
Codecov Report
@@ Coverage Diff @@
## master #1169 +/- ##
==========================================
+ Coverage 72.19% 72.24% +0.05%
==========================================
Files 35 35
Lines 5549 5553 +4
==========================================
+ Hits 4006 4012 +6
+ Misses 1543 1541 -2
Continue to review full report at Codecov.
|
|
Any news on getting this merged? |
| $defaultOptions, | ||
| array_intersect_key($this->getOptions(), $defaultOptions), | ||
| $table->getOptions() | ||
| ); |
There was a problem hiding this comment.
are those assoc arrays? then we should merge with + Operator IMO
|
What if we remove the hardcoded default settings instead? I think we should not overwrite default settings on the database. |
|
@chinpei215 - I totally agree that Phinx setting its own default to override the DB's default is a bit odd. But would that be considered backwards incompatible? Phinx has always been doing it, I think if we changed it now that would break a lot of setups if people relied on that default collation. |
|
@nebez Fair points. OK, I will confirm your patch on my local environment, as I have a little concern about charset/collation mismatch between database settings and configurations (I may be wrong though). And if there are no problems, I will also approve your pull request. |
|
@chinpei215 Does a second PR make sense to remove the default entirely? |
|
@nebez Personally, I would like to remove the default entirely. I think it would confuse users. And if someone wants some default collation and/or engine, I think they should change |
|
OK. First I thought we should merge not only phinx/src/Phinx/Db/Adapter/MysqlAdapter.php Lines 242 to 246 in d84a6c7 So there is no need to merge the option at this point. If no one argues, I will merge this PR. |
|
Sounds good @chinpei215 - thanks for taking this on! |
|
Merged. Thank you for your contribution. |
Closed and re-opened from #1165 as I put new commits into my own master branch - use this one instead please
As per the documentation on environment configuration, there should be an option named
collation. This configuration option doesn't do anything when you set it.This change will set the default collation of newly created tables in this order:
collationoption provided in the array of optionscollationoption provided in the environment configurationutf8_general_ci