BUGFIX: many_many through should allow subclasses

[unclecheese]

class HomePage extends Page
{
    private static $many_many = [
        'HeroImages' => [
            'through' => PageImageLink::class,
            'from' => 'Page',
            'to' => 'Image',
        ]
    ];

}

class PageImageLink extends DataObject
{
    private static $has_one = [
        'Page' => SiteTree::class,
        'Image' => Image::class,
    ];
}

This fails because the linking object's relation class doesn't exactly match the owner. Sharing the linking objects across various entries in the ancestry should be a supported use case.

[robbieaverill]

LGTM. @tractorcow any thoughts?

[robbieaverill]

Be aware that origin branches create entries in Packagist. I've removed it manually.

[tractorcow]

If those image links are specific to home pages, the mapping table have a has_one to the homepage.
If it's not specific to home pages, then your code should be in Page.php, or an extension on SiteTree.
Thus, I don't think the example given is a good example of why this should be allowed.

This pull request seems to be explicitly allowing the developer to write invalid relationships. Those invalid relationships often are the result of mis-alignment of types, that more often than not should be resolved by fixing, rather than having type leniency. It opens the doors to buggy code being caught in production rather than in development.

[tractorcow]

If this should be allowed, please provide a better example that demonstrates the need.

[tractorcow]

If you just need polymorphic relationships, you can use DataObject in the from relationship type. This is a supported feature, but should still be used with care.

class PageImageLink extends DataObject
{
    private static $has_one = [
        'Page' => DataObject::class,
        'Image' => Image::class,
    ];
}

I don't think this case is a valid call for it, but just putting it out there.

[unclecheese]

I understand @tractorcow 's points here. It's true that we could solve this by simply chucking it in Page, but I think too often developers are encouraged to do that as a sledgehammer solution to a surgical problem. Whenever we feel the least bit like functionality might be shared across more than one page type, we put it in Page, and this just leads to bloat, and I think we should always try to solve these things horizontally if we can.

Linking objects are a bit of a unique problem because you have to create a whole new class for every many_many relation you want to support, so my thinking here was to keep that object reusable if possible, and let the parent class opt into the relationship rather than have it grandfathered in whether it's an actual concern of the page or not.

Otherwise, you end up with a page class that has relationships that are only editable on a subset of subclasses, and templates that inherit relationships that will never be defined, and all of that that just feels weird and untidy to me.

The polymorphic linking object is a good enough solution for me if none of my thinking here is winning anyone over.

[tractorcow]

It sounds like polymorphic is your best bet if it's a re-usable component. Look at FileLink if you need an example from core.

<?php

namespace SilverStripe\Assets\Shortcodes;

use SilverStripe\Assets\File;
use SilverStripe\ORM\DataObject;

/**
 * Represents a link between a dataobject parent and a file shortcode in a HTML content area
 *
 * @method DataObject Parent() Parent object
 * @method File Linked() File being linked to
 */
class FileLink extends DataObject
{
    private static $table_name = 'FileLink';

    private static $owns = [
        'Linked',
    ];

    private static $owned_by = [
        'Parent',
    ];

    private static $has_one = [
        'Parent' => DataObject::class,
        'Linked' => File::class,
    ];
}

[unclecheese]

Yeah I'm fine with that, although it still feels suboptimal because:

• I only want pages to have images, not all DataObjects
• I did what I think most devs would do naturally and it blew up

I'm not convinced the costs of implementing this patch are greater than the offset of those two things, and would welcome anyone else to weigh their input.

[tractorcow]

You could add it as an extension to SiteTree, instead of a subclass. That's even less for devs to have to do right? :P Assuming they install your module, and their sitetrees automatically get those links enabled. You could just, ignore it for pages you don't want to use it on I guess?

[GuySartorelli]

I don't see what's different in the example from @tractorcow in this comment and the one that @unclecheese provided in the original comment other than @tractorcow's example is a much looser polymorphic relationship?

I don't see why narrowing down to a more-specific base class should be invalid - certainly the docs about polymorphic many_many through relationships doesn't mention that the class on the polymorphic side of the relationship has to be DataObject specifically, and being able to say "This could be any SiteTree but not other types of DataObject" seems like it could be very useful without forcing anyone that uses that model and relation table to subscribe to placing the relation and any corresponding logic into Page (which might be too far upstream for some applications) or an Extension (which can easily get out of hand - I prefer to keep my logic in the specific classes that need them rather than having devs try to figure out which extension is controlling the thing they've been tasked with modifying).

[GuySartorelli]

Rebased and added some test coverage. If this PR was created today I would have merged it - I agree with my comments from 2021 and with UncleCheese that this should be a supported use case.

[GuySartorelli]

The push CI is failing because CI isn't set up to handle it... I'm gonna recreate this PR using a fork.