Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NEW Allow a single has_one to manage multiple reciprocal has_many #11084

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions _config/model.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
29 changes: 28 additions & 1 deletion src/Dev/Validation/RelationValidationService.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -291,6 +293,25 @@ 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 (($spec[DataObjectSchema::HAS_ONE_MULTI_RELATIONAL] ?? false) === 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;
}
Expand All @@ -305,6 +326,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,
Expand Down Expand Up @@ -616,7 +642,8 @@ protected function parseManyManyRelation($relationData): ?string
return null;
}

return $throughRelations[$to];
$spec = $throughRelations[$to];
return is_array($spec) ? $spec['class'] ?? null : $spec;
}

return $relationData;
Expand Down
8 changes: 8 additions & 0 deletions src/Forms/GridField/GridFieldDetailForm_ItemRequest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
}

Expand Down
41 changes: 27 additions & 14 deletions src/ORM/DataObject.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
use SilverStripe\Core\Injector\Injector;
use SilverStripe\Core\Resettable;
use SilverStripe\Dev\Debug;
use SilverStripe\Dev\Deprecation;
use SilverStripe\Forms\FieldList;
use SilverStripe\Forms\FormField;
use SilverStripe\Forms\FormScaffolder;
Expand Down Expand Up @@ -1972,12 +1973,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']) {
Deprecation::withNoReplacement(fn () => $result->setForeignRelation($componentName));
}
} else {
$result = HasManyList::create($componentClass, $joinField);
$result = HasManyList::create($componentClass, $details['joinField']);
}

return $result
Expand All @@ -1993,16 +1996,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);
emteknetnz marked this conversation as resolved.
Show resolved Hide resolved
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 : []
);
Expand Down Expand Up @@ -2228,15 +2236,20 @@ public function getManyManyComponents($componentName, $id = null)
}

/**
* Return the class of a one-to-one component. If $component is null, return all of the one-to-one components and
* their classes. If the selected has_one is a polymorphic field then 'DataObject' will be returned for the type.
* Return the class of a all has_one relations.
*
* @return string|array The class of the one-to-one component, or an array of all one-to-one components and
* their classes.
* @return array An array of all has_one components and their classes.
Comment on lines -2231 to +2241
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just a plain lie before. Updated to indicate the actual functionality of this method.

*/
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(static::class, $relationName);
}
}
Comment on lines +2247 to +2251
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No early returns or breaks here? Can there be multiple and if so, is using the last ok?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean?
This will loop through all has_one relation declarations, and reduce the new array syntax for any of those relations down to just their class name.

There's nothing to break early for because we have to check each relation.
We're not "using the last" of anything - note we're assigning the same element that we're looking at.

e.g:

private static $has_one = [
    'NormalHasOne' => DataObject::class,
    'FancyHasOne' => [
        'class' => DataObject::class,
        DataObjectSchema::HASONE_IS_MULTIRECIPROCAL => true,
    ],
    'AnotherHasOne' => DataObject::class,
];

calling $record->hasOne() for a model with the above definition will return an array like so:

[
    'NormalHasOne' => DataObject::class,
    'FancyHasOne' => DataObject::class,
    'AnotherHasOne' => DataObject::class,
]

return $hasOne;
}

/**
Expand Down
104 changes: 95 additions & 9 deletions src/ORM/DataObjectSchema.php
Original file line number Diff line number Diff line change
Expand Up @@ -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 HAS_ONE_MULTI_RELATIONAL = 'multirelational';

/**
* Default separate for table namespaces. Can be set to any string for
* databases that do not support some characters.
Expand Down Expand Up @@ -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 (($spec[self::HAS_ONE_MULTI_RELATIONAL] ?? false) === true) {
$compositeFields[$fieldName] = 'PolymorphicRelationAwareForeignKey';
continue;
}
} else {
$hasOneClass = $spec;
}
if ($hasOneClass === DataObject::class) {
$compositeFields[$fieldName] = 'PolymorphicForeignKey';
} else {
Expand Down Expand Up @@ -902,12 +920,34 @@ 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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Above in RelationValidationServive you have is_array($spec) ? $spec['class'] ?? null : $spec; - here it's missing the ?? null - is that an issue? should it be added here, or remove in RelationValidationService?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't needed here because an exception will be thrown in checkHasOneArraySpec if it's missing.
It is needed in the relation validation service because this validation isn't performed within that service.

$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 hasOneComponentHandlesMultipleRelations(string $class, string $component): bool
{
$hasOnes = Config::forClass($class)->get('has_one');
if (!isset($hasOnes[$component])) {
return false;
}

$spec = $hasOnes[$component];
return ($spec[self::HAS_ONE_MULTI_RELATIONAL] ?? false) === true;
}

/**
* Return data for a specific belongs_to component.
*
Expand Down Expand Up @@ -1047,6 +1087,20 @@ protected function getManyManyInverseRelationship($childClass, $parentClass)
*/
public function getRemoteJoinField($class, $component, $type = 'has_many', &$polymorphic = false)
{
return $this->getBelongsToAndHasManyDetails($class, $component, $type, $polymorphic)['joinField'];
}

public function getHasManyComponentDetails(string $class, string $component): array
{
return $this->getBelongsToAndHasManyDetails($class, $component);
}

private function getBelongsToAndHasManyDetails(
string $class,
string $component,
string $type = 'has_many',
&$polymorphic = false
): array {
// Extract relation from current object
if ($type === 'has_many') {
$remoteClass = $this->hasManyComponent($class, $component, false);
Expand All @@ -1071,6 +1125,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
Expand Down Expand Up @@ -1104,14 +1163,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->hasOneComponentHandlesMultipleRelations($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,
];
}

/**
Expand Down Expand Up @@ -1204,6 +1272,24 @@ protected function checkManyManyJoinClass($parentClass, $component, $specificati
return $joinClass;
}

private 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 (($spec[self::HAS_ONE_MULTI_RELATIONAL] ?? false) === 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
*
Expand Down
11 changes: 9 additions & 2 deletions src/ORM/DataQuery.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
*
Comment on lines -1028 to -1029
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment was already wrong - it had code here that made it work with polymorphic relationships before I came along. I'm just adding support for the new multiple-reciprocal has_one relations.

* @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
Expand Down Expand Up @@ -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}";
Expand Down
38 changes: 38 additions & 0 deletions src/ORM/FieldType/DBPolymorphicRelationAwareForeignKey.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
<?php

namespace SilverStripe\ORM\FieldType;

use InvalidArgumentException;
use SilverStripe\ORM\DataObject;

/**
* A special polymorphic ForeignKey class that allows a single has_one relation to map to multiple has_many relations
*/
class DBPolymorphicRelationAwareForeignKey extends DBPolymorphicForeignKey
{
private static array $composite_db = [
'Relation' => '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;
Comment on lines +33 to +36
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public function setRelationValue(string $value, bool $markChanged = true): static
{
$this->setField('Relation', $value, $markChanged);
return $this;
public function setRelationValue(string $value, bool $markChanged = true): void
{
$this->setField('Relation', $value, $markChanged);

setIDValue() / setClassValue() both return void in DBPolymorphicForeignKey, seems weird making this one alone chainable

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}
}
Loading