From 6ffe4d3dda23483c8e57845cd3717ecba99ebdd2 Mon Sep 17 00:00:00 2001 From: Benjamin Eberlei Date: Sun, 15 Jan 2012 11:27:28 +0100 Subject: [PATCH] [DDC-1601] Fix bugs in SchemaValidator, using all modelsets as testdata for a large test --- lib/Doctrine/ORM/Tools/SchemaValidator.php | 75 ++++++++++--------- .../Tests/Models/Company/CompanyPerson.php | 2 +- .../Tests/Models/DDC117/DDC117Reference.php | 4 +- .../Tests/Models/DDC117/DDC117Translation.php | 4 +- .../Models/Legacy/LegacyUserReference.php | 2 +- .../Models/Navigation/NavPointOfInterest.php | 4 +- .../ORM/Functional/SchemaValidatorTest.php | 50 +++++++++++++ 7 files changed, 99 insertions(+), 42 deletions(-) create mode 100644 tests/Doctrine/Tests/ORM/Functional/SchemaValidatorTest.php diff --git a/lib/Doctrine/ORM/Tools/SchemaValidator.php b/lib/Doctrine/ORM/Tools/SchemaValidator.php index f6bcadb72..0e1f80cdf 100644 --- a/lib/Doctrine/ORM/Tools/SchemaValidator.php +++ b/lib/Doctrine/ORM/Tools/SchemaValidator.php @@ -95,8 +95,8 @@ class SchemaValidator } foreach ($class->associationMappings AS $fieldName => $assoc) { - if (!$cmf->hasMetadataFor($assoc['targetEntity'])) { - $ce[] = "The target entity '" . $assoc['targetEntity'] . "' specified on " . $class->name . '#' . $fieldName . ' is unknown.'; + if (!class_exists($assoc['targetEntity']) || $cmf->isTransient($assoc['targetEntity'])) { + $ce[] = "The target entity '" . $assoc['targetEntity'] . "' specified on " . $class->name . '#' . $fieldName . ' is unknown or not an entity.'; return $ce; } @@ -119,7 +119,7 @@ class SchemaValidator $ce[] = "The field " . $class->name . "#" . $fieldName . " is on the inverse side of a ". "bi-directional relationship, but the specified mappedBy association on the target-entity ". $assoc['targetEntity'] . "#" . $assoc['mappedBy'] . " does not contain the required ". - "'inversedBy' attribute."; + "'inversedBy=".$fieldName."' attribute."; } else if ($targetMetadata->associationMappings[$assoc['mappedBy']]['inversedBy'] != $fieldName) { $ce[] = "The mappings " . $class->name . "#" . $fieldName . " and " . $assoc['targetEntity'] . "#" . $assoc['mappedBy'] . " are ". @@ -162,30 +162,21 @@ class SchemaValidator if ($assoc['isOwningSide']) { if ($assoc['type'] == ClassMetadataInfo::MANY_TO_MANY) { + $identifierColumns = $class->getIdentifierColumnNames(); foreach ($assoc['joinTable']['joinColumns'] AS $joinColumn) { - if (!isset($class->fieldNames[$joinColumn['referencedColumnName']])) { - $ce[] = "The referenced column name '" . $joinColumn['referencedColumnName'] . "' does not " . - "have a corresponding field with this column name on the class '" . $class->name . "'."; - break; - } - - $fieldName = $class->fieldNames[$joinColumn['referencedColumnName']]; - if (!in_array($fieldName, $class->identifier)) { + if (!in_array($joinColumn['referencedColumnName'], $identifierColumns)) { $ce[] = "The referenced column name '" . $joinColumn['referencedColumnName'] . "' " . - "has to be a primary key column."; + "has to be a primary key column on the target entity class '".$class->name."'."; + break; } } - foreach ($assoc['joinTable']['inverseJoinColumns'] AS $inverseJoinColumn) { - if (!isset($targetMetadata->fieldNames[$inverseJoinColumn['referencedColumnName']])) { - $ce[] = "The inverse referenced column name '" . $inverseJoinColumn['referencedColumnName'] . "' does not " . - "have a corresponding field with this column name on the class '" . $targetMetadata->name . "'."; - break; - } - $fieldName = $targetMetadata->fieldNames[$inverseJoinColumn['referencedColumnName']]; - if (!in_array($fieldName, $targetMetadata->identifier)) { - $ce[] = "The referenced column name '" . $inverseJoinColumn['referencedColumnName'] . "' " . - "has to be a primary key column."; + $identifierColumns = $targetMetadata->getIdentifierColumnNames(); + foreach ($assoc['joinTable']['inverseJoinColumns'] AS $inverseJoinColumn) { + if (!in_array($inverseJoinColumn['referencedColumnName'], $identifierColumns)) { + $ce[] = "The referenced column name '" . $joinColumn['referencedColumnName'] . "' " . + "has to be a primary key column on the target entity class '".$targetMetadata->name."'."; + break; } } @@ -204,29 +195,23 @@ class SchemaValidator } } else if ($assoc['type'] & ClassMetadataInfo::TO_ONE) { + $identifierColumns = $targetMetadata->getIdentifierColumnNames(); foreach ($assoc['joinColumns'] AS $joinColumn) { - if (!isset($targetMetadata->fieldNames[$joinColumn['referencedColumnName']])) { - $ce[] = "The referenced column name '" . $joinColumn['referencedColumnName'] . "' does not " . - "have a corresponding field with this column name on the class '" . $targetMetadata->name . "'."; - break; - } - - $fieldName = $targetMetadata->fieldNames[$joinColumn['referencedColumnName']]; - if (!in_array($fieldName, $targetMetadata->identifier)) { + if (!in_array($joinColumn['referencedColumnName'], $identifierColumns)) { $ce[] = "The referenced column name '" . $joinColumn['referencedColumnName'] . "' " . - "has to be a primary key column."; + "has to be a primary key column on the target entity class '".$targetMetadata->name."'."; } } - if (count($class->getIdentifierColumnNames()) != count($assoc['joinColumns'])) { + if (count($identifierColumns) != count($assoc['joinColumns'])) { $ids = array(); foreach ($assoc['joinColumns'] AS $joinColumn) { $ids[] = $joinColumn['name']; } $ce[] = "The join columns of the association '" . $assoc['fieldName'] . "' " . - "have to match to ALL identifier columns of the source entity '". $class->name . "', " . - "however '" . implode(", ", array_diff($class->getIdentifierColumnNames(), $ids)) . + "have to match to ALL identifier columns of the target entity '". $class->name . "', " . + "however '" . implode(", ", array_diff($targetMetadata->getIdentifierColumnNames(), $ids)) . "' are missing."; } } @@ -260,6 +245,28 @@ class SchemaValidator return $ce; } + /** + * @param string $columnName + * @param ClassMetadataInfo $class + * @return bool + */ + private function columnExistsOnEntity($columnName, $class) + { + if (isset($class->fieldNames[$columnName])) { + return true; + } + foreach ($class->associationMappings as $assoc) { + if ($assoc['isOwningSide']) { + foreach ($assoc['joinColumns'] as $columnMapping) { + if ($columnMapping['name'] == $columnName) { + return true; + } + } + } + } + return false; + } + /** * Check if the Database Schema is in sync with the current metadata state. * diff --git a/tests/Doctrine/Tests/Models/Company/CompanyPerson.php b/tests/Doctrine/Tests/Models/Company/CompanyPerson.php index 712e81c83..0dfe9191c 100644 --- a/tests/Doctrine/Tests/Models/Company/CompanyPerson.php +++ b/tests/Doctrine/Tests/Models/Company/CompanyPerson.php @@ -28,7 +28,7 @@ class CompanyPerson */ private $name; /** - * @OneToOne(targetEntity="CompanyPerson", mappedBy="spouse") + * @OneToOne(targetEntity="CompanyPerson") * @JoinColumn(name="spouse_id", referencedColumnName="id") */ private $spouse; diff --git a/tests/Doctrine/Tests/Models/DDC117/DDC117Reference.php b/tests/Doctrine/Tests/Models/DDC117/DDC117Reference.php index 80cfb3a83..3c9017d19 100644 --- a/tests/Doctrine/Tests/Models/DDC117/DDC117Reference.php +++ b/tests/Doctrine/Tests/Models/DDC117/DDC117Reference.php @@ -16,7 +16,7 @@ class DDC117Reference /** * @Id - * @ManyToOne(targetEntity="DDC117Article", inversedBy="references") + * @ManyToOne(targetEntity="DDC117Article") * @JoinColumn(name="target_id", referencedColumnName="article_id") */ private $target; @@ -61,4 +61,4 @@ class DDC117Reference { return $this->description; } -} \ No newline at end of file +} diff --git a/tests/Doctrine/Tests/Models/DDC117/DDC117Translation.php b/tests/Doctrine/Tests/Models/DDC117/DDC117Translation.php index 1d38710c9..b0fb4375d 100644 --- a/tests/Doctrine/Tests/Models/DDC117/DDC117Translation.php +++ b/tests/Doctrine/Tests/Models/DDC117/DDC117Translation.php @@ -9,7 +9,7 @@ class DDC117Translation { /** * @Id - * @ManyToOne(targetEntity="DDC117Article") + * @ManyToOne(targetEntity="DDC117Article", inversedBy="translations") * @JoinColumn(name="article_id", referencedColumnName="article_id") */ private $article; @@ -62,4 +62,4 @@ class DDC117Translation { return $this->reviewedByEditors; } -} \ No newline at end of file +} diff --git a/tests/Doctrine/Tests/Models/Legacy/LegacyUserReference.php b/tests/Doctrine/Tests/Models/Legacy/LegacyUserReference.php index e666ae196..8dc20db23 100644 --- a/tests/Doctrine/Tests/Models/Legacy/LegacyUserReference.php +++ b/tests/Doctrine/Tests/Models/Legacy/LegacyUserReference.php @@ -17,7 +17,7 @@ class LegacyUserReference /** * @Id - * @ManyToOne(targetEntity="LegacyUser", inversedBy="_references") + * @ManyToOne(targetEntity="LegacyUser") * @JoinColumn(name="iUserIdTarget", referencedColumnName="iUserId") */ private $_target; diff --git a/tests/Doctrine/Tests/Models/Navigation/NavPointOfInterest.php b/tests/Doctrine/Tests/Models/Navigation/NavPointOfInterest.php index f212e68e2..662a57a09 100644 --- a/tests/Doctrine/Tests/Models/Navigation/NavPointOfInterest.php +++ b/tests/Doctrine/Tests/Models/Navigation/NavPointOfInterest.php @@ -26,7 +26,7 @@ class NavPointOfInterest private $name; /** - * @ManyToOne(targetEntity="NavCountry") + * @ManyToOne(targetEntity="NavCountry", inversedBy="pois") */ private $country; @@ -53,4 +53,4 @@ class NavPointOfInterest public function getCountry() { return $this->country; } -} \ No newline at end of file +} diff --git a/tests/Doctrine/Tests/ORM/Functional/SchemaValidatorTest.php b/tests/Doctrine/Tests/ORM/Functional/SchemaValidatorTest.php new file mode 100644 index 000000000..a575db0c0 --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/SchemaValidatorTest.php @@ -0,0 +1,50 @@ + $classes) { + if ($modelSet == "customtype") { + continue; + } + $modelSets[] = array($modelSet); + } + return $modelSets; + } + + /** + * @dataProvider dataValidateModelSets + */ + public function testValidateModelSets($modelSet) + { + $validator = new SchemaValidator($this->_em); + + $classes = array(); + foreach (self::$_modelSets[$modelSet] as $className) { + $classes[] = $this->_em->getClassMetadata($className); + } + + foreach ($classes as $class) { + $ce = $validator->validateClass($class); + + foreach ($ce as $key => $error) { + if (strpos($error, "must be private or protected. Public fields may break lazy-loading.") !== false) { + unset($ce[$key]); + } + } + + $this->assertEquals(0, count($ce), "Invalid Modelset: " . $modelSet . " class " . $class->name . ": ". implode("\n", $ce)); + } + } +}