Initial review of AnyField

[maxime-rainville]

AnyField has a lot of feature that could complement/supersede LinkField. But we are not sure how to approach the inter play between both module.

Acceptance criteria

• A top down review of AnyField 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.
• Any issues that come out of the LinkField review that could be addressed by AnyField are documented.

[emteknetnz]

How to access this?

Anyfield grew from the linkfield codebase and seems like it has two different goals:

• allows you do edit any Dataobject in a modal
• be a better version of linkfield

Accessing anyfield as a way to manage any DataObject in a modal

Being able to edit any dataobject relation in a modal is very ambitious because to do it properly you need to get all of the existing dataobject edit form functionality to work into a modal. After a quick look at anyfield I don't think it's close to fulfilling this as I noticed plenty of shortcomings. Just a few were:

• It cannot handle has_many relations on the DataObject being edited i.e. a nested gridfield
• The image upload field on a has_one relation on the DataObject didn't work
• Doesn't gracefully handle large edit forms
• When adding a new DataObject will no subclasses it still uses a drop down field even those there's only ever one option

It would probably make more sense to transplant some of this code to silverstripe/admin and we'd be doing this in the context of a totally different epic, not one that looks at formally supporting linkfield.

If we were to start seriously looking at editing DataObject in modals then I'd want it to be more than just "editing relations on dataobjects" and I'd want it to be the more generic "editing dataobjects". As anyfield grew from linkfield, it can only do relations on dataobjects as it communicates data updates with the server by taking the contents on edit form in the modal, turning it into JSON and sending it to the server via the parent Page edit forms POST request. If we were to start seriously looking at 'editing dataobjects in modals' I think at that point we'd be making a transition to making the CMS fully API driven rather than the existing method of POST form submissions and PJAX responses. That is obviously way too much scope for what we're doing right now which is a linkfield specific epic.

Accessing anyfield as a better linkfield

The other way to view this is that anyfield is just a better implementation of linkfield with no ambitions beyond that. This is how I'll proceed. I won't call out some things that were already called out in the linkfield review e.g. need to write dev docs

Main improvements over linkfield

has_many relations

• This module has added the ability linkfield is the ability to support has_many relations
• The new UI adds a 'picker' component which consists of essentially a button (could possibly be restyled to be more like a standard button), and a dropdown where you select from a list of subclasses of Link. By default the base Link class shows which is annoying as it's not meant to be used.
• There is an open PR to sort the has_many field though it doesn't appear to work. I tried checking out the unmerged PR pulls/0/react-sortable-hoc which adds sorting, I ran yarn install in silverstripe/admin, yarn build in the anyfield directory and refreshed the CMS while developer tools was open with 'disable cache' ticked. It did not seem to add any sorting capabilities to has_many links fields. The build for that PR is currently red.

Security

• Does have can*() write operation checks in JsonField::saveInto() and ManyAnyField::saveInto() that will 403 on fail
• There's code that prevents you from changing the 'ID' of any existing object
• I did try crafting malicious requests to do things like create new Member's by manipulating classnames in POST submissions, these attempts were unsuccessful

Test coverage

• PHPUnit coverage has improved a lot as there are many more unit test files
• There are behat tests
• Jest test coverage is light
• There are a couple of storybook stories
• CI is current green

Architectural

Over abstraction

• Because the philosophy is for 'Any dataobject' the module is fundamentally over-abstracted for only managing Link::class.
• This is adding a bunch of additional complexity for no benefit, so it's a bad thing.

maxime-rainville/silverstripe-react

• Uses maxime-rainville/silverstripe-react to load react onto entwine, would prefer to just keep this "hardcoded" like in linkfield so it's one less dependency, particularly since this one is pre-stable. README.md for the module says "IN DEVELOPMENT!!!" and has no instructions. If we include this dependency here but not everywhere else then it's inconsistent with the rest of the CMS.

GraphQL

• Use of graphql/apollo is overly complex, it's only for AnyFieldDescriptionResolver. Should be replaced with a controller + js fetch() which developers are more familiar with. We use a regular controller for the AnyFieldForm so it would be more consistent to only use one type of API endpoint for all the requests rather than two.

Documentation

• README needs a full rewrite as it talks about anyfield being for managing dataobjects, rather than just links.
• Copy pasting the "Sample usage" code causes a 500 [Emergency] Uncaught Exception: No has_one found on class 'SilverStripe\LinkField\Models\Link', the has_many relation from 'Page' to 'SilverStripe\LinkField\Models\Link' requires a has_one on 'SilverStripe\LinkField\Models\Link'
• I could resolve by adding an extension to Link which add a has_one Page to it, though this does add a Page dropdown to the scaffolded form on the Link Edit form, which we'd want to remove / convert to a hidden field

Code

PHP

• AnyService::generateFieldDefinition() looks for private static $icon and $modal_handler on arbitrary DataObjects even though they aren't defined on DataObject. Icons specific to links seems like they should be defined on the Link type, not the dataobject
• AnyService::generateDescription() does the same with DataObject::getSummary() which isn't defined on DataObject.
• AnyService has a method getAllowedDataObjectClasses though it's only called once from AllowedClassesTrait so that method should probably live there.
• Class and variable naming could be better e.g. single character variables, calling DataObject's $value some places, $record in others, etc
• Extensions directory contains classes AjaxField/LeftAndMain/ModalController which are used to make an /AnyFieldFormendpoint which serves a dynamic form schema - basically the same thing as linkfield's DynamicLink endpoint. Similar to linkfield could use a tidy up. Has the same suboptimal naming e.g.LeftAndMain extends Extensionas opposed to the standardLeftAndMainExtension extends Extension`
• ManyAnyField::setValue() accepts a mixed $value as seemingly either string|SS_List|(empty) which isn't great, should be of a single type
• AnyField has a method guessBaseClass() which just sounds unreliable
• PHP code is based on linkfield, has gotten rid of the following directories: ORM (DBJson implementation), Tasks (migration task from linkable module), Models (default 5x link implementations), Type (Registry of Link types) and the 2x interfaces.

Javascript

• When POSTing a ManyLinks a new object has a react ID e.g. '2c1de3dd-203d-4bd9-9c58-07d51895ca93' - this should be stripped out client side before sending.

Other

• behat directory needs to be added to .gitattributes so doesn't install with dist
• I tried having 2 different has_many fields both using Link::class, the first one has a couple of records and the other had none. The first field worked correctly, but the second field mistakenly rendered records from the first.

Where to from here?

• From the point of view of being a drop in replacement for the existing linkfield, I'd say no. Because this module aims to manage any dataobject it's fundamentally over-abstracted so it's adding complexity for no benefit.
• In terms of a general 'edit any dataobject' I think it's quite some distance from getting core adoption and it should probably be targeting silverstripe/admin rather than being its own module.
• So I guess the main question is what do we do with this module in the context of the linkfield epic? Seems like we'd be best to cherry-pick some of the good bits from this and apply them to linkfield, namely has_many support, better security and test coverage.

ACs that were intentionally skipped

• Feature gaps
• Any issues that come out of the LinkField review that could be addressed by AnyField are documented.

[maxime-rainville]

Being able to edit any dataobject relation in a modal is very ambitious because to do it properly you need to get all of the existing dataobject edit form functionality to work into a modal. After a quick look at anyfield I don't think it's close to fulfilling this as I noticed plenty of shortcomings. Just a few were:

• It cannot handle has_many relations on the DataObject being edited i.e. a nested gridfield
• The image upload field on a has_one relation on the DataObject didn't work
• Doesn't gracefully handle large edit forms
• When adding a new DataObject will no subclasses it still uses a drop down field even those there's only ever one option

As the ReadMe mentions "AnyField and ManyAnyField are best suited to managing simple DataObjects that are tightly coupled to their owner. This module will not work well for DataObjects with complex relations."

I see it more as something similar to the old hasonefield project in purpose. Probably, AnyField should throw an exception if you give it a class with has_many relations.

Either way, we will have the same problems with LinkField. e.g.: If you decide to stick a GridField in your Link subclass, LinkField will breakdown.

If we were to start seriously looking at editing DataObject in modals then I'd want it to be more than just "editing relations on dataobjects" and I'd want it to be the more generic "editing dataobjects".

I don't think that's something we should do. My perception of AnyField is something that complements Gridfield by being simpler and less intrusive.

I think there could be a place for both.

The new UI adds a 'picker' component which consists of essentially a button

It doesn't really add anything new in that front. It just takes the existing drop down that allows you to pick a link type. The main innovation is that when rendered in ManyAnyField the picker is always visible so you can add new entries.

There is an open PR to sort the has_many field though it doesn't appear to work.

Just realised I neglected to provide docs on how to enabled Sorting. A dev has to explicitly say what column they want to use to sort the results.

ManyAnyField::create('Pets')->setSort('Sort')

Just added the missing bit of doc.

I did try crafting malicious requests to do things like create new Member's by manipulating classnames in POST submissions, these attempts were unsuccessful

I spent a lot of time considering and mitigating this possibility. That's what the AllowedClassesTrait::validClassName() is meant to block.

This a much bigger concern for AnyField than for a plain LinkField. LinkField will understandably only let you create Links. We probably still want to add some explicit logic to preclude the creating of Links of an invalid type.

• Because the philosophy is for 'Any dataobject' the module is fundamentally over-abstracted for only managing Link::class.
• This is adding a bunch of additional complexity for no benefit, so it's a bad thing.

I'm not sure this is true. A lot of the extra logic in AnyField is there to accommodate new features that we'll want in LinkField like limiting the type of Links a specific field can create.

The only bit you could eliminate by deciding to only support Links are contains in AllowedClassesTrait. Those mostly relate to controlling what the base class for the field is and constraining what sub class can be created.

Once you get ride of the Link Type Registry and start looking at scenario this "LinkField should only accept an EmailLink".

You could make it a bit more simple by forcing dev to explicitly say if they want the LinkField to accept a sub class of link rather than all Links. But you'll still have to account for the scenario where a someone tries to save an ExternalLink in a field that's pointed to a EmailLink relation.

I think once you get stuck into it, you'll find that substituting the "this field can manage any DataObject and its subclasses" assumption with "this field can manage Link DataObject and its subclasses" doesn't allow you to reduce complexity all that much, if at all.

Uses maxime-rainville/silverstripe-react to load react onto entwine, would prefer to just keep this "hardcoded" like in linkfield so it's one less dependency, particularly since this one is pre-stable. README.md for the module says "IN DEVELOPMENT!!!" and has no instructions. If we include this dependency here but not everywhere else then it's inconsistent with the rest of the CMS.

maxime-rainville/silverstripe-react is very much an experimental thing and I agree it makes sense to ditch it if we're only focusing on getting LinkField part. I do think it highlights a possible approach we could take in building React components and we should look at doing something like that when we start exploring the "UX Framework" epic down the track.

Use of graphql/apollo is overly complex, it's only for AnyFieldDescriptionResolver. Should be replaced with a controller + js fetch() which developers are more familiar with. We use a regular controller for the AnyFieldForm so it would be more consistent to only use one type of API endpoint for all the requests rather than two.

Whether we go with LinkField or AnyField, that's going to do the same thing.

Copy pasting the "Sample usage" code causes a 500 [Emergency] Uncaught Exception: No has_one found on class 'SilverStripe\LinkField\Models\Link', the has_many relation from 'Page' to 'SilverStripe\LinkField\Models\Link' requires a has_one on 'SilverStripe\LinkField\Models\Link'

The Link Object needs a has_one back to whatever DataObject owns it. There's a separate PR for linkfield that adds the minimum bits required for this thing to work. silverstripe/silverstripe-linkfield#76

We'll need to this no matter what to support has_many relations.

• AnyService::generateFieldDefinition() looks for private static $icon and $modal_handler on arbitrary DataObjects even though they aren't defined on DataObject. Icons specific to links seems like they should be defined on the Link type, not the dataobject
• AnyService::generateDescription() does the same with DataObject::getSummary() which isn't defined on DataObject.

When doing the conversation to AnyField, I tweaked the approach for picking the description to make more amenable to customisation by configuration. If the relevant method or config option is missing from the DataObject, AnyField will default to a sensible generic behaviour.

• ManyAnyField::setValue() accepts a mixed $value as seemingly either string|SS_List|(empty) which isn't great, should be of a single type

That method can be called when the data is loaded from the Form (SS_List) or when the data is loaded from the front end (JSON strin). I don't see a way around this.

• AnyField has a method guessBaseClass() which just sounds unreliable

The primary purpose of this class is to make sure we can determine what DataObject class we expect from looking at the relation ... as opposed to needing the developer to explicitly tell the Field.

So far it has proven pretty resilient and I tried to throw quite a few variation at it in the unit test.

AnyField will hard fail if it can't figure out what the baseClass is.

One limitation would be what would happen if you put a LinkField that wasn't directly tied to a parent DataObject CMS Form. I think LinkField and AnyField are likely to fail in that scenario.

• PHP code is based on linkfield, has gotten rid of the following directories: ORM (DBJson implementation), Tasks (migration task from linkable module), Models (default 5x link implementations), Type (Registry of Link types) and the 2x interfaces.

Just be 100% clear, I removed those classes with the assumption that AnyField would exist independently from LinkField and that you would need to insall both module if you wanted to manage Links in an AnyField.

• When POSTing a ManyLinks a new object has a react ID e.g. '2c1de3dd-203d-4bd9-9c58-07d51895ca93' - this should be stripped out client side before sending.

Those IDs are needed because when we send the data to the backend to generate the description for a link, we need an ID to link the response back to a specific record. I used he uuid JS library to generate those, which is why they are very long. When editing an item from a list, that's also what the Modal uses to tell the JS field what entry to update in the JSON string.

You'll probably need something like that no matter what.

You could try to strip it out on Send, but right now the AnyField doesn't really try to know when its data is being send. It just keeps track of it's state.

PHP side, AnyField just choose to ignore any ID that's none-numeric and assume they belong to a new DataObject that needs to be created.

• I tried having 2 different has_many fields both using Link::class, the first one has a couple of records and the other had none. The first field worked correctly, but the second field mistakenly rendered records from the first.

That's just the nature of having two has_many relations pointing to the same DataObject class: If you have LinkListA and LinkListB and they both point to Link, how is a child Link object meant to know if it's part A or B?

The way around this is to create a secondary owner on Link and use this syntax:

private static $has_many = [
  'LinkLiskA' => Link::class,
  'LinkLiskB' => Link::class.'.SecondaryOwner',
];

You'll have the same problem on LinkField if you try to add support for List.

Extra improvements that the AnyField has that LinkField doesn't

• LinkField doesn't currently support icon. I think that adds a nice UI touch.
• Loading animation is slightly nicer.
• LinkField relies on a GraphQL query to figure out what LinkTypes are available. AnyField passes those as props on the react component. I think the AnyField approach is nicer.
• AnyField has a pre-react-render view that gives it a bit of UI shape so the things looks better while it's loading.
• maxime-rainville/silverstripe-linkfield-tester is a test module I created whose only purpose is to create a bunch of fake pages and elemental block types with link relations. We'll probably need something like that for testing no matter what.

[maxime-rainville]

Following our chat with our bespoke teams, we concluded that we prefer to focus on the LinkField part for now. We may revisit this further down the track.