From bf0ef0d0a767ec7040fd878a5b8cd077e019dbae Mon Sep 17 00:00:00 2001 From: beberlei Date: Sun, 6 Dec 2009 18:36:46 +0000 Subject: [PATCH] [2.0] DDC-169 - Fix several complications in update and drop schema code. --- bin/doctrine.php | 2 +- .../DBAL/Platforms/AbstractPlatform.php | 8 +++ lib/Doctrine/DBAL/Platforms/MySqlPlatform.php | 6 +++ .../DBAL/Platforms/PostgreSqlPlatform.php | 10 ++++ lib/Doctrine/DBAL/Schema/Comparator.php | 33 +++++++++--- .../DBAL/Schema/ForeignKeyConstraint.php | 21 ++++++++ lib/Doctrine/DBAL/Schema/SchemaDiff.php | 52 ++++++++++++++++--- lib/Doctrine/DBAL/Schema/SchemaException.php | 10 ++++ lib/Doctrine/DBAL/Schema/Table.php | 21 ++++++-- .../Schema/Visitor/DropSchemaSqlCollector.php | 17 +++--- lib/Doctrine/ORM/Tools/SchemaTool.php | 27 +++------- .../Doctrine/Tests/DBAL/Schema/TableTest.php | 12 ++--- .../Schema/Visitor/SchemaSqlCollectorTest.php | 6 +-- 13 files changed, 168 insertions(+), 57 deletions(-) diff --git a/bin/doctrine.php b/bin/doctrine.php index 03e7fc0a8..d93fd4f13 100644 --- a/bin/doctrine.php +++ b/bin/doctrine.php @@ -5,5 +5,5 @@ require 'Doctrine/Common/GlobalClassLoader.php'; $classLoader = new \Doctrine\Common\GlobalClassLoader(); $classLoader->register(); -$cli = new \Doctrine\ORM\Tools\Cli(); +$cli = new \Doctrine\ORM\Tools\Cli\CliController(); $cli->run($_SERVER['argv']); \ No newline at end of file diff --git a/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php b/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php index bc4809ba1..742832f00 100644 --- a/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php +++ b/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php @@ -1655,6 +1655,14 @@ abstract class AbstractPlatform return false; } + /** + * @return bool + */ + public function createsExplicitIndexForForeignKeys() + { + return false; + } + /** * Whether the platform supports getting the affected rows of a recent * update/delete type query. diff --git a/lib/Doctrine/DBAL/Platforms/MySqlPlatform.php b/lib/Doctrine/DBAL/Platforms/MySqlPlatform.php index 01caf2c8b..6726ea823 100644 --- a/lib/Doctrine/DBAL/Platforms/MySqlPlatform.php +++ b/lib/Doctrine/DBAL/Platforms/MySqlPlatform.php @@ -771,6 +771,12 @@ class MySqlPlatform extends AbstractPlatform */ public function getDropTableSql($table) { + if ($table instanceof \Doctrine\DBAL\Schema\Table) { + $table = $table->getName(); + } else if(!is_string($table)) { + throw new \InvalidArgumentException('MysqlPlatform::getDropTableSql() expects $table parameter to be string or \Doctrine\DBAL\Schema\Table.'); + } + return 'DROP TABLE ' . $table; } diff --git a/lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php b/lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php index 369f9166d..0c8528c5a 100644 --- a/lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php +++ b/lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php @@ -565,6 +565,16 @@ class PostgreSqlPlatform extends AbstractPlatform } return 'DROP SEQUENCE ' . $sequence; } + + /** + * @param ForeignKeyConstraint|string $foreignKey + * @param Table|string $table + * @return string + */ + public function getDropForeignKeySql($foreignKey, $table) + { + return $this->getDropConstraintSql($foreignKey, $table); + } /** * Gets the SQL used to create a table. diff --git a/lib/Doctrine/DBAL/Schema/Comparator.php b/lib/Doctrine/DBAL/Schema/Comparator.php index ec37c9635..06e4c42b1 100644 --- a/lib/Doctrine/DBAL/Schema/Comparator.php +++ b/lib/Doctrine/DBAL/Schema/Comparator.php @@ -70,11 +70,13 @@ class Comparator * * @return SchemaDiff */ - public function compare( Schema $fromSchema, Schema $toSchema ) + public function compare(Schema $fromSchema, Schema $toSchema) { $diff = new SchemaDiff(); - foreach ( $toSchema->getTables() as $tableName => $table ) { + $foreignKeysToTable = array(); + + foreach ( $toSchema->getTables() AS $tableName => $table ) { if ( !$fromSchema->hasTable($tableName) ) { $diff->newTables[$tableName] = $table; } else { @@ -86,10 +88,25 @@ class Comparator } /* Check if there are tables removed */ - foreach ( $fromSchema->getTables() as $tableName => $table ) { + foreach ( $fromSchema->getTables() AS $tableName => $table ) { if ( !$toSchema->hasTable($tableName) ) { $diff->removedTables[$tableName] = $table; } + + // also remember all foreign keys that point to a specific table + foreach ($table->getForeignKeys() AS $foreignKey) { + $foreignTable = strtolower($foreignKey->getForeignTableName()); + if (!isset($foreignKeysToTable[$foreignTable])) { + $foreignKeysToTable[$foreignTable] = array(); + } + $foreignKeysToTable[$foreignTable][] = $foreignKey; + } + } + + foreach ($diff->removedTables AS $tableName => $table) { + if (isset($foreignKeysToTable[$tableName])) { + $diff->orphanedForeignKeys = array_merge($diff->orphanedForeignKeys, $foreignKeysToTable[$tableName]); + } } foreach ( $toSchema->getSequences() AS $sequenceName => $sequence) { @@ -116,7 +133,7 @@ class Comparator * @param Sequence $sequence1 * @param Sequence $sequence2 */ - public function diffSequence($sequence1, $sequence2) + public function diffSequence(Sequence $sequence1, Sequence $sequence2) { if($sequence1->getAllocationSize() != $sequence2->getAllocationSize()) { return true; @@ -139,7 +156,7 @@ class Comparator * * @return bool|TableDiff */ - public function diffTable( Table $table1, Table $table2 ) + public function diffTable(Table $table1, Table $table2) { $changes = 0; $tableDifferences = new TableDiff($table1->getName()); @@ -249,7 +266,7 @@ class Comparator * @param ForeignKeyConstraint $key2 * @return bool */ - public function diffForeignKey($key1, $key2) + public function diffForeignKey(ForeignKeyConstraint $key1, ForeignKeyConstraint $key2) { if ($key1->getLocalColumns() != $key2->getLocalColumns()) { return true; @@ -289,7 +306,7 @@ class Comparator * * @return array */ - public function diffColumn( Column $column1, Column $column2 ) + public function diffColumn(Column $column1, Column $column2) { $changedProperties = array(); if ( $column1->getType() != $column2->getType() ) { @@ -350,7 +367,7 @@ class Comparator * @param Index $index2 * @return bool */ - public function diffIndex( Index $index1, Index $index2 ) + public function diffIndex(Index $index1, Index $index2) { if($index1->isPrimary() != $index2->isPrimary()) { return true; diff --git a/lib/Doctrine/DBAL/Schema/ForeignKeyConstraint.php b/lib/Doctrine/DBAL/Schema/ForeignKeyConstraint.php index 74c1e4d8c..5ee351a55 100644 --- a/lib/Doctrine/DBAL/Schema/ForeignKeyConstraint.php +++ b/lib/Doctrine/DBAL/Schema/ForeignKeyConstraint.php @@ -25,6 +25,11 @@ use Doctrine\DBAL\Schema\Visitor\Visitor; class ForeignKeyConstraint extends AbstractAsset implements Constraint { + /** + * @var Table + */ + protected $_localTable; + /** * @var array */ @@ -67,6 +72,22 @@ class ForeignKeyConstraint extends AbstractAsset implements Constraint $this->_options = $options; } + /** + * @return string + */ + public function getLocalTableName() + { + return $this->_localTable->getName(); + } + + /** + * @param Table $table + */ + public function setLocalTable(Table $table) + { + $this->_localTable = $table; + } + /** * @return array */ diff --git a/lib/Doctrine/DBAL/Schema/SchemaDiff.php b/lib/Doctrine/DBAL/Schema/SchemaDiff.php index 152b2a88d..a3d737fda 100644 --- a/lib/Doctrine/DBAL/Schema/SchemaDiff.php +++ b/lib/Doctrine/DBAL/Schema/SchemaDiff.php @@ -72,6 +72,11 @@ class SchemaDiff */ public $removedSequences = array(); + /** + * @var array + */ + public $orphanedForeignKeys = array(); + /** * Constructs an SchemaDiff object. * @@ -87,23 +92,56 @@ class SchemaDiff } /** - * @param Schema $fromSchema - * @param Schema $toSchema + * The to save sql mode ensures that the following things don't happen: + * + * 1. Tables are deleted + * 2. Sequences are deleted + * 3. Foreign Keys which reference tables that would otherwise be deleted. + * + * This way it is ensured that assets are deleted which might not be relevant to the metadata schema at all. + * + * @param AbstractPlatform $platform + * @return array + */ + public function toSaveSql(AbstractPlatform $platform) + { + return $this->_toSql($platform, true); + } + + /** * @param AbstractPlatform $platform * @return array */ public function toSql(AbstractPlatform $platform) + { + return $this->_toSql($platform, false); + } + + /** + * @param AbstractPlatform $platform + * @param bool $saveMode + * @return array + */ + protected function _toSql(AbstractPlatform $platform, $saveMode = false) { $sql = array(); + if ($platform->supportsForeignKeyConstraints()) { + foreach ($this->orphanedForeignKeys AS $orphanedForeignKey) { + $sql[] = $platform->getDropForeignKeySql($orphanedForeignKey, $orphanedForeignKey->getLocalTableName()); + } + } + if ($platform->supportsSequences() == true) { foreach ($this->changedSequences AS $sequence) { $sql[] = $platform->getDropSequenceSql($sequence); $sql[] = $platform->getCreateSequenceSql($sequence); } - foreach ($this->removedSequences AS $sequence) { - $sql[] = $platform->getDropSequenceSql($sequence); + if ($saveMode === false) { + foreach ($this->removedSequences AS $sequence) { + $sql[] = $platform->getDropSequenceSql($sequence); + } } foreach ($this->newSequences AS $sequence) { @@ -118,8 +156,10 @@ class SchemaDiff ); } - foreach ($this->removedTables AS $table) { - $sql[] = $platform->getDropTableSql($table); + if ($saveMode === true) { + foreach ($this->removedTables AS $table) { + $sql[] = $platform->getDropTableSql($table); + } } foreach ($this->changedTables AS $tableDiff) { diff --git a/lib/Doctrine/DBAL/Schema/SchemaException.php b/lib/Doctrine/DBAL/Schema/SchemaException.php index 3a76397f6..b72833e64 100644 --- a/lib/Doctrine/DBAL/Schema/SchemaException.php +++ b/lib/Doctrine/DBAL/Schema/SchemaException.php @@ -114,4 +114,14 @@ class SchemaException extends \Doctrine\DBAL\DBALException { return new self("Invalid case mode given to Schema Asset."); } + + static public function namedForeignKeyRequired($localTable, $foreignKey) + { + return new self( + "The performed schema operation on ".$localTable->getName()." requires a named foreign key, ". + "but the given foreign key from (".implode(", ", $foreignKey->getColumns()).") onto foreign table ". + "'".$foreignKey->getForeignTableName()."' (".implode(", ", $foreignKey->getForeignColumns()).") is currently ". + "unnamed." + ); + } } \ No newline at end of file diff --git a/lib/Doctrine/DBAL/Schema/Table.php b/lib/Doctrine/DBAL/Schema/Table.php index a898c8c98..63314e883 100644 --- a/lib/Doctrine/DBAL/Schema/Table.php +++ b/lib/Doctrine/DBAL/Schema/Table.php @@ -144,7 +144,9 @@ class Table extends AbstractAsset public function addIndex(array $columnNames, $indexName=null) { if($indexName == null) { - $indexName = $this->_generateIdentifierName($columnNames, "idx"); + $indexName = $this->_generateIdentifierName( + array_merge(array($this->getName()), $columnNames), "idx" + ); } return $this->_createIndex($columnNames, $indexName, false, false); @@ -159,7 +161,9 @@ class Table extends AbstractAsset public function addUniqueIndex(array $columnNames, $indexName=null) { if ($indexName == null) { - $indexName = $this->_generateIdentifierName($columnNames, "uniq"); + $indexName = $this->_generateIdentifierName( + array_merge(array($this->getName()), $columnNames), "uniq" + ); } return $this->_createIndex($columnNames, $indexName, true, false); @@ -313,11 +317,12 @@ class Table extends AbstractAsset throw SchemaException::columnDoesNotExist($columnName); } } - + $constraint = new ForeignKeyConstraint( $localColumnNames, $foreignTableName, $foreignColumnNames, $name, $options ); $this->_addForeignKeyConstraint($constraint); + return $this; } @@ -355,6 +360,14 @@ class Table extends AbstractAsset */ protected function _addIndex(Index $index) { + // check for duplicates + $c = new Comparator(); + foreach ($this->_indexes AS $existingIndex) { + if ($c->diffIndex($index, $existingIndex) == false) { + return $this; + } + } + $indexName = $index->getName(); $indexName = strtolower($indexName); @@ -375,6 +388,8 @@ class Table extends AbstractAsset */ protected function _addForeignKeyConstraint(ForeignKeyConstraint $constraint) { + $constraint->setLocalTable($this); + if(strlen($constraint->getName())) { $name = $constraint->getName(); } else { diff --git a/lib/Doctrine/DBAL/Schema/Visitor/DropSchemaSqlCollector.php b/lib/Doctrine/DBAL/Schema/Visitor/DropSchemaSqlCollector.php index c455c419c..c2809cd12 100644 --- a/lib/Doctrine/DBAL/Schema/Visitor/DropSchemaSqlCollector.php +++ b/lib/Doctrine/DBAL/Schema/Visitor/DropSchemaSqlCollector.php @@ -83,7 +83,7 @@ class DropSchemaSqlCollector implements Visitor */ public function acceptTable(Table $table) { - $this->_tables = array_merge($this->_tables, $this->_platform->getDropTableSql($table->getName())); + $this->_tables[] = $this->_platform->getDropTableSql($table->getName()); } /** @@ -100,11 +100,11 @@ class DropSchemaSqlCollector implements Visitor */ public function acceptForeignKey(Table $localTable, ForeignKeyConstraint $fkConstraint) { - $this->_constraints = array_merge($this->_constraints, - $this->_platform->getDropForeignKeySql( - $localTable->getName(), $fkConstraint->getName() - ) - ); + if (strlen($fkConstraint->getName()) == 0) { + throw SchemaException::namedForeignKeyRequired($localTable, $fkConstraint); + } + + $this->_constraints[] = $this->_platform->getDropForeignKeySql($fkConstraint->getName(), $localTable->getName()); } /** @@ -121,10 +121,7 @@ class DropSchemaSqlCollector implements Visitor */ public function acceptSequence(Sequence $sequence) { - $this->_sequences = array_merge( - $this->_sequences, - $this->_platform->getDropSequenceSql($sequence->getName()) - ); + $this->_sequences[] = $this->_platform->getDropSequenceSql($sequence->getName()); } /** diff --git a/lib/Doctrine/ORM/Tools/SchemaTool.php b/lib/Doctrine/ORM/Tools/SchemaTool.php index 8451e48ce..c82ed633f 100644 --- a/lib/Doctrine/ORM/Tools/SchemaTool.php +++ b/lib/Doctrine/ORM/Tools/SchemaTool.php @@ -446,28 +446,15 @@ class SchemaTool * @param string $mode * @return array */ - public function getDropSchemaSql(array $classes, $mode=self::DROP_METADATA) + public function getDropSchemaSql(array $classes) { - if($mode == self::DROP_METADATA) { - $tables = $this->_getDropSchemaTablesMetadataMode($classes); - } else if($mode == self::DROP_DATABASE) { - $tables = $this->_getDropSchemaTablesDatabaseMode($classes); - } else { - throw new \Doctrine\ORM\ORMException("Given Drop Schema Mode is not supported."); - } - $sm = $this->_em->getConnection()->getSchemaManager(); - /* @var $sm \Doctrine\DBAL\Schema\AbstractSchemaManager */ - $allTables = $sm->listTables(); - - $sql = array(); - foreach($tables AS $tableName) { - if(in_array($tableName, $allTables)) { - $sql[] = $this->_platform->getDropTableSql($tableName); - } - } + $schema = $sm->createSchema(); - return $sql; + $visitor = new \Doctrine\DBAL\Schema\Visitor\DropSchemaSqlCollector($this->_platform); + /* @var $schema \Doctrine\DBAL\Schema\Schema */ + $schema->visit($visitor); + return $visitor->getQueries(); } /** @@ -534,7 +521,7 @@ class SchemaTool { $updateSchemaSql = $this->getUpdateSchemaSql($classes); $conn = $this->_em->getConnection(); - + foreach ($updateSchemaSql as $sql) { $conn->execute($sql); } diff --git a/tests/Doctrine/Tests/DBAL/Schema/TableTest.php b/tests/Doctrine/Tests/DBAL/Schema/TableTest.php index 3e99eff6f..fa415a153 100644 --- a/tests/Doctrine/Tests/DBAL/Schema/TableTest.php +++ b/tests/Doctrine/Tests/DBAL/Schema/TableTest.php @@ -109,8 +109,8 @@ class TableTest extends \PHPUnit_Framework_TestCase $table->addIndex(array("foo", "bar", "baz")); $table->addUniqueIndex(array("foo", "bar", "baz")); - $this->assertTrue($table->hasIndex("foo_bar_baz_idx")); - $this->assertTrue($table->hasIndex("foo_bar_baz_uniq")); + $this->assertTrue($table->hasIndex("foo_foo_bar_baz_idx")); + $this->assertTrue($table->hasIndex("foo_foo_bar_baz_uniq")); } public function testIndexCaseInsensitive() @@ -158,10 +158,10 @@ class TableTest extends \PHPUnit_Framework_TestCase $this->setExpectedException("Doctrine\DBAL\Schema\SchemaException"); $type = \Doctrine\DBAL\Types\Type::getType('integer'); - $columns = array(new Column("foo", $type)); + $columns = array(new Column("foo", $type), new Column("bar", $type)); $indexes = array( new Index("the_primary", array("foo"), true, true), - new Index("other_primary", array("foo"), true, true), + new Index("other_primary", array("bar"), true, true), ); $table = new Table("foo", $columns, $indexes, array()); } @@ -171,10 +171,10 @@ class TableTest extends \PHPUnit_Framework_TestCase $this->setExpectedException("Doctrine\DBAL\Schema\SchemaException"); $type = \Doctrine\DBAL\Types\Type::getType('integer'); - $columns = array(new Column("foo", $type)); + $columns = array(new Column("foo", $type), new Column("bar", $type)); $indexes = array( new Index("an_idx", array("foo"), false, false), - new Index("an_idx", array("foo"), false, false), + new Index("an_idx", array("bar"), false, false), ); $table = new Table("foo", $columns, $indexes, array()); } diff --git a/tests/Doctrine/Tests/DBAL/Schema/Visitor/SchemaSqlCollectorTest.php b/tests/Doctrine/Tests/DBAL/Schema/Visitor/SchemaSqlCollectorTest.php index ac828fbb7..99f12aacd 100644 --- a/tests/Doctrine/Tests/DBAL/Schema/Visitor/SchemaSqlCollectorTest.php +++ b/tests/Doctrine/Tests/DBAL/Schema/Visitor/SchemaSqlCollectorTest.php @@ -41,13 +41,13 @@ class SchemaSqlCollectorTest extends \PHPUnit_Framework_TestCase ); $platformMock->expects($this->exactly(2)) ->method('getDropTableSql') - ->will($this->returnValue(array("tbl"))); + ->will($this->returnValue("tbl")); $platformMock->expects($this->exactly(1)) ->method('getDropSequenceSql') - ->will($this->returnValue(array("seq"))); + ->will($this->returnValue("seq")); $platformMock->expects($this->exactly(1)) ->method('getDropForeignKeySql') - ->will($this->returnValue(array("fk"))); + ->will($this->returnValue("fk")); $schema = $this->createFixtureSchema();