Initial review of linkfield

[maxime-rainville]

The LinkField never went through a full peer review process.

Acceptance criteria

• A top down review of LinkField is performed.
• Potential problems or architectural decisions are highlighted.
• Questions that require further discussion are raised.
• Feature gaps are identified.
• Existing documentation is reviewed and gaps are called out.
• Test coverage and CI builds are reviewed.

Related

• AnyField may be able to answer some of the questions raised by this card, but will be handle separately.

[emteknetnz]

Link to my first look at the module which includes a list of maintenance tasks to perform

Here I've had done a deeper look at the code. I've assumed that project implementations are done via adding the $has_one version of linkfield to a Page dataobject.

Feature gaps

Permission checks

• Link::canView() + other can*() methods are missing - though this doesn't really seem like an issue .. since they're saved as as part of page save process they essentially get just get the same permissions as the page they're on
• If a project really needs can*() checks on links independent of the page they're on (e.g. manage links on a gridfield) the project can just subclass Link or one of the existing Link implementations and add a custom canView()/canEdit()/etc checks there.
• Need to add a couple of canView() permission checks inside the Link subclass generateLinkDescription() methods where they use DataLists.

Validation

• RequiredFields validation doesn't work Link as Required Field doesn't appear to work #69
• This is seems fairly easy to fix, for example just add this to PhoneLink and it now there's frontend validation:

    public function getCMSCompositeValidator(): CompositeValidator
    {
        $validator = parent::getCMSCompositeValidator();
        $validator->addValidator(RequiredFields::create([
            'Phone',
        ]));
        return $validator;
    }

Versioning

• No versioning Versioned by default #7 i.e. no _Live tables for links
• Draft changes on links goes live as e.g. You change a phone number, click page save (not publish). The phone number will go straight to live. Fix with versioning. This is pretty easy to implement by simply:
  • add private static array $extensions = [Versioned::class]; to Link
  • assuming private static array $has_one = ['HasOneLink' => Link::class]; on Page, add private static array $owns = ['HasOneLink']; and also the same for $cascade_deletes and $cadcade duplicates.
• This does triple the number of database tables from 6 to 18

Multiple links support in a single field

• Missing has_many support LinkField support for has_many #44.
• Also missing many_many, though that adds so much complexity I'd like to avoid this and simply say that has_many is "good enough"

Internal slack convo

Internal slack conversation asking developers about what they perceive as feature gaps - https://silverstripeltd.slack.com/archives/CLXKD9X51/p1695690436734529. tl;dr:

• Docs are poor
• Validation a bit broken
• Ability to have multiple links
• Make it easy to customise - docs showing you how and/or extension points
• Make it easier to add custom attributes / class - current template makes it hard
• Add ability to specify link types at a per field level, rather than just globally
• Lack of test coverage
• Make it look/behave more like TinyMCE plugin

Code architecture

PHP

• ORM/DBJson.php / ORM/DBLink.php and friends will be deleted - see notes below in "Notes about the 2x implementations"
• There's a stack of extension (AjaxField/FormFactoryExtension/LeftAndMain/ModalController) in the 'Extensions' directory that are a bit messy which support a 'DynamicLink' used to dynamically provide a JSON form schema. On AjaxField there's a comment "There's probably a less dumb way of doing this". This could use a tidy up. LeftAndMain needs to be renamed to LeftAndMainExtension :-)
• The Registry class and _config/types.yml is used to define the link types, though it's unnecessary we can just look for subclasses of Link::class. _config/types.yml does allow you to se enabled: false to globally disable link types, though a better way would be to just have a private static $enabled on Link and just config it off on the subclasses you don't want. Also I'd much rather just have LinkField.php::setAllowTypes()/setDenyTypes() methods directly on the LinkField to specific allowed link types in a given context
• JsonData and Type interface are both only used on the Link class, which is a base class that is subclassed. Just get rid of the interfaces since they're pointless abstraction.
• The module converts form data to a JSON in LinkField.js::stringifyData() before submitting as a single field via POST on the page edit form, where it's then decoded on the server and turned into a related dataobject. It's kind of like doing a REST API call done via POST instead of XHR.

Javascript

• Uses functional react components and hooks, so that's good
• Has some redux - FileLinkModal.js calls the INIT_FORM_SCHEMA_STACK action which presumably does something in asset-admin
• React component are loaded on to the HTML via entwine/JsonField.js
• uses graphql/apollo for a pair of simple of readonly operations - get a list of link types and descriptions on individual links. The queries live in the 'state' directory which is kind of confusing cos it's not application state. graphql/apollo just adds a lot of unnecessary complexity which makes ongoing maintenance and development hard. It could easily be replaced with a little PHP controller and the JS fetch() method
• eslint is probably a bit out of date e.g. still has <Fragment> in there

CSS

• A little bit of CSS which uses BEM

Documentation

• Readme seems very out of date and could lots of work particularly around "Quickstart sample code" e.g provide better examples of using it on a template, including versioning, etc
• Needs dev docs Developer documentation #27
• Users docs ... (assuming they stay in their current form) User documentation #26

Test coverage (phpunit, jest, behat)

Unit testing

• PHPunit is a bit light, though it is a fairly simple module. It's missing a test for setData() though otherwise it seems generally OK, could be improved
• No Jest tests
• No behat - could use a happy day scenario
• Has story book for LinkPicker.js

CI

• CI is currently green

Maintenance

• See Standardise module #78 for a list of things to do for module standardisation

Notes about the 2x implementations

DBLink:

Faster database performance, though uses controversial JSON data in db field - makes finding data via DB queries much harder. We don't do this anywhere else in Silverstripe.

    private static array $db = [
        'DbLink' => DBLink::class
    ];

mysql> select * from Page where ID = 2;
+----+----------+-----------------------------------------------------------------------------------------------------+--------------+
| ID | MyFileID | DbLink                                                                                              | HasOneLinkID |
+----+----------+-----------------------------------------------------------------------------------------------------+--------------+
|  2 |        0 | {"Root":null,"Main":null,"Title":"My db link","PageID":3,"Anchor":"","OpenInNew":0,"typeKey":"cms"} |            1 |
+----+----------+-----------------------------------------------------------------------------------------------------+--------------+

Traditional has_one:

Slower database performance as it needs to join a bunch of tables together, though consistent with the rest of Silverstripe

    private static array $has_one = [
        'HasOneLink' => Link::class,
    ];

mysql> select * from LinkField_Link;
+----+--------------------------------------------+---------------------+---------------------+-----------------+-----------+
| ID | ClassName                                  | LastEdited          | Created             | Title           | OpenInNew |
+----+--------------------------------------------+---------------------+---------------------+-----------------+-----------+
|  1 | SilverStripe\LinkField\Models\SiteTreeLink | 2023-09-14 15:55:03 | 2023-09-14 15:55:03 | My Has one link |         0 |
+----+--------------------------------------------+---------------------+---------------------+-----------------+-----------+

mysql> select * from LinkField_SiteTreeLink;
+----+--------+--------+
| ID | Anchor | PageID |
+----+--------+--------+
|  1 | NULL   |      2 |
+----+--------+--------+

[emteknetnz]

Questions that require further discussion

• Elephant in the room is https://github.com/maxime-rainville/silverstripe-anyfield - what gaps above does that fill? It will be looked at here though would be good just to get a high level "this is what the motivation for doing Anyfield was" - presumably it was to fix some shortcomings of linkfield?
• Are philosophically willing to remove the graphql/apollo complexity in favour of a simple controller + fetch() for a better developer experience?
• Are we concerned about a large number of tables created as noted on RFC-30 / Data Model #30 (comment) "too many extra DB entities (aka tables) for every link type (multiplied by versioned and fluent modules)"?

[GuySartorelli]

I'll add some extra limitations here.
IF the whole point of this is that it's providing a standardised way for handling links across the board in Silverstripe CMS, then it should provide a TinyMCE plugin that overrides the ones currently provided by admin/cms/asset-admin which creates/uses Link records in the DB.
It should also provide reports for orphaned and for broken Link records
It should probably also provide either a BuildTask or a button in the above-mentioned report for deleting orphaned links.

[emteknetnz]

Broken links / TinyMCE are just making shortcodes in a Content field though right? They're not the same thing as the Link Dataobjects that this module does?

That said maybe a leftfield idea here is to save everything in linkfield as shortcodes instead of DB Objects, and require any custom Link subclasses to define their own shortcodes

[GuySartorelli]

Broken links / TinyMCE are just making shortcodes in a Content field though right? They're not the same thing as the Link Dataobjects that this module does?

Broken links are not shortcodes so I don't know what you meant by that part.

TinyMCE links are stored in the DBHtmlField as shortcodes, and I would recommend that happen with an implementation that this module provides, as well. The shortcode would look something like [link,id=1], and the shortcode parser would get the associated link record and render its template. You would then have a standardised model which handles links everywhere, whether it's in the WYSIWYG or in a GridField or in the AnyRelationField or whatever.

That said maybe a leftfield idea here is to save everything in linkfield as shortcodes instead of DB Objects, and require any custom Link subclasses to define their own shortcodes

Shortcodes don't actually store data, really. Look at the "page on this site" shortcode for example: <a href="[sitetree_link,id=1]">my page</a> all the shortcode itself stores is the ID for the page. The shortcode parser then fetches that page and replaces the shortcode with the URL for that page. You're still storing the URL in the SiteTree record. In the same way, I'm proposing we store all links in a Link record, and use shortcodes to say "this is where that link should go in the content"

[GuySartorelli]

Note that that would not be MVP - I'd recommend we look at doing that as an enhancement down the line rather than as part of the initial epic.

[emteknetnz]

Broken links are not shortcodes so I don't know what you meant by that part.

Ah yup you're right, sorry confused myself. From memory the broken links uses some pretty old and fragile code that at least partially uses SiteTreeLinkTracking to generate the report.

It should also provide reports for orphaned and for broken Link records

OK so you're saying there should be a central way to quickly work out which of all the internal/external Link records are broken?

[link,id=1]

So you could "Insert a link" in TinyMCE and select an email address (which you can do now), though you're saying that link would now get saved as a new DataObject and a shortcode inserted into the Content field, instead of what currently happens where it just gets turned straight into <a title="My description" href="mailto:address@example.com?subject=My%20subject">My link text</a>?

[GuySartorelli]

so you're saying there should be a central way to quickly work out which of all the internal/external Link records are broken?

Yeah. My understanding is that the whole point of this module is that there's a single standardised way to manage links in the CMS. The WYSIWYG should not be excluded from that goal IMO.

One very clear benefit being that a broken links report will "just work" - we wouldn't have to go through all HTML fields hunting for links, we can just do a Link::get(). We could then choose to remove the SiteTreeLinkTracking logic and change it to look for Link records - but we could also just add an edit link/button to the broken links report that opens the link modal, and fix the broken link right there without caring what page it's on.

Another advantage is that it makes it really easy to modify the way links display in a standardised way (e.g. adding an "external link" icon to all external links would be as simple as editing the link template).

So you could "Insert a link" in TinyMCE and select an email address (which you can do now), though you're saying that link would now get saved as a new DataObject and a shortcode inserted into the Content field

Yup that's exactly what I'm proposing.

[emteknetnz]

I've created another issue here #84 with a bunch of linked draft PRs that implement a bunch (though not all) of what I've discussed on this issue plus more

[maxime-rainville]

Link::canView() + other can*() methods are missing - though this doesn't really seem like an issue .. since they're saved as as part of page save process they essentially get just get the same permissions as the page they're on

Every DataObject should have proper permission check out of the box. Whether we tie the permissions to an explicit owner for the link or to some pre-existing CMS permission, I'm not sure.

But I don't consider the current status quo acceptable.

RequiredFields validation doesn't work Link as Required Field doesn't appear to work #69

We should aim to have more than just a black and white validation. The LinkField is designed to be extensible with the expectation that you could provide more complex Link Type. e.g. The external link should have validation to make sure that the provided URL is a valid URL.

The same way we use form schema to retrieve the form list, we could post back the data we got to see if it's valid.

Versioning

Do we want to versioned this by default? This introduces a dependency on versioned.

Would we consider conditionally versioning link only if versioned is installed?

There's also a related concern that if you are versioning Links, you need to make sure they get published when the parent record is published.

Multiple links support in a single field

Any field has some pretty solid support for "Has many" list. If we don't want to go all the way and jump on the AnyField band wagon right away, we can just copy its implementation.

I agree that ManyMany support is a bad idea. Links should be strongly tied to a parent ... the same way that an Elemental block is tightly coupled to an Elemental Area.

Internal slack convo

Add ability to specify link types at a per field level, rather than just globally

AnyField has that capability. Once you dump the registry idea and the need to register link classes up front, this becomes pretty easy.

Make it look/behave more like TinyMCE plugin

I like this idea. I could image a link shortcode that gets managed in a similar way to a link field.

However, this sounds more like a nice to have than something we should aim to support right away.

Code architecture

PHP

ORM/DBJson.php / ORM/DBLink.php and friends will be deleted - see notes below in "Notes about the 2x implementations"

Agree

AjaxField/FormFactoryExtension/LeftAndMain/ModalController

ModalController is equivalent to RemoteFileModalExtension

The LeftAndMain extension is just working around the fact that there's no "LinkAdmin" controller to host some endpoints. Asset-admin doesn't have that problem because it has its own controller.

FormFactory is something @mfendeksilverstripe created: #65 . Not sure why he didn't directly update FormFactory.

AjaxField is working around a limitation of how some ajax field work (e.g. TreeDropDown). Those fields need a REST endpoints to fetch data from the CMS. That REST endpoint is accessed via their parent form. However, in the case of linkfield, the parent form depends on your Link type key ... if you don't know what kind of link you are editing, ModalController can't retrive the form fields.

In hindsight, AjaxField works OKish, but it's probably worth investigating some alternatives ... especially if we give ourselves permission to change things in core to accomodate it. Of the top of my head, I'm thinking we could rewrite route that fetches the link form so the link key is a URL parameter instead of a GET parameter. Alternatively, we could create versions of those form fields that call generic end points that are not tied to a form.

The Registry class and _config/types.yml is used to define the link types, though it's unnecessary we can just look for subclasses of Link::class. _config/types.yml does allow you to se enabled: false to globally disable link types, though a better way would be to just have a private static $enabled on Link and just config it off on the subclasses you don't want. Also I'd much rather just have LinkField.php::setAllowTypes()/setDenyTypes() methods directly on the LinkField to specific allowed link types in a given context

All this part can go if we get rid of the registry and assume that Link subclass can be managed by a LinkField. We should add an option to allow specific subclasses to be disabled in specific LinkField instances.

Javascript

FileLinkModal.js calls the INIT_FORM_SCHEMA_STACK action which presumably does something in asset-admin

The file link form modal is a hack. I'm piggy backing on the File Link Form modal in the WYSIWIG. This means that you can't customised the file link form to add more information.

There's probably a way to do this more cleanly if you update core. I think there could be some really nice side benefit here. For example, you could give developers the ability to use their own form schema in an Insert Media Modal. I would like to do something like that in AnyField.

Frontend/backend comms

I disagree that GraphQL is substantially more complex than a REST here.

I think there's an argument that I'm subverting the main value GraphQL by JSON serializing the Link Form data and sending that to a GraphQL service to generate a description for it.

If we're going to ditch the AnyField idea, maybe we should consider making the entire thing work with GraphQL, including the link creation/update/delete. Only the ID of the created link would be attached to the form field. That would allow us to remove a bunch of logic from the LinkField PHP because the Link creation logic would be handled by GraphQL instead. (AnyField couldn't work that way without forcing you to pre-register all your DataObject in your admin schema.)

If we go REST, then I would like us to come up with a comprehensive convention of how and when we add REST endpoints to the CMS. Also, how we interact with those endpoints in React.

One link solution to rule them all

IF the whole point of this is that it's providing a standardised way for handling links across the board in Silverstripe CMS, then it should provide a TinyMCE plugin that overrides the ones currently provided by admin/cms/asset-admin which creates/uses Link records in the DB.

I don't think this needs to be a "be-all and end-all" link management solution. People have a need to manage links outside the WYSIWYG. That's basically the problem the other link module were trying to solve. To the extent that we could combine the two to provide a seamless experience, that might be worth exploring, but I don't see it as a outright blocker.

It should also provide reports for orphaned and for broken Link records. It should probably also provide either a BuildTask or a button in the above-mentioned report for deleting orphaned links.

Broken link tracking would be a plus.

On the orphan link tracking, I'm tempted to put it back on the developers to define cascade delete and ownership rules. But not 100% sure about this.

Overarching assumptions

There's a couple overarching assumption that I made that probably should be made explicit.

• Link can't exists by themselves and should be owned by a parent objects
• Link objects will be flat (no complex relations)
• Developer can choose to create their own link types by subclassing Link and the LinkField can accommodate that.
• Developers can chose to create their own LinkModal for custom link types that will allow them to handle unusual scenarios.

If we are not comfortable with those assumptions, we should call it out.

[emteknetnz]

Link can't exists by themselves and should be owned by a parent objects

There's not a whole lot we can do to stop that from happening since people can just programatically create DataObjects, or add them via a GridField. Only way to stop it is to get has_one implementation and forced all links to be has_many which means they'll have a ParentID (page) which we could validate beforeWrite(). I don't think there is any practical need to do this though

Link objects will be flat (no complex relations)

They're not flat now, SiteTreeLink and FileLink both have relations on them to SiteTree and File respectively

Developer can choose to create their own link types by subclassing Link and the LinkField can accommodate that.

That's already already doable

Developers can chose to create their own LinkModal for custom link types that will allow them to handle unusual scenarios.

I'm not sure what this means. If you create a new Link type you'll automatically get a dynamic form schema created for you inside the modal. Or do you mean using the js injector to swap out the modal itself? Not sure if that's possible now, though seems unnecessary / out of scope for what we're doing here if it's not.

I disagree that GraphQL is substantially more complex than a REST here.

GraphQL is lots more cluttered/complex for no benefit - I did a quick PR to POC getting rid of graphql and you end up removing a bunch of files and get functionally the same thing. To me that's seems like classic refactoring.

If we go REST, then I would like us to come up with a comprehensive convention of how and when we add REST endpoints to the CMS. Also, how we interact with those endpoints in React.

Convention I had in my PR was just using /admin/linkfield/<action> and use the standard fetch global function.

Multiple links

Seems like you've already implemented that in this PR. I'm happy to just merge this as is and then we can do a tidy up PR as we looks at doing other refactoring.

Do we want to versioned this by default? This introduces a dependency on versioned.

I'd say so since the main use case here is to put them on pages, which are versioned.

Would we consider conditionally versioning link only if versioned is installed?

We could, though I'm not sure in what the scenario someone would be using the linkfield module and not have versioned installed

There's also a related concern that if you are versioning Links, you need to make sure they get published when the parent record is published.

Just update the usage documentation to include $own, $cascade_deletes and $cascade_duplicates when adding Link::class to a relationship on your Page/DataObject

e.g. The external link should have validation to make sure that the provided URL is a valid URL.

I've got that implemented on a PR for validation

Whether we tie the permissions to an explicit owner for the link or to some pre-existing CMS permission, I'm not sure.

Does "tie the permissions to an explicit owner for the link" mean just use the owner Page for the permission check? That's functionally what happens now. I've added canView() checks for objects for related data in this PR.

"or to some pre-existing CMS permission" - I'm not sure what this means. We wouldn't be using the Permission::check() API here

[maxime-rainville]

We discuss this at our architecture catch up yesterday.

The key take away are:

• We will not be using the AnyField at this time.
• Link DataObject will be created when the Link Modal is save.
• The ID of the links will stored in the in the hidden field (rather than the JSON representation of the Link)
• Link registry is gone. The list of possible link type that can be created by a link will be passed as props.
• The Link title and Link description that get displayed in the LinkField prior to the Link being edited will retrieve via a GraphQL request. This will use regular GraphQL DataObject scaffolding.