NEW LinkFieldController to handle FormSchema

[emteknetnz]

Issue #84

You can use the following to quickly create a page with a link field on it

Page.php

<?php

use SilverStripe\CMS\Model\SiteTree;
use SilverStripe\LinkField\Form\LinkField;
use SilverStripe\LinkField\Models\Link;

class Page extends SiteTree
{
    private static $db = [];

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

    public function getCMSFields()
    {
        $fields = parent::getCMSFields();
        $fields->insertAfter('Content', new LinkField('MyLink'));
        return $fields;
    }
}

You can test form validation of PhoneLink using invalid values of "x" and "y" with the following extension:

extensions.yml

SilverStripe\LinkField\Models\PhoneLink:
  extensions:
    - PhoneLinkExtension

PhoneLinkExtension.php

<?php

use SilverStripe\ORM\DataExtension;
use SilverStripe\ORM\ValidationResult;
use SilverStripe\Forms\CompositeValidator;
use SilverStripe\Forms\Validator;

class PhoneLinkExtension extends DataExtension
{
    public function validate(ValidationResult $validationResult)
    {
        $phone = $this->owner->Phone;
        if ($phone == 'x') {
            $validationResult->addFieldError('Phone', 'Cannot be x - DataObject::validate()');
        }
        return $validationResult;
    }

    public function updateCMSCompositeValidator(CompositeValidator $compositeValidator): void
    {
        $compositeValidator->addValidator(new class extends Validator {
            public function php($data): bool
            {
                $valid = true;
                $phone = $data['Phone'];
                if ($phone == 'y') {
                    $valid = false;
                    $this->validationError('Phone', 'Cannot be y -- DataObject::getCMSCompositeValidator()');
                }
                return $valid;
            }
        });
    }
}

Parent issue

• Refactor LinkField around FormSchema #105

[maxime-rainville]

This is just an initial review. I'm planning on having a second pass.

• The re-factored link field doesn't work at all in Elemental.
• This is probably because the Root state management has some problems.
• We've lost the FileLinkModal. I'm fine with spinning this off into it's own card because it is a bit of a headache to get working correctly, but I would like us to at least be confident the new approach can support it.

[maxime-rainville]

Setting the initial value of useState hook from a prop is wrong.

onChange should be updating the parent state and the information should flow back down there.

[emteknetnz]

Updated

[maxime-rainville]

Need to handle other errors beyond validation. (e.g. Session time-out, not enough permission, random 500)

[maxime-rainville]

Can we look at moving all the methods that do HTTP request to their own file?

It's somewhat manageable now, but once we throw in Link lists in there I think it's going to became a bit more difficult.

Having them as individual method will also make it easier to write JEST test for them.

[emteknetnz]

Not sure what quite sure why we'd move HTTP request stuff to a different file. HTTP request are:

• GraphQL query to get link types
• fetch() (lib/Backend) query to fetch data
• fetch() (lib/Backend) query to delete link

The queries are simple and don't take up much in the file. Splitting things across files seems like it's needlessly increasing complexity.

For the 2x fetch() requests we can mock in jest them pretty easily - e.g. https://github.com/silverstripe/silverstripe-session-manager/blob/2/client/src/components/LoginSession/tests/LoginSessionContainer-test.js#L10

For GraphQL, I don't know how to mock the responses in jest

[emteknetnz]

Can implement in #110

[maxime-rainville]

Having played with the thing a bit more, the const match = formSchema.id.match(/\/linkForm\/([0-9]+)/) logic doesn't seem robust.

I'm thinking this could be substantially better.

• If you're form validation that should return a 400 along with a form schema.
• If you're form submission is successful, then it shouldn't return a form schema at all. It should return the ID of the successfully created record. That would mimic tho behaviour of a regular Form object where we return a view with a success message.

[emteknetnz]

If you're form validation [failed] that should return a 400 along with a form schema.

I can't find the exact commit (it's been forced pushed over so many times), though I do remember there was a technical reason why I chose to not return a 400 on validation failure, which I'd definitely prefer to do.

From memory it's because in lib/Backend (Silverstripe wrapper for fetch()) we have some custom code to throw Errors on 400+ status codes (i.e. !response.ok) so that we can then use fetch's .catch() to handle these. However there's this code in FormBuilder that will then just throw these Errors again and won't get the FormSchema returned, meaning that now validation messages don't show in the FormBuilder form

You can test this by updating the last bit of $form->setValidationResponseCallback in LinkFieldController::createLinkForm()

            $response = $this->getSchemaResponse($schemaId, $form, $errors);
            $response->setStatusCode(400);
            return $response;

You'll get a 400 returned on validation failure, however you'll also get javascript errors in FormBuilderModal.js and Backend.js in browser developer tools

[emteknetnz]

If you're form submission is successful, then it shouldn't return a form schema at all. It should return the ID of the successfully created record.

I don't agree. To the best of my knowledge the existing FormBuilder/FormSchema "standard" is designed to expect a FormSchema with no errors returned on success. Presumably under the hood it just re-renders the form at that point using the returned FormSchema? Having a quick look at campaign admin it returns a FormSchema on success.

We absolutely should NOT be tinkering with FormBuilder/FormSchema at this stage IMO. While it's a kind of weird standard it does work pretty great and we should just implement it as is

[maxime-rainville]

Looking at Campaign Admin, on success it it's stick a message attribute on success that then gets displayed on the front end.

I don't like that behaviour. It should be displaying a toast instead.

I've added a few ACs to silverstripe/silverstripe-admin#1589:

• There's a standardised and documented way of creating new FormSchema endpoints in the CMS.
  • This accounts for form validation
  • This accounts for communicating success to the front end (e.g. Displaying success message)
  • Extra context about the success of the form submission can be communicated to the parent JS process

Created a follow up card.

[maxime-rainville]

We should have a follow up card so that the link title can be inferred from it's metadata later where appropriate. e.g.: Pointing a link to a Page probably means that the page title would be a suitable title.

[maxime-rainville]

I want to retain this for now.

[emteknetnz]

Reverted

[maxime-rainville]

I had second pass. I think the approach looks broadly solid.

I'll create some follow up cards for problems that I don't think need to be addressed in this card.

Some key things I think need to be addressed now:

• The link field needs to work in Elemental
• Root state management
• Retain the ability to use different Link modal (I'm fine if the file link modal is broken for now)
• Do we want to store just the ID in the parent input?
• Making sure LinkField still work if you customise your admin area url.

Things that need to be addressed, but can be spun off as related card

• Handling all possible errors form AJAX requests
• Loading UI
• File link modal and alternative link modal
• Using GraphQL to for initial link description and clearing link
• Localisation
• Controller for parking API end points and form schema
• Readonly state of form schema

Other things that we can decide later

• Migration from other modules ... just retain the existing one for now.
• Orphan links.
• Inferring Link title

FYI I didn't look at the fine print of all the test coverage for now, since I presume there's a substantial bit that still needs to be updated.

[maxime-rainville]

We have an immediate use case for creating link for File. The FileInsertModal is design so you can use different form schema inside of it:

• Insert a link to file in the WYSIWYG,
• Insert an image in the in the WYSIWYG,
• Edit file details,
• Edit folder details.

I could imagine complex scenarios where you might want to display a complex form for creating links:

• I want to create a link to GitHub issues and I need to build a search form in my modal so I can get there.

[maxime-rainville]

Can we look at moving all the methods that do HTTP request to their own file?

It's somewhat manageable now, but once we throw in Link lists in there I think it's going to became a bit more difficult.

Having them as individual method will also make it easier to write JEST test for them.

[emteknetnz]

@maxime-rainville I've split off a separate card for elemental support, looks like it's non-trivial - #116

[maxime-rainville]

I've split off a separate card for elemental support, looks like it's non-trivial

Making sure this new approach works with Elemental is pretty fundamental. I think getting it working in Elemental is mostly a question of stealing some bits from my silverstripe-react module.

I had a go to try to get it working in this PR and mostly got there. I'm confident the new approach can be made to work with the new form schema approach.

It took me a couple days to wrap my head around getting a form field working in React and Entwine. Probably best if I pick the elemental follow up card.

[maxime-rainville]

Having played with the thing a bit more, the const match = formSchema.id.match(/\/linkForm\/([0-9]+)/) logic doesn't seem robust.

I'm thinking this could be substantially better.

• If you're form validation that should return a 400 along with a form schema.
• If you're form submission is successful, then it shouldn't return a form schema at all. It should return the ID of the successfully created record. That would mimic tho behaviour of a regular Form object where we return a view with a success message.

[maxime-rainville]

I think we need to agree with some convention up front.

linkID should be called value. This is not just an academic argument. Redux-form will call it value when it passes it in the form schema.

Equally, id should be reserved for the id of the form field.

[emteknetnz]

I've changed the prop to value which does make sense as it is the value of the FormField, though I've also kept a local variable const linkID = value; - it's simply a lot easier to follow the code as linkID is a more descriptive name

[maxime-rainville]

If it was already like that I would be fine with it. But we are literally removing something we are planning to bring back in a not too distant PR.

[maxime-rainville]

Suggested change

	$adminRoot = AdminRootController::get_admin_route();
	$urlSegment = $this->config()->get('url_segment');
	$typeKey = Registry::create()->keyByClassName($link->ClassName);
	$form->setFormAction("$adminRoot/$urlSegment/linkForm/$id?typeKey=$typeKey");
	$typeKey = Registry::create()->keyByClassName($link->ClassName);
	$form->setFormAction($this->Link("linkForm/$id?typeKey=$typeKey"));

[emteknetnz]

Updated

[maxime-rainville]

Not quite happy with everything. But happy enough to merge in the interest of keeping things moving and unblocking follow up cards.

[maxime-rainville]

Looking at Campaign Admin, on success it it's stick a message attribute on success that then gets displayed on the front end.

I don't like that behaviour. It should be displaying a toast instead.

I've added a few ACs to silverstripe/silverstripe-admin#1589:

• There's a standardised and documented way of creating new FormSchema endpoints in the CMS.
  • This accounts for form validation
  • This accounts for communicating success to the front end (e.g. Displaying success message)
  • Extra context about the success of the form submission can be communicated to the parent JS process

Created a follow up card.