NEW Add an extension to dynamically generate edit URLs #1361

GuySartorelli · 2022-09-13T04:11:34Z

Currently getting an edit link for an object is a bit of a pain - you have to generate a new link based on the predictable gridfield or other admin edit paths. This is partly improved by #1360 - but that still doesn't resolve the problem for nested setups (e.g. object in a page's gridfield).

NOTE

CI won't go green until #1360 is merged in as the new tests rely on ModelAdmin having a getEditLinkForManagedDataObject() method

Related issue

ModelAdmin is a little difficult to work with #1354

code/HasCanonicalEditLink.php

michalkleiner · 2022-09-14T05:43:03Z

code/HasCanonicalEditLink.php

+    /**
+     * Get a link to edit this page in the CMS.
+     */
+    public function CMSEditLink(): string


File already has CMSEditLink method, would this be a problem when applied to e.g. Image or other class extending File?

I can't think of literally any reason someone would do that - but if they did, I'm pretty sure the trait implementation would take precedence.

michalkleiner · 2022-09-14T05:45:24Z

code/HasCanonicalEditLink.php

+ * If the canonical edit owner is a has_one relation, the class on the other end
+ * of the relation must have a CMSEditLink() method.
+ */
+trait HasCanonicalEditLink


I was out for a bit and may have missed it, but do we have a naming convention for traits?

I'm a proponent of suffixing the name with the type of class/construct it is, e.g. SomethingTrait, PageController, FieldExtension etc. so it's immediately obvious what it is.

Suggested change

trait HasCanonicalEditLink

trait CanonicalEditLinkTrait

We do have quite a few traits already - some are suffixed with Trait but the majority are not.
I think the fact that you declare it as a trait, and you use it, make it abundantly clear it's a trait.

Extension and Controller are a bit different because those aren't built-in language structure things like a trait (we don't say SiteTreeClass after all).

Agree we shouldn't suffix Trait on this one (even though I'd actually like it, I also like the Interface suffix), because most of the code base doesn't suffix it

I don't like the use of the word Canonical at all - it really should just be CMSEditLink

I had a look at different traits in the codebase to find a common naming convention, there isn't that much in the way of consistency.

The most common pattern I could find was just calling the trait the feature it implemented e.g.

trait ImageManiputlation

trait FlushScheduler

trait FormMessage

The next most consistent, though less common one was the Configurable / Extensible / Injectable - though those are less "concrete" concepts than just the other traits that just bolt a feature on

This makes a couple of obvious candidates

trait CMSEditLink

trait CMSEditLinkable (weird IMO)

I think CMSEditLink is the most non-horrible of the two

class MyClass { use CMSEditLink;

And I get a CMSEditLink() method

I'll rename the trait to CMSEditLink

michalkleiner · 2022-09-14T05:49:44Z

I remember having issues not being able to get an edit link to objects in past so this would be a welcome addition, however I'm not sure if it should be a trait or somehow integrated more closely with DataObject itself or via an automatically applied extension to DataObject etc. What is the benefit of using the trait over an extension? Is this opt-in or would it be automatically applied?

GuySartorelli · 2022-09-14T05:56:52Z

What is the benefit of using the trait over an extension?

Less (if any) overhead is the main benefit - but mostly it just didn't really occur to me to make this as an extension.

Is this opt-in or would it be automatically applied?

It has to be opt-in. There's no way to know the canonical parent of some arbitrary model. How would we differentiate between the 12 has_one relations and the 3 model admins that all have edit links for some record?
These numbers are arbitrary but that's kinda my point. There's no way for us to determine, at a product-level, what some arbitrary project's model's canonical edit context is.

GuySartorelli · 2022-09-16T00:47:15Z

I've renamed the trait to CMSEditLink and renamed the getEditLinkForObject() method to getEditLinkForDataObject() to match the change in #1360

GuySartorelli · 2022-09-19T22:36:11Z

Steve and I have talked about this offline - I'll be converting this into an Extension and renaming getEditLinkForDataObject() to getEditLinkForManagedDataObject() so it's more intuitive.

GuySartorelli · 2022-09-26T22:25:31Z

code/ModelAdmin.php

            if (is_numeric($k)) {
                $models[$v] = ['dataClass' => $v, 'title' => singleton($v)->i18n_plural_name()];
                unset($models[$k]);
+            // Custom tab url segment but no custom title
+            } elseif (is_a($v, DataObject::class, true)) {
+                $models[$k] = ['dataClass' => $v, 'title' => singleton($v)->i18n_plural_name()];


This fix was needed for tests to go green.

emteknetnz

Mostly naming stuff

Key thing that we missed when we merged #1360 was the method name ModelAdmin::getEditLinkForManagedDataObject - it should be getCMSEditLinkForManagedDataObject for consistency with the rest of the code base

A quick string search

vendor/silverstripe/**/*.php

"CMSEditLink(" - 33 results
"EditLink(" - 37 results

Of the "EditLink(" that weren't "CMSEditLink(":
"updateCMSEditLink(" - 1 result
"getFileEditLink(" - 3 results

So you'll also need to rename https://github.com/silverstripe/silverstripe-admin/blob/1/code/ModelAdmin.php#L230 in this PR as well

The other thing is the word "Canonical" is very confusing. Rename this to "CMSEdit" so that it's standard with everything else

I've left a bunch of recommended edits, though there's a large number of things that will need renaming e.g. the test files, docs, etc

emteknetnz · 2022-09-29T21:04:17Z

code/CMSEditLinkExtension.php

+            && $field->getConfig()->getComponentByType(GridFieldDetailForm::class);
+    }
+
+    private function constructLink(string $relation, int $id): string


Suggested change

private function constructLink(string $relation, int $id): string

private function constructCMSEditLink(string $relation, int $id): string

emteknetnz · 2022-09-29T21:04:50Z

code/CMSEditLinkExtension.php

+        return Director::absoluteURL($relativeLink);
+    }
+
+    private function getLinkForRelation(array $componentConfig, DataObject $obj, string $reciprocalRelation, FieldList $fields): string


Suggested change

private function getLinkForRelation(array $componentConfig, DataObject $obj, string $reciprocalRelation, FieldList $fields): string

private function getCMSEditLinkForRelation(array $componentConfig, DataObject $obj, string $reciprocalRelation, FieldList $fields): string

emteknetnz · 2022-09-29T21:05:11Z

code/CMSEditLinkExtension.php

+     * @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 getEditLinkForManagedDataObject(DataObject $obj, string $reciprocalRelation): string


Suggested change

public function getEditLinkForManagedDataObject(DataObject $obj, string $reciprocalRelation): string

public function getCMSEditLinkForManagedDataObject(DataObject $obj, string $reciprocalRelation): string

emteknetnz · 2022-09-29T21:15:23Z

code/CMSEditLinkExtension.php

+ */
+class CMSEditLinkExtension extends Extension
+{
+    private static string $canonical_edit_owner = '';


Suggested change

private static string $canonical_edit_owner = '';

private static string $cms_edit_owner = '';

emteknetnz · 2022-09-29T21:15:32Z

code/CMSEditLinkExtension.php

+     *
+     * @return LeftAndMain|DataObject|null
+     */
+    public function getCanonicalEditOwner()


Suggested change

public function getCanonicalEditOwner()

public function getCMSEditOwner()

emteknetnz · 2022-09-29T21:16:06Z

code/CMSEditLinkExtension.php

+     */
+    public function CMSEditLink(): string
+    {
+        $owner = $this->owner->getCanonicalEditOwner();


Suggested change

$owner = $this->owner->getCanonicalEditOwner();

$owner = $this->owner->getCMSEditOwner();

emteknetnz · 2022-09-29T21:16:18Z

code/CMSEditLinkExtension.php

+            return '';
+        }
+
+        if (!$owner->hasMethod('getEditLinkForManagedDataObject')) {


Suggested change

if (!$owner->hasMethod('getEditLinkForManagedDataObject')) {

if (!$owner->hasMethod('getCMSEditLinkForManagedDataObject')) {

emteknetnz · 2022-09-29T21:16:24Z

code/CMSEditLinkExtension.php

+        }
+
+        if (!$owner->hasMethod('getEditLinkForManagedDataObject')) {
+            throw new LogicException('The canonical owner must implement getEditLinkForManagedDataObject()');


Suggested change

throw new LogicException('The canonical owner must implement getEditLinkForManagedDataObject()');

throw new LogicException('The canonical owner must implement getCMSEditLinkForManagedDataObject()');

emteknetnz · 2022-09-29T21:16:32Z

code/CMSEditLinkExtension.php

+        if ($owner instanceof DataObject) {
+            $relativeLink = $owner->getEditLinkForManagedDataObject($this->owner, $this->owner->config()->get('canonical_edit_owner'));
+        } else {
+            $relativeLink = $owner->getEditLinkForManagedDataObject($this->owner);


Suggested change

$relativeLink = $owner->getEditLinkForManagedDataObject($this->owner);

$relativeLink = $owner->getCMSEditLinkForManagedDataObject($this->owner);

emteknetnz · 2022-09-29T21:16:49Z

code/CMSEditLinkExtension.php

+
+    private function constructLink(string $relation, int $id): string
+    {
+        $ownerType = $this->owner->config()->get('canonical_edit_owner');


Suggested change

$ownerType = $this->owner->config()->get('canonical_edit_owner');

$ownerType = $this->owner->config()->get('cms_edit_owner');

GuySartorelli · 2022-09-29T21:39:22Z

The other thing is the word "Canonical" is very confusing. Rename this to "CMSEdit" so that it's standard with everything else

"canonical" is correct in this context - e.g. the <link rel="canonical" href="https://www.example.com/page/" /> meta tag tells search engines what the canonical URL is for a page, in case the page can be accessed via multiple URLs.

But I'll make that change to avoid peer review ping pong and to match the other changes where "canonical" was already changed.

emteknetnz · 2022-09-29T22:59:57Z

code/CMSEditLinkExtension.php

+    }
+
+    /**
+     * Get a link to edit this page in the CMS.


Suggested change

* Get a link to edit this page in the CMS.

* Get a link to edit this DataObject in the CMS.

emteknetnz · 2022-09-29T23:03:06Z

code/CMSEditLinkExtension.php

+ *
+ * Note that the cms edit owner must implement a getCMSEditLinkForManagedDataObject() method.
+ *
+ * If the cms edit owner is a has_one relation, the class on the other end


Suggested change

* If the cms edit owner is a has_one relation, the class on the other end

* If the cms_edit_owner is a has_one relation, the class on the other end

emteknetnz · 2022-09-29T23:03:57Z

code/CMSEditLinkExtension.php

+    private static string $cms_edit_owner = '';
+
+    /**
+     * Get the admin or DataObject which owns this object for CMS editing purposes.


Suggested change

* Get the admin or DataObject which owns this object for CMS editing purposes.

* Get the ModelAdmin, LeftAndMain or DataObject which owns this object for CMS editing purposes.

ModelAdmin is a LeftAndMain - and they're all admin. I'll make this change but it's super subjective.

code/CMSEditLinkExtension.php

emteknetnz · 2022-09-29T23:48:50Z

tests/php/CMSEditLinkExtensionTest/BelongsToModelAdmin.php

+use SilverStripe\Dev\TestOnly;
+use SilverStripe\ORM\DataObject;
+
+class BelongsToModelAdmin extends DataObject implements TestOnly


Could you please rename this, it's not a "belongs_to" relationship, which is defined as the reciprocal of a has_one relationship when it's a 1-1 relations.

Also, it being suffixed with "ModelAdmin" confused me for a long time because I mistakenly thought it extended ModelAdmin

Even just calling this TestDataObject would be an improvement

That wouldn't be an improvement, it's a lot less descriptive. I'll rename it to something that doesn't contain "belongsto" or end with "modeladmin". But given this is a test object I don't see that it matters all that much what it's called.

emteknetnz · 2022-09-29T23:56:02Z

tests/php/CMSEditLinkExtensionTest/BelongsToModelAdmin.php

+        'Nested' => NestedObject::class . '.Parent',
+        'AnotherArbitraryRelation' => NestedObject::class . '.AnotherOfTheSameClass',
+        'BasicNested' => BasicNestedObject::class,
+        'PolyMorphic' => PolymorphicNestedObject::class,


Suggested change

'PolyMorphic' => PolymorphicNestedObject::class,

'Polymorphic' => PolymorphicNestedObject::class,

This was referenced Sep 13, 2022

ModelAdmin is a little difficult to work with #1354

Closed

ENH Use the new CMSEditLink extension on SiteTree silverstripe/silverstripe-cms#2778

Merged

GuySartorelli force-pushed the pulls/1/cmslink-trait branch 2 times, most recently from 62cf84c to 1431dee Compare September 13, 2022 05:29

GuySartorelli mentioned this pull request Sep 13, 2022

DOC Document new CMSEditLink extension. silverstripe/developer-docs#85

Merged

michalkleiner reviewed Sep 14, 2022

View reviewed changes

code/HasCanonicalEditLink.php Outdated Show resolved Hide resolved

michalkleiner reviewed Sep 14, 2022

View reviewed changes

GuySartorelli force-pushed the pulls/1/cmslink-trait branch from 1431dee to abc3fe5 Compare September 14, 2022 05:48

GuySartorelli force-pushed the pulls/1/cmslink-trait branch from abc3fe5 to 9fbb42f Compare September 16, 2022 00:55

GuySartorelli mentioned this pull request Sep 19, 2022

NEW New API for managing models in ModelAdmin #1360

Merged

GuySartorelli force-pushed the pulls/1/cmslink-trait branch from 9fbb42f to a153fad Compare September 19, 2022 22:57

GuySartorelli changed the title ~~NEW Add a trait to dynamically generate edit URLs~~ NEW Add a extension to dynamically generate edit URLs Sep 19, 2022

GuySartorelli force-pushed the pulls/1/cmslink-trait branch from a153fad to ac803c2 Compare September 19, 2022 23:22

GuySartorelli changed the title ~~NEW Add a extension to dynamically generate edit URLs~~ NEW Add an extension to dynamically generate edit URLs Sep 19, 2022

GuySartorelli force-pushed the pulls/1/cmslink-trait branch 2 times, most recently from fb1a370 to 9cedf3d Compare September 26, 2022 22:24

GuySartorelli commented Sep 26, 2022

View reviewed changes

FIX Always add dataClass to models spec

fd91da3

GuySartorelli force-pushed the pulls/1/cmslink-trait branch 2 times, most recently from ab2b181 to fafd681 Compare September 26, 2022 22:35

emteknetnz requested changes Sep 29, 2022

View reviewed changes

GuySartorelli force-pushed the pulls/1/cmslink-trait branch 2 times, most recently from 3d9d6a4 to 27c65cd Compare September 29, 2022 22:05

emteknetnz reviewed Sep 29, 2022

View reviewed changes

GuySartorelli force-pushed the pulls/1/cmslink-trait branch from 27c65cd to 052ed99 Compare September 29, 2022 23:01

emteknetnz reviewed Sep 29, 2022

View reviewed changes

code/CMSEditLinkExtension.php Show resolved Hide resolved

GuySartorelli force-pushed the pulls/1/cmslink-trait branch 2 times, most recently from 43affee to 767db75 Compare September 29, 2022 23:07

emteknetnz requested changes Sep 30, 2022

View reviewed changes

NEW Add an extension to dynamically generate edit URLs

b525ae2

GuySartorelli force-pushed the pulls/1/cmslink-trait branch from 767db75 to b525ae2 Compare September 30, 2022 00:08

emteknetnz approved these changes Sep 30, 2022

View reviewed changes

emteknetnz merged commit 769bbab into silverstripe:1 Sep 30, 2022

emteknetnz deleted the pulls/1/cmslink-trait branch September 30, 2022 01:23

	private function constructLink(string $relation, int $id): string
	private function constructCMSEditLink(string $relation, int $id): string

	private function getLinkForRelation(array $componentConfig, DataObject $obj, string $reciprocalRelation, FieldList $fields): string
	private function getCMSEditLinkForRelation(array $componentConfig, DataObject $obj, string $reciprocalRelation, FieldList $fields): string

	public function getEditLinkForManagedDataObject(DataObject $obj, string $reciprocalRelation): string
	public function getCMSEditLinkForManagedDataObject(DataObject $obj, string $reciprocalRelation): string

	private static string $canonical_edit_owner = '';
	private static string $cms_edit_owner = '';

	public function getCanonicalEditOwner()
	public function getCMSEditOwner()

	$owner = $this->owner->getCanonicalEditOwner();
	$owner = $this->owner->getCMSEditOwner();

	if (!$owner->hasMethod('getEditLinkForManagedDataObject')) {
	if (!$owner->hasMethod('getCMSEditLinkForManagedDataObject')) {

	throw new LogicException('The canonical owner must implement getEditLinkForManagedDataObject()');
	throw new LogicException('The canonical owner must implement getCMSEditLinkForManagedDataObject()');

	$relativeLink = $owner->getEditLinkForManagedDataObject($this->owner);
	$relativeLink = $owner->getCMSEditLinkForManagedDataObject($this->owner);

	$ownerType = $this->owner->config()->get('canonical_edit_owner');
	$ownerType = $this->owner->config()->get('cms_edit_owner');

	* Get a link to edit this page in the CMS.
	* Get a link to edit this DataObject in the CMS.

	* If the cms edit owner is a has_one relation, the class on the other end
	* If the cms_edit_owner is a has_one relation, the class on the other end

	* Get the admin or DataObject which owns this object for CMS editing purposes.
	* Get the ModelAdmin, LeftAndMain or DataObject which owns this object for CMS editing purposes.

	'PolyMorphic' => PolymorphicNestedObject::class,
	'Polymorphic' => PolymorphicNestedObject::class,

NEW Add an extension to dynamically generate edit URLs #1361

NEW Add an extension to dynamically generate edit URLs #1361

Conversation

GuySartorelli commented Sep 13, 2022 • edited Loading

NOTE

Related issue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalkleiner commented Sep 14, 2022

GuySartorelli commented Sep 14, 2022 • edited Loading

GuySartorelli commented Sep 16, 2022

GuySartorelli commented Sep 19, 2022

Choose a reason for hiding this comment

emteknetnz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli commented Sep 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emteknetnz Sep 29, 2022 • edited Loading

Choose a reason for hiding this comment

GuySartorelli Sep 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli commented Sep 13, 2022 •

edited

Loading

GuySartorelli commented Sep 14, 2022 •

edited

Loading

emteknetnz Sep 29, 2022 •

edited

Loading

GuySartorelli Sep 30, 2022 •

edited

Loading