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

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

Closed
GuySartorelli opened this issue Oct 11, 2023 · 2 comments
Closed

Comments

@GuySartorelli
Copy link
Member

GuySartorelli commented Oct 11, 2023

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.
  1. Everywhere that instantiates a HasManyList (eager loading, DataObject::getComponents(), etc) would need to account for this new behaviour
  2. DataQuery::joinHasManyRelation() probably needs to account for the new column
  3. 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

  • This does not replace dot notation for has_many - if there is more than one has_one that the has_many could be targetting, the has_many still needs to know which has_one it's supposed to save its relation against in the first place. Dot notation is still required to make that association.
@GuySartorelli
Copy link
Member Author

GuySartorelli commented Oct 12, 2023

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
Copy link
Member Author

This was resolved in #11084

@GuySartorelli GuySartorelli closed this as not planned Won't fix, can't repro, duplicate, stale May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant