CMSEditLinkExtension doesn't work with Hierarchy

[tiller1010]

Original context

We have a class that uses both the Hierarchy and CMSEditLinkExtension extensions. After the ParenID is changed, and the ContentLayout is saved, I get the error: `[Emergency] Uncaught LogicException: Could not produce an edit link for the passed object.`

It doesn't look like there is any support for DataObjects in a Hierarchy in the CMSEditLinkExtension. Instead, it looks like it only checks for has_many/has_one relationships: https://github.com/silverstripe/silverstripe-admin/blob/2/code/CMSEditLinkExtension.php#L62

class ContentLayout extends DataObject implements CMSPreviewable
{
  private static $cms_edit_owner = 'Parent';

  private static $extensions = [
    Hierarchy::class,
    CMSEditLinkExtension::class,
  ];

  /*
    CMSEditLink
    Returns the cms edit link for the content layout object
  */
  public function CMSEditLink()
  {
    // Get the value returned by the CMSEditLinkExtension
    return $this->extend('CMSEditLink')[0];
  }

  public function getCMSFields()
  {
    ...
    // Parent Container
    $Parent = TreeDropdownField::create('ParentID', 'Parent', ContentLayout::class)
      ->setEmptyString('Top Level');
    ...
  }
}

If a class has the Hierarchy extension it cannot rely on the Parent relation as the owner relation, because there is no corresponding has_many relation. Hierarchy relies on its own methods to get the related children records.

Developers could choose either of Children() or AllChildren() to put in the GridField so we'd need to check if either of these are editable, and if so return the link for it. They could also implement their own method to link back to the child records, though that would be out of scope.

Note that we also need to validate that the parent record and the child record both share the same hierarchical base class before getting a hierarchical link - otherwise you could have a has_one from MyRandomDataObject to SiteTree, and accidentally try to give a hierarchical link even though they don't share a hierarchical base class.

There's also no way to define what the edit owner is for the root level item in the hierarchy.

e.g:

some admin section
└─ root record
       └─ child record
               └─ grandchild record

The grandchild checks its parent and finds the child. The child checks its parent and finds the root. But the root will try to check its parent and find nothing...

[GuySartorelli]

Can you please provide some more information about how the parent provides the edit link for this model? i.e. what is the modeladmin, gridfield, or other setup you're using in order to manage the ContentLayout records?

[tiller1010]

@GuySartorelli ContentLayouts are managed by Pages, which has_many of them. We have other models like Articles that also have many ContentLayouts, and these can be managed by either a BlogPage or the BlogAdmin (model admin)

Some ContentLayout sub classes also use gridfields to manage other ContentLayouts.

[GuySartorelli]

Ahhh, interesting. So you're using the Hierarchy extension to denote that this has a parent - but the parent is a SiteTree record? I've not seen that extension used across model types like that before, I'm not sure how well supported it is. I doubt there's any test coverage for that scenario

That said, if the Hierarchy extension works for that scenario, then we should indeed get CMSEditLinkExtension to work alongside it. I assume your pages have a GridField for managing the ContentLayout records?

Also, you said in your report:

Instead, it looks like it only checks for has_many/has_one relationships

But just now you said:

ContentLayouts are managed by Pages, which has_many of them

So... it should be looking for the has_many of the page, shouldn't it?
Anyway, I'll see if I can find some time this or next week to reproduce the bug and see what's going on.

[tiller1010]

I assume your pages have a GridField for managing the ContentLayout records?

Yes, that's correct.

Also, you said in your report:
Instead, it looks like it only checks for has_many/has_one relationships
But just now you said:
ContentLayouts are managed by Pages, which has_many of them
So... it should be looking for the has_many of the page, shouldn't it?

Yes, I think we need a dynamic cms_edit_owner property, as sometimes it is the Parent, other times it is the Page or an Article.

[tiller1010]

So you're using the Hierarchy extension to denote that this has a parent - but the parent is a SiteTree record?

There are PageID, ArticleID, and ParentID columns on ContentLayouts. Parent() only works for ContentLayouts that are not directly under a page/article:

• Page
  • Container ContentLayout that only can use Page()
    • Child ContentLayout who's Parent method returns the container above.

[GuySartorelli]

Ooohhhh.... so you're saying that CMSEditLinkExtension does work with Hierarchy, but it doesn't know to look for your other "parents" when there is no Parent page? Or.... I'm confused now lol. Can you please put a really clear "what is expected" vs "what is happening" with your various scenarios?

[tiller1010]

@GuySartorelli

What is expected

When I save a ContentLayout after changing the ParentID, I should not receive the error: "[Emergency] Uncaught LogicException: Could not produce an edit link for the passed object."

Instead of using the cms_edit_owner property, I've found that I can use the getCMSEditOwner method to dynamically return my ContentLayout's owner:

public function getCMSEditOwner()
{
  $owner = null;
  if ($this->PageID) {
    $owner = $this->Page();
  } else if ($this->ParentID) {
    $owner = $this->Parent();
  }
  $this->extend('updateCMSEditOwner', $owner);
  return $owner;
}

I think we need a getCMSEditOwnerRelation method to be added and used throughout the CMSEditLinkExtension code though, instead of $this->owner->config()->get('cms_edit_owner');.
By default this could look like:

public function getCMSEditOwnerRelation()
{
  return $this->owner->config()->get('cms_edit_owner');
}

But we could use it in our ContentLayout class like:

public function getCMSEditOwnerRelation()
{
  $owner = '';
  if ($this->PageID) {
    $owner = 'Page';
  } else if ($this->ParentID) {
    $owner = 'Parent';
  }
  $this->extend('updateCMSEditOwnerRelation', $owner);
  return $owner;
}

Then, the CMSEditLinkExtension's getCMSEditLinkForManagedDataObject method could be updated to work with Hierarchy, by passing in Children instead of has_many objects:

/**
 * Get the link for editing an object from the CMS edit form of this object.
 * @throws LogicException if a link cannot be established
 * e.g. if the object is not in a has_many relation or not edited inside a GridField.
 */
public function getCMSEditLinkForManagedDataObject(DataObject $obj, string $reciprocalRelation): string
{
    $fields = $this->owner->getCMSFields();
    if ($reciprocalRelation == 'Parent' && $this->owner->hasExtension(Hierarchy::class)) {
        $children = $this->owner->AllChildren()->toArray();
    } else {
        $children = $this->owner->hasMany(false);
    }
    $link = $this->getCMSEditLinkForRelation($children, $obj, $reciprocalRelation, $fields);
    if (!$link) {
        throw new LogicException('Could not produce an edit link for the passed object.');
    }
    return $link;
}

[GuySartorelli]

Ahh thank you for that context. Sounds like this is working exactly as expected, then. It's not meant to handle scenarios where different contexts can be used to manage a model. Overriding the getCMSEditOwner() method as you have identified there is the correct way to handle your use case.

[GuySartorelli]

I've updated the context to what it seems like the problem actually is - not that the new description only mentioned the Hierarchy class, and removes all of the context for your use case of having two different parent relations. Can you please let me know if this new description appropriately captures the problem with using Hierarchy?

I have had a closer look at this and can see that Hierarchy is indeed not covered by this extension - but it will require adding new API to handle fully, which would not be addable in a patch release. It will need to be done as a feature in CMS 5.