Schema cache fixes #14073
Schema cache fixes #14073
Conversation
brandonkelly
commented
Apr 27, 2017
brandonkelly
commented
| Q | A |
|---|---|
| Is bugfix? | yes |
| New feature? | no |
| Breaks BC? | no |
| Tests pass? | yes |
| Fixed issues | #14072 |
tests/framework/db/CommandTest.php
Outdated
|
|
||
| $db->createCommand()->addColumn($tableName, 'value', 'integer')->execute(); | ||
| $newSchema = $db->getSchema()->getTableSchema($tableName); | ||
| $this->assertNotEquals($initialSchema, $newSchema); | ||
|
|
||
| $db->createCommand()->addForeignKey($fkName, $tableName, 'fk', $tableName, 'id')->execute(); |
There was a problem hiding this comment.
https://travis-ci.org/yiisoft/yii2/jobs/226569561#L325
I think some tests is better to mark skipped for some particular DBMS (like https://github.com/yiisoft/yii2/blob/master/tests/framework/db/CommandTest.php#L561)
There was a problem hiding this comment.
Sorry if I mislead you by pointing to markTestSkipped() method without futher explanations.
With this method ALL test method will be skipped, and it's not so correct now 'cause addColumn(), for instance, works perfect with sqlite, but also will not be tested.
markTestSkipped() is applicable if we'll split this method to several ones.
There was a problem hiding this comment.
@MysteryDragon What do you recommend? Should I split testAutoRefreshTableSchema into multiple tests (which would require re-building the table multiple times), or just skip the FK command + assertion if running SQLite?
There was a problem hiding this comment.
@brandonkelly IMO the first option is more "theoretically correct", but I would prefer the second one due to simplicity and efficiency. I can imagine some other options, but they also have their disadvantages.
There was a problem hiding this comment.
@MysteryDragon Alright, done: 11c0d4b (skipped the column comment things too, since sqlite doesn’t support that either).
tests/framework/db/CommandTest.php
Outdated
|
|
||
| $db->createCommand()->addColumn($tableName, 'value', 'integer')->execute(); | ||
| $newSchema = $db->getSchema()->getTableSchema($tableName); | ||
| $this->assertNotEquals($initialSchema, $newSchema); | ||
|
|
||
| if ($this->driverName === 'sqlite') { |
There was a problem hiding this comment.
@brandonkelly I think you mean !== here.
There was a problem hiding this comment.
Doh! Sorry about that.
| @@ -537,7 +537,7 @@ public function createTable($table, $columns, $options = null) | |||
| { | |||
| $sql = $this->db->getQueryBuilder()->createTable($table, $columns, $options); | |||
|
|
|||
| return $this->setSql($sql); | |||
| return $this->setSql($sql)->requireTableSchemaRefresh($table); | |||
There was a problem hiding this comment.
If the table is just created, there should be no past cache so it makes no sense to refresh it.
There was a problem hiding this comment.
Schema caches also negative results, i.e. a non existing table will cache a null value:
Line 152 in 533da62
There was a problem hiding this comment.
Right, as demonstrated in #14072
There was a problem hiding this comment.
@samdark Also this PR comes with a couple new test assertions that will demonstrate the issue. If this change is removed, this will fail:
https://github.com/yiisoft/yii2/pull/14073/files#diff-e48002914d2773120698a96387ecabd2R822
|
Merged. Thank you! |
