-
Notifications
You must be signed in to change notification settings - Fork 15
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
Reverse ownership #5
Comments
Added an alternative solution. |
No input as to the issue itself, just o note on the story language — 'sensible' can be quite ambiguous and it's not clear (to me) how to quantify that and what it should express/mean. |
I had a glance at Guy's solution. I didn't think it was obviously wrong or horrible ... although it seems like a lot of effort to accommodate a rather simple use case. In the spirit of iteration, what would happen if we just declared that only one link list can be tied to one record? We could wait to see if anyone actually has a use case for having two or more link lists. Could support for multiple Link List on one DataObject retroactively added later on? |
No need to wait, I had that exact use case multiple times when I was working at signify (different link module, but same use case)
Arguably yes but adding a relation field like this after the fact would be hard to retrofit, since the data won't be there to filter against. The better option if you don't want to implement this right off is to just have a simpler permission model, remove the has_one, and accept that we never have access to the 'owner' from the link side of the relationship. |
Isn't the difficulty with the can*() checks only an issue for the has_one's on a Page/DataObject? Seems like what Max is talking about is a separate issue which is support for multiple has_many's on a Page/DataObject. I don't think there's any issue finding the owner to do can*() checks with the has_many relationships I'd agree with Guy that we should solve the multiple has_manys now. I don't think Guy's solution was particularly much effort, it's really just an extra column added next to a couple of existing columns. |
It's also a problem for multiple has_many relationships if you don't use a workaround to have all has_many relationships use the same has_one. I'd be strongly against saying we can't support multiple has_many, so we either have to point them all at the same has_one or not have the can* rely on the owner. |
I'm a bit confused - on Max's has_many implementation there was already Link.has_one = [ Owner => DataObject::class ] - which will add the columns OwnerClass + OwnerID to the Link database table - that's enough for you to get the owner easily? Your PR adds OwnerRelation so that we can assign Links to the correct Relation, though I'm not sure why that's necessary to get the Owner specifically? |
We have decided to proceed with option 2, pending the creation of a version of the linkfield that can handle has_many relationships. This card is blocked until that has been implemented. |
Linked PRs have been merged |
Story
As a developer I want Link objects to have a sensible ownership model so that Link objects behave in a sensible way when versioned operation are performed.
Acceptance criteria
Note
The links belongs to a single owner relationship.
POCs
Option one:
NEW Linkfield ownership #101rejected, use option 2Option two:
NOTE: This option could be mostly implemented in framework if we have some new way to define it (e.g.
has_one_special
) - that's necessary so we know when we should have and use the new dbfield which existing has_one relations won't have.INSTALLER CI RUN
https://github.com/creative-commoners/silverstripe-installer/actions/runs/7160809301
Note the
SilverStripe\CMS\Tests\Model\SiteTreeTest::testRelativeLink
failure is pre-existing. See silverstripe/.github#169PRs
The text was updated successfully, but these errors were encountered: