From a12982c605b24169b520c05b5d96d1ee2a3c0525 Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Mon, 4 Dec 2023 17:47:14 +1300 Subject: [PATCH] NEW Allow a single has_one to manage multiple reciprocal has_many --- _config/model.yml | 2 + src/Dev/FixtureBlueprint.php | 2 +- .../Validation/RelationValidationService.php | 29 ++++- .../GridFieldDetailForm_ItemRequest.php | 8 ++ src/ORM/DataObject.php | 34 ++++-- src/ORM/DataObjectSchema.php | 103 ++++++++++++++++-- src/ORM/DataQuery.php | 11 +- .../DBPolymorphicRelationAwareForeignKey.php | 48 ++++++++ src/ORM/PolymorphicHasManyList.php | 57 ++++++++++ tests/php/Dev/Validation/Member.php | 2 + .../Dev/Validation/RelationValidationTest.php | 49 +++++++++ tests/php/Dev/Validation/Team.php | 8 ++ 12 files changed, 330 insertions(+), 23 deletions(-) create mode 100644 src/ORM/FieldType/DBPolymorphicRelationAwareForeignKey.php diff --git a/_config/model.yml b/_config/model.yml index 9520d393389..cfb60c02aa6 100644 --- a/_config/model.yml +++ b/_config/model.yml @@ -48,6 +48,8 @@ SilverStripe\Core\Injector\Injector: class: SilverStripe\ORM\FieldType\DBPercentage PolymorphicForeignKey: class: SilverStripe\ORM\FieldType\DBPolymorphicForeignKey + PolymorphicRelationAwareForeignKey: + class: SilverStripe\ORM\FieldType\DBPolymorphicRelationAwareForeignKey PrimaryKey: class: SilverStripe\ORM\FieldType\DBPrimaryKey Text: diff --git a/src/Dev/FixtureBlueprint.php b/src/Dev/FixtureBlueprint.php index 7652f6ddb2d..b1901489941 100644 --- a/src/Dev/FixtureBlueprint.php +++ b/src/Dev/FixtureBlueprint.php @@ -236,7 +236,7 @@ public function createObject($identifier, $data = null, $fixtures = null) if ($className = $schema->hasOneComponent($class, $hasOneField)) { $obj->{$hasOneField . 'ID'} = $this->parseValue($fieldVal, $fixtures, $fieldClass); // Inject class for polymorphic relation - if ($className === 'SilverStripe\\ORM\\DataObject') { + if ($className === DataObject::class) { $obj->{$hasOneField . 'Class'} = $fieldClass; } } diff --git a/src/Dev/Validation/RelationValidationService.php b/src/Dev/Validation/RelationValidationService.php index 5fc4a75e357..b51096e2240 100644 --- a/src/Dev/Validation/RelationValidationService.php +++ b/src/Dev/Validation/RelationValidationService.php @@ -2,12 +2,14 @@ namespace SilverStripe\Dev\Validation; +use InvalidArgumentException; use ReflectionException; use SilverStripe\Core\ClassInfo; use SilverStripe\Core\Config\Configurable; use SilverStripe\Core\Injector\Injectable; use SilverStripe\Core\Resettable; use SilverStripe\ORM\DataObject; +use SilverStripe\ORM\DataObjectSchema; use SilverStripe\ORM\DB; /** @@ -291,6 +293,26 @@ protected function validateHasOne(string $class): void $relations = (array) $singleton->config()->uninherited('has_one'); foreach ($relations as $relationName => $relationData) { + if (is_array($relationData)) { + $spec = $relationData; + if (!isset($spec['class'])) { + $this->logError($class, $relationName, 'No class has been defined for this relation.'); + continue; + } + $relationData = $spec['class']; + if (isset($spec[DataObjectSchema::HASONE_MULTIPLE_HASMANY]) + && $spec[DataObjectSchema::HASONE_MULTIPLE_HASMANY] === true + && $relationData !== DataObject::class + ) { + $this->logError( + $class, + $relationName, + 'has_one relation that can handle multiple reciprocal has_many relations must be polymorphic.' + ); + continue; + } + } + if ($this->isIgnored($class, $relationName)) { continue; } @@ -305,6 +327,11 @@ protected function validateHasOne(string $class): void return; } + // Skip checking for back relations when has_one is polymorphic + if ($relationData === DataObject::class) { + continue; + } + if (!is_subclass_of($relationData, DataObject::class)) { $this->logError( $class, @@ -616,7 +643,7 @@ protected function parseManyManyRelation($relationData): ?string return null; } - return $throughRelations[$to]; + return DataObject::getSchema()->hasOneComponent($through, $to); } return $relationData; diff --git a/src/Forms/GridField/GridFieldDetailForm_ItemRequest.php b/src/Forms/GridField/GridFieldDetailForm_ItemRequest.php index e3b5b8c3b97..8804c69eea5 100644 --- a/src/Forms/GridField/GridFieldDetailForm_ItemRequest.php +++ b/src/Forms/GridField/GridFieldDetailForm_ItemRequest.php @@ -204,6 +204,14 @@ public function ItemEditForm() $classKey = $list->getForeignClassKey(); $class = $list->getForeignClass(); $this->record->$classKey = $class; + + // If the has_one relation storing the data can handle multiple reciprocal has_many relations, + // make sure we tell it which has_many relation this belongs to. + $relation = $list->getForeignRelation(); + if ($relation) { + $relationKey = $list->getForeignRelationKey(); + $this->record->$relationKey = $relation; + } } } diff --git a/src/ORM/DataObject.php b/src/ORM/DataObject.php index 3e978eb05f0..18033b8fed1 100644 --- a/src/ORM/DataObject.php +++ b/src/ORM/DataObject.php @@ -1972,12 +1972,14 @@ public function getComponents($componentName, $id = null) } // Determine type and nature of foreign relation - $joinField = $schema->getRemoteJoinField(static::class, $componentName, 'has_many', $polymorphic); - /** @var HasManyList $result */ - if ($polymorphic) { - $result = PolymorphicHasManyList::create($componentClass, $joinField, static::class); + $details = $schema->getHasManyComponentDetails(static::class, $componentName); + if ($details['polymorphic']) { + $result = PolymorphicHasManyList::create($componentClass, $details['joinField'], static::class); + if ($details['needsRelation']) { + $result->setForeignRelation($componentName); + } } else { - $result = HasManyList::create($componentClass, $joinField); + $result = HasManyList::create($componentClass, $details['joinField']); } return $result @@ -1993,16 +1995,21 @@ public function getComponents($componentName, $id = null) */ public function getRelationClass($relationName) { - // Parse many_many - $manyManyComponent = $this->getSchema()->manyManyComponent(static::class, $relationName); + // Parse many_many, which can have an array instead of a class name + $manyManyComponent = static::getSchema()->manyManyComponent(static::class, $relationName); if ($manyManyComponent) { return $manyManyComponent['childClass']; } - // Go through all relationship configuration fields. + // Parse has_one, which can have an array instead of a class name + $hasOneComponent = static::getSchema()->hasOneComponent(static::class, $relationName); + if ($hasOneComponent) { + return $hasOneComponent; + } + + // Go through all remaining relationship configuration fields. $config = $this->config(); $candidates = array_merge( - ($relations = $config->get('has_one')) ? $relations : [], ($relations = $config->get('has_many')) ? $relations : [], ($relations = $config->get('belongs_to')) ? $relations : [] ); @@ -2236,7 +2243,14 @@ public function getManyManyComponents($componentName, $id = null) */ public function hasOne() { - return (array)$this->config()->get('has_one'); + $hasOne = (array) $this->config()->get('has_one'); + // Boil down has_one spec to just the class name + foreach ($hasOne as $relationName => $spec) { + if (is_array($spec)) { + $hasOne[$relationName] = DataObject::getSchema()->hasOneComponent($this->objectClass, $relationName); + } + } + return $hasOne; } /** diff --git a/src/ORM/DataObjectSchema.php b/src/ORM/DataObjectSchema.php index 09e15149aac..5fc48fb2a56 100644 --- a/src/ORM/DataObjectSchema.php +++ b/src/ORM/DataObjectSchema.php @@ -24,6 +24,11 @@ class DataObjectSchema use Injectable; use Configurable; + /** + * Configuration key for has_one relations that can support multiple reciprocal has_many relations. + */ + public const HASONE_MULTIPLE_HASMANY = 'handles_multiple_has_many'; + /** * Default separate for table namespaces. Can be set to any string for * databases that do not support some characters. @@ -501,7 +506,20 @@ protected function cacheDatabaseFields($class) // Add in all has_ones $hasOne = Config::inst()->get($class, 'has_one', Config::UNINHERITED) ?: []; - foreach ($hasOne as $fieldName => $hasOneClass) { + foreach ($hasOne as $fieldName => $spec) { + if (is_array($spec)) { + if (!isset($spec['class'])) { + throw new LogicException("has_one relation {$class}.{$fieldName} must declare a class"); + } + // Handle has_one which handles multiple reciprocal has_many relations + $hasOneClass = $spec['class']; + if (isset($spec[self::HASONE_MULTIPLE_HASMANY]) && $spec[self::HASONE_MULTIPLE_HASMANY] === true) { + $compositeFields[$fieldName] = 'PolymorphicRelationAwareForeignKey'; + continue; + } + } else { + $hasOneClass = $spec; + } if ($hasOneClass === DataObject::class) { $compositeFields[$fieldName] = 'PolymorphicForeignKey'; } else { @@ -902,12 +920,36 @@ public function hasOneComponent($class, $component) return null; } + $spec = $hasOnes[$component]; + // Validate - $relationClass = $hasOnes[$component]; + if (is_array($spec)) { + $this->checkHasOneArraySpec($class, $component, $spec); + } + $relationClass = is_array($spec) ? $spec['class'] : $spec; $this->checkRelationClass($class, $component, $relationClass, 'has_one'); + return $relationClass; } + /** + * Check if a has_one relation handles multiple reciprocal has_many relations. + * + * @return bool True if the relation exists and handles multiple reciprocal has_many relations. + */ + public function hasOneComponentHandlesMultipleHasMany(string $class, string $component): bool + { + $hasOnes = Config::forClass($class)->get('has_one'); + if (!isset($hasOnes[$component])) { + return false; + } + + $spec = $hasOnes[$component]; + return is_array($spec) + && isset($spec[self::HASONE_MULTIPLE_HASMANY]) + && $spec[self::HASONE_MULTIPLE_HASMANY] === true; + } + /** * Return data for a specific belongs_to component. * @@ -1046,6 +1088,16 @@ protected function getManyManyInverseRelationship($childClass, $parentClass) * @throws Exception */ public function getRemoteJoinField($class, $component, $type = 'has_many', &$polymorphic = false) + { + return $this->getBelongsAndHasManyDetails($class, $component, $type, $polymorphic)['joinField']; + } + + public function getHasManyComponentDetails(string $class, string $component) + { + return $this->getBelongsAndHasManyDetails($class, $component); + } + + private function getBelongsAndHasManyDetails(string $class, string $component, string $type = 'has_many', &$polymorphic = false) { // Extract relation from current object if ($type === 'has_many') { @@ -1071,6 +1123,11 @@ public function getRemoteJoinField($class, $component, $type = 'has_many', &$pol // Reference remote has_one to check against $remoteRelations = Config::inst()->get($remoteClass, 'has_one'); + foreach ($remoteRelations as $key => $value) { + if (is_array($value)) { + $remoteRelations[$key] = $this->hasOneComponent($remoteClass, $key); + } + } // Without an explicit field name, attempt to match the first remote field // with the same type as the current class @@ -1104,14 +1161,23 @@ public function getRemoteJoinField($class, $component, $type = 'has_many', &$pol on class {$class}"); } - // Inspect resulting found relation - if ($remoteRelations[$remoteField] === DataObject::class) { - $polymorphic = true; - return $remoteField; // Composite polymorphic field does not include 'ID' suffix - } else { - $polymorphic = false; - return $remoteField . 'ID'; + $polymorphic = $this->hasOneComponent($remoteClass, $remoteField) === DataObject::class; + $remoteClassField = $polymorphic ? $remoteField . 'Class' : null; + $needsRelation = $type === 'has_many' && $polymorphic && $this->hasOneComponentHandlesMultipleHasMany($remoteClass, $remoteField); + $remoteRelationField = $needsRelation ? $remoteField . 'Relation' : null; + + // This must be after the above assignments, as they rely on the original value. + if (!$polymorphic) { + $remoteField .= 'ID'; } + + return [ + 'joinField' => $remoteField, + 'relationField' => $remoteRelationField, + 'classField' => $remoteClassField, + 'polymorphic' => $polymorphic, + 'needsRelation' => $needsRelation, + ]; } /** @@ -1204,6 +1270,25 @@ protected function checkManyManyJoinClass($parentClass, $component, $specificati return $joinClass; } + protected function checkHasOneArraySpec(string $class, string $component, array $spec): void + { + if (!array_key_exists('class', $spec)) { + throw new InvalidArgumentException( + "has_one relation {$class}.{$component} doesn't define a class for the relation" + ); + } + + if (isset($spec[self::HASONE_MULTIPLE_HASMANY]) + && $spec[self::HASONE_MULTIPLE_HASMANY] === true + && $spec['class'] !== DataObject::class + ) { + throw new InvalidArgumentException( + "has_one relation {$class}.{$component} must be polymorphic, or not support multiple" + . 'reciprocal has_many relations' + ); + } + } + /** * Validate a given class is valid for a relation * diff --git a/src/ORM/DataQuery.php b/src/ORM/DataQuery.php index 327a0c4ea19..05692c3f720 100644 --- a/src/ORM/DataQuery.php +++ b/src/ORM/DataQuery.php @@ -1025,8 +1025,6 @@ public function applyRelation($relation, $linearOnly = false) * Join the given has_many relation to this query. * Also works with belongs_to * - * Doesn't work with polymorphic relationships - * * @param string $localClass Name of class that has the has_many to the joined class * @param string $localField Name of the has_many relationship to join * @param string $foreignClass Class to join @@ -1065,6 +1063,15 @@ protected function joinHasManyRelation( $localClassColumn = $schema->sqlColumnForField($localClass, 'ClassName', $localPrefix); $joinExpression = "{$foreignKeyIDColumn} = {$localIDColumn} AND {$foreignKeyClassColumn} = {$localClassColumn}"; + + // Add relation key if the has_many points to a has_one that could handle multiple reciprocal has_many relations + if ($type === 'has_many') { + $details = $schema->getHasManyComponentDetails($localClass, $localField); + if ($details['needsRelation']) { + $foreignKeyRelationColumn = $schema->sqlColumnForField($foreignClass, "{$foreignKey}Relation", $foreignPrefix); + $joinExpression .= " AND {$foreignKeyRelationColumn} = {$localField}"; + } + } } else { $foreignKeyIDColumn = $schema->sqlColumnForField($foreignClass, $foreignKey, $foreignPrefix); $joinExpression = "{$foreignKeyIDColumn} = {$localIDColumn}"; diff --git a/src/ORM/FieldType/DBPolymorphicRelationAwareForeignKey.php b/src/ORM/FieldType/DBPolymorphicRelationAwareForeignKey.php new file mode 100644 index 00000000000..2eaf2b05fb9 --- /dev/null +++ b/src/ORM/FieldType/DBPolymorphicRelationAwareForeignKey.php @@ -0,0 +1,48 @@ + 'Varchar', + ]; + + /** + * Get the value of the "Relation" this key points to + * + * @return string Name of the has_many relation being stored + */ + public function getRelationValue(): string + { + return $this->getField('Relation'); + } + + /** + * Set the value of the "Relation" this key points to + * + * @param string $value Name of the has_many relation being stored + * @param bool $markChanged Mark this field as changed? + */ + public function setRelationValue(string $value, bool $markChanged = true): static + { + $this->setField('Relation', $value, $markChanged); + return $this; + } + + public function setValue($value, $record = null, $markChanged = true) + { + // We can't map dataobject value because we need to know what has_many relation to store. + if ($value instanceof DataObject) { + throw new InvalidArgumentException('$value must be an associative array - DataObject found'); + } + + parent::setValue($value, $record, $markChanged); + } +} diff --git a/src/ORM/PolymorphicHasManyList.php b/src/ORM/PolymorphicHasManyList.php index 499d8e00355..6c85a3fb0dc 100644 --- a/src/ORM/PolymorphicHasManyList.php +++ b/src/ORM/PolymorphicHasManyList.php @@ -5,6 +5,8 @@ use SilverStripe\Core\ClassInfo; use SilverStripe\Core\Convert; use InvalidArgumentException; +use SilverStripe\Dev\Deprecation; +use Traversable; /** * Represents a has_many list linked against a polymorphic relationship @@ -19,6 +21,12 @@ class PolymorphicHasManyList extends HasManyList */ protected $classForeignKey; + /** + * Name of the foreign key field that references the relation name, for has_one + * relations that can handle multiple reciprocal has_many relations. + */ + protected string $relationForeignKey; + /** * Retrieve the name of the class this relation is filtered by * @@ -29,6 +37,27 @@ public function getForeignClass() return $this->dataQuery->getQueryParam('Foreign.Class'); } + /** + * Retrieve the name of the relation this list is filtered by + */ + public function getForeignRelation(): ?string + { + return $this->dataQuery->getQueryParam('Foreign.Relation'); + } + + /** + * Retrieve the name of the relation this list is filtered by + * + * @deprecated 5.2.0 Will be replaced with a parameter in the constructor + */ + public function setForeignRelation(string $relationName): static + { + Deprecation::notice('5.2.0', 'Will be replaced with a parameter in the constructor'); + $this->dataQuery->where(["\"{$this->relationForeignKey}\"" => $relationName]); + $this->dataQuery->setQueryParam('Foreign.Relation', $relationName); + return $this; + } + /** * Gets the field name which holds the related object class. */ @@ -37,6 +66,17 @@ public function getForeignClassKey(): string return $this->classForeignKey; } + /** + * Gets the field name which holds the has_many relation name. + * + * Note that this will return a value even if the has_one relation + * doesn't support multiple reciprocal has_many relations. + */ + public function getForeignRelationKey(): string + { + return $this->relationForeignKey; + } + /** * Create a new PolymorphicHasManyList relation list. * @@ -50,6 +90,7 @@ public function __construct($dataClass, $foreignField, $foreignClass) // Set both id foreign key (as in HasManyList) and the class foreign key parent::__construct($dataClass, "{$foreignField}ID"); $this->classForeignKey = "{$foreignField}Class"; + $this->relationForeignKey = "{$foreignField}Relation"; // Ensure underlying DataQuery globally references the class filter $this->dataQuery->setQueryParam('Foreign.Class', $foreignClass); @@ -98,11 +139,19 @@ public function add($item) return; } + // set the {$relationName}Class field value $foreignKey = $this->foreignKey; $classForeignKey = $this->classForeignKey; $item->$foreignKey = $foreignID; $item->$classForeignKey = $this->getForeignClass(); + // set the {$relationName}Relation field value if appropriate + $foreignRelation = $this->getForeignRelation(); + if ($foreignRelation) { + $relationForeignKey = $this->getForeignRelationKey(); + $item->$relationForeignKey = $foreignRelation; + } + $item->write(); } @@ -137,6 +186,14 @@ public function remove($item) || $foreignID == $item->$foreignKey || (is_array($foreignID) && in_array($item->$foreignKey, $foreignID ?? [])) ) { + // Unset the foreign relation key if appropriate + $foreignRelation = $this->getForeignRelation(); + if ($foreignRelation) { + $relationForeignKey = $this->getForeignRelationKey(); + $item->$relationForeignKey = null; + } + + // Unset the rest of the relation and write the record $item->$foreignKey = null; $item->$classForeignKey = null; $item->write(); diff --git a/tests/php/Dev/Validation/Member.php b/tests/php/Dev/Validation/Member.php index 1af4a81f7ce..9c302e0d897 100644 --- a/tests/php/Dev/Validation/Member.php +++ b/tests/php/Dev/Validation/Member.php @@ -43,6 +43,8 @@ class Member extends DataObject implements TestOnly */ private static $has_many = [ 'TemporaryMembers' => Freelancer::class . '.TemporaryMember', + 'ManyTeams' => Team::class . '.SingleMember', + 'ManyMoreTeams' => Team::class . '.SingleMember', ]; /** diff --git a/tests/php/Dev/Validation/RelationValidationTest.php b/tests/php/Dev/Validation/RelationValidationTest.php index adc8022e97a..bb6e6484f87 100644 --- a/tests/php/Dev/Validation/RelationValidationTest.php +++ b/tests/php/Dev/Validation/RelationValidationTest.php @@ -6,6 +6,8 @@ use SilverStripe\Core\Config\Config; use SilverStripe\Dev\SapphireTest; use SilverStripe\Dev\Validation\RelationValidationService; +use SilverStripe\ORM\DataObject; +use SilverStripe\ORM\DataObjectSchema; class RelationValidationTest extends SapphireTest { @@ -107,6 +109,14 @@ public function validateCasesProvider(): array 'SilverStripe\Dev\Tests\Validation\Member / Hat : Back relation is ambiguous', ], ], + 'polymorphic has one' => [ + Team::class, + 'has_one', + [ + 'SingleMember' => DataObject::class, + ], + [], + ], 'invalid has one' => [ Member::class, 'has_one', @@ -118,6 +128,45 @@ public function validateCasesProvider(): array 'SilverStripe\Dev\Tests\Validation\Member / HomeTeam : Relation SilverStripe\Dev\Tests\Validation\Team.UnnecessaryRelation is not in the expected format (needs class only format).' ], ], + 'has_one missing class in array config' => [ + Team::class, + 'has_one', + [ + 'SingleMember' => [ + DataObjectSchema::HASONE_MULTIPLE_HASMANY => true, + ], + ], + [ + 'SilverStripe\Dev\Tests\Validation\Team / SingleMember : No class has been defined for this relation.' + ], + ], + 'multi-reciprocal has_one should be polymorphic' => [ + Team::class, + 'has_one', + [ + 'SingleMember' => [ + 'class' => Member::class, + DataObjectSchema::HASONE_MULTIPLE_HASMANY => true, + ], + ], + [ + 'SilverStripe\Dev\Tests\Validation\Team / SingleMember : has_one relation that can handle multiple reciprocal has_many relations must be polymorphic.' + ], + ], + 'has_one defines class in array config' => [ + Team::class, + 'has_one', + [ + 'SingleMember' => [ + 'class' => Member::class, + ], + ], + // Note there's no message about the has_one class, which is technically correctly defined. + // The bad thing now is just that we still have multiple has_many relations pointing at it. + [ + 'SilverStripe\Dev\Tests\Validation\Team / SingleMember : Back relation is ambiguous' + ], + ], 'ambiguous has_many - no relation name' => [ Team::class, 'has_many', diff --git a/tests/php/Dev/Validation/Team.php b/tests/php/Dev/Validation/Team.php index 701423380ab..0e30bade0a0 100644 --- a/tests/php/Dev/Validation/Team.php +++ b/tests/php/Dev/Validation/Team.php @@ -4,6 +4,7 @@ use SilverStripe\Dev\TestOnly; use SilverStripe\ORM\DataObject; +use SilverStripe\ORM\DataObjectSchema; use SilverStripe\ORM\HasManyList; use SilverStripe\ORM\ManyManyList; use SilverStripe\ORM\ManyManyThroughList; @@ -31,6 +32,13 @@ class Team extends DataObject implements TestOnly 'Title' => 'Varchar(255)', ]; + private static array $has_one = [ + 'SingleMember' => [ + 'class' => DataObject::class, + DataObjectSchema::HASONE_MULTIPLE_HASMANY => true, + ], + ]; + /** * @var array */