silverstripe / silverstripe-framework

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

Orphan polymorphic relations get assigned to random DataObject class #11165

Open maxime-rainville opened 4 months ago

maxime-rainville commented 4 months ago

Module version(s) affected

5.x-dev ... but probably every version of 5

Description

Orphan Polymorphic has_one get attached to the first DataObject class available.

How to reproduce

  1. Run the scrip below.

Expected results: PolyHasOne comes back null Actual results: An unsaved Bar object is returned. (Can be a different DataObject ... dependes what the first DataObject to be discovered is).

<?php

use SilverStripe\ORM\DataObject;
use SilverStripe\Dev\BuildTask;

class Bar extends DataObject {

    private static array $has_one = [
        'PolyHasOne' => DataObject::class,
        'RegularHasOne' => Page::class
    ];
}

class MultiRelation extends BuildTask
{
    public function run($request)
    {
        echo "Creating a Foo object with a Bar object\n";
        $bar = Bar::create();
        $bar->write();
        $bar = Bar::get()->byID($bar->ID);
        echo $bar->debug();
        echo $bar->PolyHasOne->debug();
        echo $bar->RegularHasOne->debug();
    }
}

Possible Solution

No response

Additional Context

I think this happens because the Class column for polymorphic relations are enum and default to whatever the first DataObject class in the enum is.

I'm not actually sure to what degree this is a problem. It's definitely confusing. But what the expected behaviour should be in this situation is also not 100% clear to me.

Related card

GuySartorelli commented 4 months ago

Expected results: PolyHasOne comes back null

That's not how has_one relations work usually - regular (non-polymorphic) has_one relations give you a singleton of the relevant class. There is a lot of code out there that assumes a dummy record like this will be provided when requesting a non-existent has_one relation so we can't just return null all of a sudden.

i.e. there's a lot of if ($this->MyHasOne()->exists()) {/* do something */} in the wild which would result in exceptions being thrown if we made that change.

GuySartorelli commented 4 months ago

One solution is to always return null for any missing has_one relation in a future major release, which I'd be in favour of.

maxime-rainville commented 4 months ago

I agree that's not clear what the correct outcome should be here. The very confusing thing here is that you'll get a completely random DataObject class on your Polymoprhic has one.

On the related question of what should you get when trying to access an has_one without a value, I think returning null would make more sense. I guess it's a bit better that you are at least getting an instance of the relation's Class.

I'm inclined to call this a CMS6 problem for now.