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

GuySartorelli · 2023-12-04T04:48:39Z

This PR provides a new has_one syntax that allows for a single has_one relation to be pointed at by multiple has_many relations - and each will correctly retain only their own items, without bleeding between them.

Without this PR, if you wanted multiple has_many relations on class A all pointing at class B, you would need to have a separate has_one relation on class B for each of the has_many relations.

Issue

Reverse ownership silverstripe-linkfield#5

src/ORM/DataObject.php

GuySartorelli · 2023-12-04T23:27:52Z

src/ORM/DataObject.php

    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;
    }


This one method is the reason this whole PR works without being a breaking change. Wherever this is called, people can rely on having an associative array of relation name to class name for has_one relations.

Anywhere people are currently using SomeClass::config()->get('has_one'), they should probably have been using this instead - and that advice will be included in the changelog.

GuySartorelli · 2023-12-04T23:29:38Z

src/ORM/DataQuery.php

-     * Doesn't work with polymorphic relationships
-     *


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.

GuySartorelli · 2023-12-05T00:07:43Z

src/ORM/DataObject.php

-     * 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.


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

GuySartorelli · 2023-12-05T02:26:02Z

tests/php/ORM/DataObjectSchemaTest.php

@@ -159,68 +159,94 @@ public function testTableForObjectField()
        );
    }

-    public function testFieldSpec()
+    public function provideFieldSpec()


All of the scenarios in this provider except for the last one come from the original test - I've just refactored it to use a provider so it's clearer what's going on.

michalkleiner · 2023-12-05T03:50:53Z

src/ORM/DataObject.php

+        foreach ($hasOne as $relationName => $spec) {
+            if (is_array($spec)) {
+                $hasOne[$relationName] = DataObject::getSchema()->hasOneComponent(static::class, $relationName);
+            }
+        }


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

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, ]

michalkleiner · 2023-12-05T03:58:13Z

I looked at the linked ticket for linkfield and looked at the PR here but it's still not clear to me what high value issue we are solving here and whether it even was an issue to have to have a multiple has_ones for the same class. It made it clear and separated.. now it feels a bit more like magic with all the required extra config. I guess both approaches will now work.

GuySartorelli · 2023-12-05T04:04:07Z

I looked at the linked ticket for linkfield and looked at the PR here but it's still not clear to me what high value issue we are solving here and whether it even was an issue to have to have a multiple has_ones for the same class. It made it clear and separated.. now it feels a bit more like magic with all the required extra config. I guess both approaches will now work.

The specific scenario we're trying to solve here is that we want the Link model to know who its "owner" record is, so it can ask that record what permissions the current user has (e.g. the link can only be edited if its owner can be edited).

This also makes it easier to say (for example) "I want 3 different lists of links on my SiteConfig" - you just have to define the has_many relations in your SiteConfig extension. You don't also have to add 3 new has_one relations to the link class via yaml.

It made it clear and separated.

Having to define multiple has_one relations to handle corresponding has_many relations has never been clear to me. Might just be a me thing, but that's caught me off guard time and again, and has looked untidy to me having to declare:

Some\Vendor\Model:
  has_one:
    SiteConfig1: SilverStripe\SiteConfig\SiteConfig
    SiteConfig2: SilverStripe\SiteConfig\SiteConfig
    SiteConfig3: SilverStripe\SiteConfig\SiteConfig

emteknetnz

When looking up the has_many relation e.g. Link->Owner() should raise something that gets logged but not hard exception if {relationname}Relation does not exist on the model because the relation was either deleted or renamed. Can't use hard exception because we're dealing with data which can easily be different in prod than local when testing

Just an idea, is it possible to simplify this so that you don't need to 'opt-in' this this behaviour e.g. instead of this

        'Owner' => [
            'class' => DataObject::class,
            DataObjectSchema::HASONE_IS_MULTIRECIPROCAL => true,
        ],

You just have this

        'Owner' => DataObject::class,

And then do the new clever stuff if the relationship is polymorphic i.e. DataObject::class AND the {relationname}Relation column is non empty?

GuySartorelli · 2023-12-06T03:30:07Z

When looking up the has_many relation e.g. Link->Owner() should raise something that gets logged but not hard exception if {relationname}Relation does not exist on the model because the relation was either deleted or renamed. Can't use hard exception because we're dealing with data which can easily be different in prod than local when testing

I was hoping we could do this, but looking again at this, I don't think there's a sensible place where we can throw this sort of warning. The only times we really look at the relation name are:

Saving a record to the relation, in which case we're setting the value not reading it
Getting the list of records in a PolymorphicHasManyList, in which case we're filtering by the value rather than interrogating the has_one on a specific record

Renaming relations is pretty uncommon anyway, I'd hope - and for most relations if you change the name you'll need to do some sort of data migration already. I think we can accept that this is another case where a manual data migration will be necessary and until such a migration is done there'll be orphaned records.

Just an idea, is it possible to simplify this so that you don't need to 'opt-in' this this behaviour ... then do the new clever stuff if the relationship is polymorphic

I can't think of a way to make that work without data loss, for essentially the same reason we can't throw a warning for has_ones.

The way it works with the new config relies on passing the has_many relation to the PolymorphicHasManyList only if the relation is pointing at a multi-reciprocal has_one relation.
Then, it always stores the relation name if it's given one, and it always filters against the relation name if it's given one.

If we were to apply multi-reciprocal behaviour to all polymorphic has_many relations, we'd have to filter all of them by both the relation name and relation name = null. But because we're allowing relation name to be null, this makes no distinction between "that item belongs in this list" and "we're really not sure where that item belongs".

Why this breaks down

People could decide "I want to add a second has_many to point at that existing polymorphic has_one". If they do so, they'll end up with all the records from their existing has_many relation showing up in their new has_many list, which is exactly what this PR is intended to avoid.

michalkleiner · 2023-12-06T05:27:11Z

I looked at the linked ticket for linkfield and looked at the PR here but it's still not clear to me what high value issue we are solving here and whether it even was an issue to have to have a multiple has_ones for the same class. It made it clear and separated.. now it feels a bit more like magic with all the required extra config. I guess both approaches will now work.

The specific scenario we're trying to solve here is that we want the Link model to know who its "owner" record is, so it can ask that record what permissions the current user has (e.g. the link can only be edited if its owner can be edited).

This also makes it easier to say (for example) "I want 3 different lists of links on my SiteConfig" - you just have to define the has_many relations in your SiteConfig extension. You don't also have to add 3 new has_one relations to the link class via yaml.

I never had a need where many_many wouldn't be sufficient for that. Of course there might be others with different requirements.

It made it clear and separated.

Having to define multiple has_one relations to handle corresponding has_many relations has never been clear to me. Might just be a me thing, but that's caught me off guard time and again, and has looked untidy to me having to declare:
Some\Vendor\Model:
  has_one:
    SiteConfig1: SilverStripe\SiteConfig\SiteConfig
    SiteConfig2: SilverStripe\SiteConfig\SiteConfig
    SiteConfig3: SilverStripe\SiteConfig\SiteConfig

Fair enough. I would name them based on what they were representing, even if all would link to the same model for some reason, but more often we'd use an extension to the vendor model where you'd put all the config and code, like you'd do with project models.

Even though the use case here might be narrow (and doesn't talk about the advantages of using has_one/has_many vs many_many), I'm not against it and could be solving a legitimate problem.
It's just yet another thing that most folks outside of the CMS squad don't have visibility of (with a lack of public roadmap and a list of high value/prioritised issues somewhere), which might be contributing to my lack of understanding and buy-in here.

emteknetnz · 2023-12-07T00:20:45Z

src/ORM/FieldType/DBPolymorphicRelationAwareForeignKey.php

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


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

I'm following the documented best practices for new setter method implementations: https://github.com/silverstripe/developer-docs/blob/5/en/05_Contributing/05_Coding_Conventions/01_PHP_Coding_Conventions.md#properties-and-accessors

emteknetnz · 2023-12-07T00:52:56Z

src/ORM/DataObjectSchema.php

+    /**
+     * Configuration key for has_one relations that can support multiple reciprocal has_many relations.
+     */
+    public const HASONE_IS_MULTIRECIPROCAL = 'multireciprocal';


Suggested change

public const HASONE_IS_MULTIRECIPROCAL = 'multireciprocal';

public const HAS_ONE_MULTI_RELATIONAL = 'multi-relational';

HASONE_IS_MULTIRECIPROCAL is a pretty weird name IMO

HAS_ONE_MULTI_RELATIONAL seems much clearer:

Uses 'RELATIONAL' instead of 'RECIPROCAL' which lines up with it adding a 'Relation' column. Reciprocal seems much less obvious why it's called that, at least to me

Removes the redundant "IS"

Adds in underscores

Seem alright?

multi-relation makes it sound like this is multiple relations.... which it isn't. But I'll make this change because I know we'll never agree on this and at the end of the day it's more important that there is a name than that it's a name we agree on.

It's probably worth noting though that reciprocal is a real word and was being used correctly, and has a precedent (if a fairly minor one) both in our docs and in code (e.g. DataObject::inferReciprocalComponent() (where component effectively means relation))

emteknetnz · 2023-12-07T00:54:05Z

src/Dev/Validation/RelationValidationService.php

+                if (isset($spec[DataObjectSchema::HASONE_IS_MULTIRECIPROCAL])
+                    && $spec[DataObjectSchema::HASONE_IS_MULTIRECIPROCAL] === true
+                    && $relationData !== DataObject::class


Suggested change

if (isset($spec[DataObjectSchema::HASONE_IS_MULTIRECIPROCAL])

&& $spec[DataObjectSchema::HASONE_IS_MULTIRECIPROCAL] === true

&& $relationData !== DataObject::class

if ($spec[DataObjectSchema::HASONE_IS_MULTIRECIPROCAL] ?? false === true

&& $relationData !== DataObject::class

Everywhere you've asked for this change I've wrapped it in quotes because tests were failing when I didn't.

emteknetnz · 2023-12-07T00:56:02Z

src/ORM/DataObject.php

+        if ($details['polymorphic']) {
+            $result = PolymorphicHasManyList::create($componentClass, $details['joinField'], static::class);
+            if ($details['needsRelation']) {
+                Deprecation::withNoReplacement(fn () =>$result->setForeignRelation($componentName));


Suggested change

Deprecation::withNoReplacement(fn () =>$result->setForeignRelation($componentName));

Deprecation::withNoReplacement(fn () => $result->setForeignRelation($componentName));

emteknetnz · 2023-12-07T00:56:36Z

src/ORM/DataObjectSchema.php

+                }
+                // Handle has_one which handles multiple reciprocal has_many relations
+                $hasOneClass = $spec['class'];
+                if (isset($spec[self::HASONE_IS_MULTIRECIPROCAL]) && $spec[self::HASONE_IS_MULTIRECIPROCAL] === true) {


Suggested change

if (isset($spec[self::HASONE_IS_MULTIRECIPROCAL]) && $spec[self::HASONE_IS_MULTIRECIPROCAL] === true) {

if ($spec[self::HASONE_IS_MULTIRECIPROCAL] ?? false === true) {

emteknetnz · 2023-12-07T01:11:54Z

src/ORM/DataObjectSchema.php

@@ -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'];


Suggested change

return $this->getBelongsAndHasManyDetails($class, $component, $type, $polymorphic)['joinField'];

return $this->getBelongsToAndHasManyDetails($class, $component, $type, $polymorphic)['joinField'];

emteknetnz · 2023-12-07T01:12:22Z

src/ORM/DataObjectSchema.php

+        if (isset($spec[self::HASONE_IS_MULTIRECIPROCAL])
+            && $spec[self::HASONE_IS_MULTIRECIPROCAL] === true
+            && $spec['class'] !== DataObject::class


Suggested change

if (isset($spec[self::HASONE_IS_MULTIRECIPROCAL])

&& $spec[self::HASONE_IS_MULTIRECIPROCAL] === true

&& $spec['class'] !== DataObject::class

if ($spec[self::HASONE_IS_MULTIRECIPROCAL] ?? false === true

&& $spec['class'] !== DataObject::class

Done but had to wrap in brackets to avoid broken functionality

emteknetnz · 2023-12-07T01:13:13Z

src/ORM/PolymorphicHasManyList.php

            $item->$foreignKey = null;
            $item->$classForeignKey = null;
            $item->write();
        }
    }
+
+    private function relationMatches(?string $actual, ?string $expected)


Suggested change

private function relationMatches(?string $actual, ?string $expected)

private function relationMatches(?string $actual, ?string $expected): bool

emteknetnz · 2023-12-07T01:13:32Z

tests/php/Forms/GridField/GridFieldDetailFormTest.php

@@ -143,6 +145,31 @@ public function testAddFormWithPolymorphicHasOne()
        $this->assertEquals($group->ID, $record->PolymorphicGroupID);
    }

+    public function testAddFormWithMultiReciprocalHasOne()


Suggested change

public function testAddFormWithMultiReciprocalHasOne()

public function testAddFormWithMultiReciprocalHasOne(): void

emteknetnz · 2023-12-07T01:13:51Z

tests/php/ORM/DataObjectSchemaTest.php

+    /**
+     * @dataProvider provideFieldSpec
+     */
+    public function testFieldSpec(array $args, array $expected)


Suggested change

public function testFieldSpec(array $args, array $expected)

public function testFieldSpec(array $args, array $expected): void

GuySartorelli marked this pull request as draft December 4, 2023 04:48

This was referenced Dec 4, 2023

NEW Link ownership (alternative to #101) silverstripe/silverstripe-linkfield#102

Closed

POC Add new extension hook to update hasmany lists #10973

Closed

Reverse ownership silverstripe/silverstripe-linkfield#5

Closed

GuySartorelli force-pushed the pulls/5/hasone_multiple_hasmany branch 3 times, most recently from 359851a to a12982c Compare December 4, 2023 23:18

GuySartorelli commented Dec 4, 2023

View reviewed changes

src/ORM/DataObject.php Show resolved Hide resolved

GuySartorelli commented Dec 4, 2023

View reviewed changes

GuySartorelli force-pushed the pulls/5/hasone_multiple_hasmany branch 3 times, most recently from c42c13b to fcd6f8c Compare December 5, 2023 00:07

GuySartorelli commented Dec 5, 2023

View reviewed changes

GuySartorelli force-pushed the pulls/5/hasone_multiple_hasmany branch 4 times, most recently from d151c93 to 3f39f22 Compare December 5, 2023 02:18

GuySartorelli marked this pull request as ready for review December 5, 2023 02:23

GuySartorelli commented Dec 5, 2023

View reviewed changes

GuySartorelli force-pushed the pulls/5/hasone_multiple_hasmany branch from 3f39f22 to fccccab Compare December 5, 2023 02:35

This was referenced Dec 5, 2023

DOC Document multi-reciprocal has_one relations silverstripe/developer-docs#417

Merged

API Add new Owner relation for handling permissions silverstripe/silverstripe-linkfield#127

Merged

michalkleiner reviewed Dec 5, 2023

View reviewed changes

GuySartorelli mentioned this pull request Dec 5, 2023

ENH Respect new has_one config silverstripe/silverstripe-versioned#427

Merged

emteknetnz requested changes Dec 6, 2023

View reviewed changes

emteknetnz requested changes Dec 7, 2023

View reviewed changes

GuySartorelli force-pushed the pulls/5/hasone_multiple_hasmany branch 2 times, most recently from 973224d to 915483b Compare December 10, 2023 22:32

NEW Allow a single has_one to manage multiple reciprocal has_many

d400125

GuySartorelli force-pushed the pulls/5/hasone_multiple_hasmany branch from 915483b to d400125 Compare December 10, 2023 22:51

emteknetnz approved these changes Dec 11, 2023

View reviewed changes

emteknetnz merged commit c405ed6 into silverstripe:5 Dec 11, 2023
15 checks passed

GuySartorelli deleted the pulls/5/hasone_multiple_hasmany branch December 11, 2023 21:39

GuySartorelli mentioned this pull request May 22, 2024

Can't have multiple has_many relations for a single has_one relation. #10971

Closed

	public const HASONE_IS_MULTIRECIPROCAL = 'multireciprocal';
	public const HAS_ONE_MULTI_RELATIONAL = 'multi-relational';

	Deprecation::withNoReplacement(fn () =>$result->setForeignRelation($componentName));
	Deprecation::withNoReplacement(fn () => $result->setForeignRelation($componentName));

	if (isset($spec[self::HASONE_IS_MULTIRECIPROCAL]) && $spec[self::HASONE_IS_MULTIRECIPROCAL] === true) {
	if ($spec[self::HASONE_IS_MULTIRECIPROCAL] ?? false === true) {

	return $this->getBelongsAndHasManyDetails($class, $component, $type, $polymorphic)['joinField'];
	return $this->getBelongsToAndHasManyDetails($class, $component, $type, $polymorphic)['joinField'];

	private function relationMatches(?string $actual, ?string $expected)
	private function relationMatches(?string $actual, ?string $expected): bool

	public function testAddFormWithMultiReciprocalHasOne()
	public function testAddFormWithMultiReciprocalHasOne(): void

	public function testFieldSpec(array $args, array $expected)
	public function testFieldSpec(array $args, array $expected): void

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

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

Conversation

GuySartorelli commented Dec 4, 2023 • edited Loading

Issue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalkleiner commented Dec 5, 2023 • edited Loading

GuySartorelli commented Dec 5, 2023 • edited by michalkleiner Loading

emteknetnz left a comment

Choose a reason for hiding this comment

GuySartorelli commented Dec 6, 2023 • edited Loading

Why this breaks down

michalkleiner commented Dec 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emteknetnz Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli Dec 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli commented Dec 4, 2023 •

edited

Loading

michalkleiner commented Dec 5, 2023 •

edited

Loading

GuySartorelli commented Dec 5, 2023 •

edited by michalkleiner

Loading

GuySartorelli commented Dec 6, 2023 •

edited

Loading

emteknetnz Dec 7, 2023 •

edited

Loading

GuySartorelli Dec 10, 2023 •

edited

Loading