silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
720 stars 823 forks source link

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

Closed GuySartorelli closed 3 months ago

GuySartorelli commented 11 months ago

Currently, there's no way to have multiple has_many relations on a class which all link back to the same has_one relation, such as this example:

class MyChildObject extends DataObject
{
    private static $has_one = [
        'Parent' => MyParentObject::class
    ];
}

class MyParentObject extends DataObject
{
    private static $has_many = [
        'ListOne' => MyChildObject::class,
        'ListTwo' => MyChildObject::class,
    ];
}

With this code, adding a child record to either has_many list will result in it displaying in both. The current documented way to do this is to have two has_one relations and use dot notation to indicate which has_many is for which has_one, but that's not useful if you're creating a model in a module and you want the permissions (can* methods) to related to the parent has_one relation.

So, for example, if I implemented this method in MyChildObject:

public function canView($member = null)
{
    $parent = $this->Parent();
    if ($parent->exists()) {
        return $parent->canView($member);
    }
    return parent::canView($member);
}

In that scenario, the permissions are only correct for whichever has_many ends up having its dot notation point at that relation.

Proposed solution

On the has_one relation, save the has_many relation it corresponds to as well (similar to how polymorphic has_one stores the classname)

This would mean the following changes (at a minimum) would be required:

  1. DataObjectSchema::cacheDatabaseFields() needs to add a new "{$fieldName}Relation" or similarly named database column for all has_one relations
  2. We would need to either somehow backfill the new column, or only use the new db column in the event that multiple has_many relations can point to it. The latter would probably be too hard to detect.
  3. HasManyList would need to accept a relation name either as a constructor argument or via a new setter method, or both.
    • The relation name is the name of the has_many relation it represents and would be used in its query (WHERE {$fieldName}Relation = {$this->relationName}) and to store the value against records that are added to the list.
  4. Everywhere that instantiates a HasManyList (eager loading, DataObject::getComponents(), etc) would need to account for this new behaviour
  5. DataQuery::joinHasManyRelation() probably needs to account for the new column
  6. Make sure that all of that doesn't break for a given has_one that doesn't relate to a has_many - there's no need to store the relation in any other scenario I think but we gotta make sure nothing else breaks.

NOTES

GuySartorelli commented 11 months ago

Note that an alternative solution, which would not be a breaking change, would be to allow opting-in to storing the has_many relation in a join table - with the inverse relationship being a belongs_to.

This is the approach Doctrine takes to handle this use case - you have have the same approach we have by default (store in has_one, has_many looks up the has_one), or you can opt to use a join table.

This is also how the Eloquent ORM works - it has the equivalent of a has_many which stores its relational data in a join table, with a belongs_to as the (optional) inverse relationship. It only uses the has_one style when dealing with polymorphic relations.

Notes about this

This is good for a concrete or polymorphic has_many with a concrete belongs_to but doesn't solve the use case that this issue was intended to solve, which is for the has_one (or belongs_to) side to be polymorphic.

GuySartorelli commented 3 months ago

This was resolved in https://github.com/silverstripe/silverstripe-framework/pull/11084