NEW Link ownership (alternative to #101)

[GuySartorelli]

TL;DR: Only look at the last commit

quick link to view the commit

Note that this PR is based on #45 as it relies on the LinkField and ManyLinkField doing the heavy lifting.
This would also work just as well with the AnyField implementations, but I'm using that PR for simplicity to keep everything in one place as much as possible.

In order to correctly see what changes I'm proposing, exclusively look at the changes in the last commit! Ignore the other commits, as they're just there to get a has_many linkfield implementation in the first place.

What this PR does

• Adds a new OwnerRelation DB column which stores the name of the relation on the owner class. This allows us to link multiple has_many relations to a single has_one, which means we always know who our owner is (normally a separate has_one would be required for each has_many)
• Adds a new extension to DataObject which filters the has_many by the new OwnerRelation column to get only the relevant records. There may be edge cases that this doesn't catch (e.g. when inferReciprocalComponent() is used) but this is a POC. I'm not looking for all the possible edge cases right now.
• For owners with a has_one to link, it stores that info in the link table as well. This is double handling, but means the link always knows who its owner is. We also override the Owner() magic method to check if the owner is still valid in the event of a has_one to has_one relationship.
• Adds can* checks as per Permisson model #12

Limitations

• This ONLY works when you create the Link records using LinkField or MultiLinkField (and that obviously can be ported to whatever implementation we choose to ship with this module).
  • If you create has_many links in a gridfield, it won't add the relation name so you won't be able to have multiple has_many records
  • If you create has_one links in one of the community provided has one fields (e.g. silvershop/silverstripe-hasonefield) it won't add the relation so your can* checks won't be based on the owner.
• This relies on the new extension being applied - if we don't apply this by default, people may forget to apply it or apply it too late and things will fall apart. If we do apply it by default, that's another extension applied to DataObject which won't be necessary for most subclasses of DataObject.
• This relies on POC Add new extension hook to update hasmany lists silverstripe-framework#10973 to work.
• I haven't checked things like cascade_deletes, cascade_duplicates, and owns work the way they should - but they probably do.

Issues

• Reverse ownership #5
• Permisson model #12

[GuySartorelli]

Obviously this should be in LinkField instead of JsonField - but for the sake of this PR it's clearer to just put it here rather than refactor LinkField to make that work.

This is required because the owner will inevitably use a has_one instead of a belongs_to - and that means the data for the relation will be stored on the owner's db table. We need to also store it in the Link's table so that we can refer to it for can* checks, and link to the owner if we add e.g. broken link reports in the future.

In theory this being here also allows developers to use a belongs_to to link (assuming that can't already be done)

[GuySartorelli]

Doing this check here means we will never be falsely stating that we have an owner when the owner doesn't think it owns the link anymore. Resolves the problem of storing the data in two places by ignoring that secondary data source if it's incorrect

[emteknetnz]

Seem like like it would be better to simply not add the data in two places for has_one's and instead just use the normal way of doing it i.e. only save it on Page.MyLinkID? Not sure why we'd want to double handle it?

Seems like your thinking was the relationships work like this:

a) Page.has_one = [ MyLink => Link::class ] -- Link.has_one = [ Owner => Page::class ] <<< seems wrong
b) Page.has_many = [ SomeLinks => Links::class ] -- Link.has_one = [ Owner => Page::class ]

The relationships should be:

a) Page.has_one = [ MyLink => Link::class ] -- Link.belongs_to = [ OptionalOwner => Page::class ]
b) Page.has_many = [ SomeLinks => Links::class ] -- Link.has_one = [ Owner => Page::class ]

In practice a) Link.belongs_to won't get filled in, though that's fine

This means for the Page.has_one's we don't need Link.OwnerID/OwnerClass/OwnerRelation filled in

Let me know if I've understood this correctly

[GuySartorelli]

We can't have a polymorphic belongs_to - i.e. this won't work:

private static $belongs_to = [
    'OptionalOwner' => DataObject::class,
];

And we don't want to explicitly say it has to be a Page (or SiteTree) that owns the link - that severely limits the use case, especially since bespoke are saying they want this mostly for use inside elemental blocks.

Yes my proposed solution is double handling, but it is the only way we have have a one-to-one polymorphic relation like this, to my knowledge. The double handling is mitigated by what I'm doing here to validate the data is still correct before providing the record when $link->Owner() is called.

[GuySartorelli]

Note that we could also say "you have to use belongs_to instead of has_one in your model" but you already rejected that because it's not as intuitive as adding a has_one.

[emteknetnz]

OK so can't have a belongs_to OptionalOwner. So we'd have this:

a) Page.has_one = [ MyLink => Link::class ]
b) Page.has_many = [ SomeLinks => Links::class ] -- Link.has_one = [ Owner => Page::class ]

Both scenario's have has_one's which is the relationship that actually adds something to the database, so technically this is all we need to at least get things rendering in the correct place

I guess the downside here is now in a) you can't get the Owner() of the Link in in order to do a canCheck(). This is could be worked around if you you assume that you're strictly editing Links in the CMS on a Page/Element Form in which case the canCheck() for Link is implicitly done when rendering the Form.

However this falls apart if you have a GridField of Link's, or using XHR to call an endpoint to interact with the dataobject's directly

This is essentially the permission model for linkfield now i.e. there isn't one

If guess we're going to have a permissions model that calls Owner()->canCheck() then we'll need to double handle, or we just (technically correctly) use $belongs_to on Page so the foriegn key isn't added there, though that be an awful upgrade experience (all linkfields currently use $has_one) and is just generally unintuitive (i.e. saying that a Page belongs-to a Link rather than a Page has-one Link)

[GuySartorelli]

Yeah the has_one to has_one connection I'm proposing here is specifically and exclusively so we can refer to the Owner record from the Link.

This is useful for:

• Correct can* checks (regardless of where you're accessing the link from)
• Linking back to the owner e.g. from a broken links report

I personally think it's worthwhile having, but we can live without it if we decide we want simpler can* checks (e.g. gorriecoe/silverstripe-link just assumes everyone can do everything) and we don't care about being able to access the owner in any given scenario. In that case I'd recommend we never refer to the Owner relation in code except for when declaring the has_one which would only be there as a way to store the has_many data in the database. Other than storing that data, if we're not using it for one-to-one relationships as well then we should pretend it doesn't exist.

The main purpose of this POC is for the has_many to has_one stuff anyway.

[emteknetnz]

"double check the owner"

I do like this returns null since there are two has_ones pointing at each other, it's treating Page as the "source of truth".

We're really only adding the has_one to Link here out of necessity to make our canCheck() work.

This double has_one is inherently fragile as things can get out of sync. Need to assume that we're not use the LinkField UI to manage links so everything needs to happen at the model level

Perhaps there's additional things we can do to strengthen this?

• Disallow changing OwnerID/OwnerClass/OwnerRelation on Link if OwnerID !== 0 (though maybe do allow them to be set back to 0/empty)
• DataObjectExtension onBeforeWrite ... if a has_one that's changed is for a Link::class, then gets gets the OLD Link and reset OwnerID/OwnerClass/OwnerRelation back to 0/empty. Though at this point maybe we're better off just outright deleting the Link).

[GuySartorelli]

We're really only adding the has_one to Link here out of necessity to make our canCheck() work.

And as a way to allow us to link to of refer to the Owner for any other purpose, such as adding a link to the cms edit form from a broken links report, etc. It's mostly for the can* checks, but not exclusively.

Need to assume that we're not use the LinkField UI to manage links so everything needs to happen at the model level

We're already throwing that assumption out the window for the has_many relation here. I think for anything in this POC to work we have to say "If you are creating new links, you must use the link fields provided in this module".
You should be able to edit links fine in any context because the relation data has already been saved by that point.

Perhaps there's additional things we can do to strengthen this?

Quite possibly. Remember that this is just a POC and again it's mostly just to ensure we can have multiple has_many relations on the owner side. The one-to-one is just extra sugar on top.

[emteknetnz]

Approach looks really good. I've tested locally to confirm that adding multiple links to multiple has many's will render in a template correctly

I've left a couple of comments on the last commit

[emteknetnz]

This bit's a bit hard to understand, I'd just simplify this to something like (untested)

$check = ucfirst(strtolower($perm));
return match ($check) {
    'View', 'Create', 'Edit', 'Delete' => $this->{"can$check"}($member, $context),
    default => parent::can($perm, $member, $context)
}

[GuySartorelli]

Makes sense. I won't make the change right now (the underlying PR that this relies on needs to be merged or swapped out for anyfield stuff or something first anyway) but we'll go with that when it's time to turn this from a POC into a real PR.

[emteknetnz]

Seem like like it would be better to simply not add the data in two places for has_one's and instead just use the normal way of doing it i.e. only save it on Page.MyLinkID? Not sure why we'd want to double handle it?

Seems like your thinking was the relationships work like this:

a) Page.has_one = [ MyLink => Link::class ] -- Link.has_one = [ Owner => Page::class ] <<< seems wrong
b) Page.has_many = [ SomeLinks => Links::class ] -- Link.has_one = [ Owner => Page::class ]

The relationships should be:

a) Page.has_one = [ MyLink => Link::class ] -- Link.belongs_to = [ OptionalOwner => Page::class ]
b) Page.has_many = [ SomeLinks => Links::class ] -- Link.has_one = [ Owner => Page::class ]

In practice a) Link.belongs_to won't get filled in, though that's fine

This means for the Page.has_one's we don't need Link.OwnerID/OwnerClass/OwnerRelation filled in

Let me know if I've understood this correctly

[GuySartorelli]

Closing in favor of #127 and silverstripe/silverstripe-framework#11084