[WIP] Added support for alter table, foreign keys and autoincrement detection to Sqlite platform and schema #220

Merged
merged 9 commits into from Dec 23, 2012

Projects

None yet

9 participants

@hason

No description provided.

@doctrinebot

Hello,

thank you for positing this Pull Request. I have automatically opened an issue on our Jira Bug Tracker for you with the details of this Pull-Request. See the Link:

http://doctrine-project.org/jira/browse/DBAL-370

@stof stof and 1 other commented on an outdated diff Oct 20, 2012
lib/Doctrine/DBAL/Schema/SqliteSchemaManager.php
+ $autoincrementCount++;
+ if (null === $autoincrementColumn && 'integer' == strtolower($tableColumn['type'])) {
+ $autoincrementColumn = $tableColumn['name'];
+ }
+ }
+ }
+
+ if (1 == $autoincrementCount && null !== $autoincrementColumn) {
+ foreach ($list as $column) {
+ if ($autoincrementColumn == $column->getName()) {
+ $column->setAutoincrement(true);
+ }
+ }
+ }
+
+ return $list;
@stof
stof Oct 20, 2012

Wrong indentation (using tabs ?)

@stof stof commented on an outdated diff Oct 20, 2012
lib/Doctrine/DBAL/Schema/SchemaDiff.php
@@ -35,6 +35,11 @@
class SchemaDiff
{
/**
+ * @var Schema
+ */
+ public $schema;
@stof
stof Oct 20, 2012

Do you really need this public property ? I don't see it used anywhere

@stof stof commented on an outdated diff Oct 20, 2012
lib/Doctrine/DBAL/Schema/TableDiff.php
@@ -42,6 +42,11 @@ class TableDiff
public $newName = false;
/**
+ * @var Table
+ */
+ public $table;
@stof
stof Oct 20, 2012

should it really be a mutable public property ? It looks to me it should be immutable

@lsmith77
Doctrine member

would be great if you could also add support for deferred/deferrable as per lsmith77@bb54ca0#commitcomment-2158125

furthermore that patch also tries to make the support for foreign keys dynamic to handle the case of old sqlite versions and sqlite versions where FKs are disabled. not sure if that is necessary

@lsmith77 lsmith77 referenced this pull request in jackalope/jackalope-doctrine-dbal Nov 16, 2012
Merged

split the references into 2 tables to allow using defered FKs in the future #75

@lsmith77
Doctrine member

there are also some tests you might then also want to add to this PR once you add support for deferred/deferrable #228

@beberlei
Doctrine member

The pull request is not mergable anymore, can you rebase onto master?

@hason

@lsmith77 Support for deferred/deferrable foreign keys is done (added also sql parsing).
@beberlei Rebased

@stof stof commented on an outdated diff Nov 18, 2012
lib/Doctrine/DBAL/Platforms/SqlitePlatform.php
@@ -498,6 +520,60 @@ protected function getReservedKeywordsClass()
/**
* {@inheritDoc}
*/
+ protected function getPreAlterTableIndexForeignKeySQL(TableDiff $diff)
+ {
+ if (!$diff->fromTable instanceof Table) {
@stof
stof Nov 18, 2012

missing spaces

@stof stof commented on an outdated diff Nov 18, 2012
lib/Doctrine/DBAL/Platforms/SqlitePlatform.php
+ $sql = array();
+ foreach ($diff->fromTable->getIndexes() as $index) {
+ if ( ! $index->isPrimary()) {
+ $sql[] = $this->getDropIndexSQL($index, $diff->name);
+ }
+ }
+
+ return $sql;
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ protected function getPostAlterTableIndexForeignKeySQL(TableDiff $diff)
+ {
+ if (!$diff->fromTable instanceof Table) {
@stof
stof Nov 18, 2012

missing spaces

@stof stof and 1 other commented on an outdated diff Nov 18, 2012
lib/Doctrine/DBAL/Platforms/SqlitePlatform.php
+
+ return "PRAGMA foreign_key_list($table)";
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ public function getAlterTableSQL(TableDiff $diff)
+ {
+ $sql = $this->getSimpleAlterTableSQL($diff);
+ if (false !== $sql) {
+ return $sql;
+ }
+
+ $fromTable = $diff->fromTable;
+ if (!$fromTable instanceof Table) {
@stof
stof Nov 18, 2012

missing spaces

@hason
hason Nov 19, 2012

Fixed. Thanks!

@guilhermeblanco guilhermeblanco commented on the diff Nov 19, 2012
lib/Doctrine/DBAL/Platforms/SqlitePlatform.php
$query[] = 'CREATE TABLE ' . $name . ' (' . $queryFields . ')';
+ if (isset($options['alter']) && true === $options['alter']) {
+ return $query;
@guilhermeblanco
guilhermeblanco Nov 19, 2012

Is it correct? It seems if you return here you're not considering the indexes created for this table anymore.

@hason
hason Nov 19, 2012

Yes. The indexes are created in getPostAlterTableIndexForeignKeySQL method. We want to create indexes on renamed table (not on __temp__<name>).

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 19, 2012
lib/Doctrine/DBAL/Platforms/SqlitePlatform.php
- /**
- * {@inheritDoc}
- */
- public function supportsAlterTable()
- {
- return false;
+ if ($foreignKey->hasOption('deferrable') && $foreignKey->getOption('deferrable') !== false) {
@guilhermeblanco
guilhermeblanco Nov 19, 2012

I'd rewrite this piece of code:

$query .= (($foreignKey->hasOption('deferrable') && $foreignKey->getOption('deferrable') !== false) ? 'NOT ' : '') . 'DEFERRABLE';
$query .= 'INITIALLY ' . (($foreignKey->hasOption('deferred') && $foreignKey->getOption('deferred') !== false) ? 'DEFERRED' : 'IMMEDIATE');
@hason hason commented on the diff Nov 28, 2012
lib/Doctrine/DBAL/Schema/SqliteSchemaManager.php
+ 'onDelete' => $constraint['onDelete'],
+ 'onUpdate' => $constraint['onUpdate'],
+ 'deferrable' => $constraint['deferrable'],
+ 'deferred'=> $constraint['deferred'],
+ )
+ );
+ }
+
+ return $result;
+ }
+
+ private function getTableDiffForAlterForeignKey(ForeignKeyConstraint $foreignKey, $table)
+ {
+ if ( ! $table instanceof Table) {
+ $tableDetails = $this->tryMethod('listTableDetails', $table);
+ if (false === $table) {
@hason
hason Nov 28, 2012

Typo false === $tableDetails

@hason hason commented on the diff Nov 28, 2012
.../Doctrine/Tests/DBAL/Platforms/SqlitePlatformTest.php
+ $table->addColumn('id', 'integer');
+ $table->addColumn('article', 'integer');
+ $table->addColumn('post', 'integer');
+ $table->addColumn('parent', 'integer');
+ $table->setPrimaryKey(array('id'));
+ $table->addForeignKeyConstraint('article', array('article'), array('id'), array('deferrable' => true));
+ $table->addForeignKeyConstraint('post', array('post'), array('id'), array('deferred' => true));
+ $table->addForeignKeyConstraint('user', array('parent'), array('id'), array('deferrable' => true, 'deferred' => true));
+ $table->addIndex(array('article', 'post'), 'index1');
+
+ $diff = new TableDiff('user');
+ $diff->fromTable = $table;
+ $diff->newName = 'client';
+ $diff->renamedColumns['id'] = new \Doctrine\DBAL\Schema\Column('key', \Doctrine\DBAL\Types\Type::getType('integer'), array());
+ $diff->renamedColumns['post'] = new \Doctrine\DBAL\Schema\Column('comment', \Doctrine\DBAL\Types\Type::getType('integer'), array());
+ $diff->removedColumns['parent'] = new \Doctrine\DBAL\Schema\Column('comment', \Doctrine\DBAL\Types\Type::getType('integer'), array());
@hason
hason Nov 28, 2012

$diff->removedColumns['parent'] = $table->getColumn('parent');

@beberlei beberlei merged commit 63b7ef1 into doctrine:master Dec 23, 2012

1 check passed

Details default The Travis build passed
@Ocramius
Doctrine member

@hason is it just me or will the current implementation simply fail on anything that implies creation of a foreign key? This PR currently breaks all my functional test suites that generate a schema (ORM-side) in memory SQLite because of getCreateForeignKeySQL.

@Ocramius Ocramius commented on the diff Jan 2, 2013
lib/Doctrine/DBAL/Platforms/SqlitePlatform.php
{
- return false;
@Ocramius
Ocramius Jan 2, 2013

This should be reverted. After all, this PR doesn't really add support for it right now

@zorang

New doctrine 2.3.3 version is available, and I waited a lot for that because i hoped that when create SQLite schema that FK-s will be created also! But not! FK-s are still not generated! Can anyone tell me why?! I know that it was implemented last year (2012) in some branch... What happened?! Its removed or what?! Its not supported in new 2.3.3 doctrine?!?!

Zoran

@zorang

I hoped that in new doctrine 2.3.3 foreign keys will be implemented for SQLite, at least for tables creation. But I see that when I generate schema its not implemented! Can anyone tell me why?! We should wait next release for that functionality?!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment