NEW Control components written on GridFieldEditableColumns::handleSave

[NightJar]

Many objects contain a link back to the parent object (by nature of has_many), the same object that is being saved and triggering the GridFieldComponent to save its relational components in turn. This is often unnecessary and creates a 2N scenario for each record in the list, affecting performance. Especially when other onBefore/After/During write hooks are hanging off that class (e.g. fulltext search updates).

E.g.

Page:
  has_many:
    Things: Thing

Action: Save Page

bad execution flow:

Write Thing1
=>Write Page
Write Thing2
=>Write Page
Write Page

good execution flow:

Write Thing1
Write Thing2
Write Page

In order to have the opportunity to allow components to perform the second, this PR adds API.

[elliot-sawyer]

I have a large userform with ~30 formfields, after updating to a tag with this in it and manually applying this change, I shaved about 10 seconds off my publish time

[michalkleiner]

@elliot-sawyer how did you apply this change? Via an extension for the userforms page and change the config in onBeforeWrite or something similar? I still don't see how this would help on its own, but a more robust example/a gist would help and this could get in faster.

[elliot-sawyer]

I updated to userforms 5.14.0-rc1, then manually changed $item->write(false, false, false, true) to $item->write(false, false, false, false); to trial it out. Can't commit it obviously, but both changes did make a difference.

At first I thought I might be able to subclass and replace with the Injector, but this class isn't Injectable and gets invoked with new instead of ::create()

[elliot-sawyer]

I created a subclass using something like this:

<?php

class MyGridFieldEditableColumns extends GridFieldEditableColumns
{
    public function handleSave(GridField $grid, DataObjectInterface $record)
    {
        /** @var DataList $list */
        $list  = $grid->getList();
        $value = $grid->Value();

        if (!isset($value[self::POST_KEY]) || !is_array($value[self::POST_KEY])) {
            return;
        }

        /** @var GridFieldOrderableRows $sortable */
        $sortable = $grid->getConfig()->getComponentByType(GridFieldOrderableRows::class);

        // Fetch the items before processing them
        $ids = array_keys($value[self::POST_KEY]);
        if (empty($ids)) {
            return;
        }
        $itemsCollection = ArrayList::create($list->filter('ID', $ids)->toArray());

        foreach ($value[self::POST_KEY] as $id => $fields) {
            if (!is_numeric($id) || !is_array($fields)) {
                continue;
            }

            // Find the item from the fetched collection of items
            $item = $itemsCollection->find('ID', $id);

            // Skip not found item, or don't have any changed fields, or current user can't edit
            if (!$item || !$this->isChanged($item, $fields) || !$item->canEdit()) {
                continue;
            }

            $extra = array();

            $form = $this->getForm($grid, $item);
            $form->loadDataFrom($fields, Form::MERGE_CLEAR_MISSING);
            $form->saveInto($item);


            // Check if we are also sorting these records
            if ($sortable) {
                $sortField = $sortable->getSortField();
                if (isset($fields[$sortField])) {
                    $item->setField($sortField, $fields[$sortField]);
                }
            }

            if ($list instanceof ManyManyList) {
                $extra = array_intersect_key($form->getData() ?? [], (array) $list->getExtraFields());
            }

            $item->write(false, false, false, false);
            $list->add($item, $extra);
        }
    }

    /**
     * Whether or not an object in the grid field has changed data.
     */
    private function isChanged(DataObject $item, array $fields): bool
    {
        foreach ($fields as $name => $value) {
            if ($item->getField($name) !== $value) {
                return true;
            }
        }

        return false;
    }
}

The only line I changed was the 4th parameter of $item->write() from true to false. The isChanged method was copied from the parent, because it's private and cannot be inherited. Pop it into your userform like this:

public function getCMSFields()
{
...
        $formfields = $fields->dataFieldByName('Fields');
        $fieldsConfig = $formfields->getConfig();
        $fieldsConfig->removeComponentsByType(GridFieldEditableColumns::class);
        $editableColumns = new MyGridFieldEditableColumns();
//add your displayfields again
        $fieldsConfig->addComponent($editableColumns);

}

As you can see, it's quite a bit of effort to modify the class without this change. With the fix, I can remove my custom class and do this instead:

        $formfields = $fields->dataFieldByName('Fields');
        $fieldsConfig = $formfields->getConfig();
        $component = $fieldsConfig->getComponentByType(GridFieldEditableColumns::class);
        $component->setSkipWriteComponents(true);

The fix does work, but if I could suggest an improvement, I would change the name of the methods and variables to $writeComponents, setWriteComponents($var), and getWriteComponents(). This means once merged, no breaking changes would be introduced, and a developer like myself or @NightJar would have to go out of their way to use it.

[michalkleiner]

Thanks @elliot-sawyer , that's really helpful. I also agree with the suggested name changes.
@NightJar could you rename the additions to $writeComponents, setWriteComponents($var), and getWriteComponents()?

I won't be able to merge this myself but will get someone else to merge it.

[michalkleiner]

With this change the intended use would be

$formfields = $fields->dataFieldByName('Fields');
$fieldsConfig = $formfields->getConfig();
$component = $fieldsConfig->getComponentByType(GridFieldEditableColumns::class);
$component->setWriteComponents(false);

[NightJar]

I did briefly consider that when I wrote the PR.

However I thought it better to keep it like this because

• consistency in the Framework naming
• obvious what it's setting in the Framework context
• reduces confusion with other "components" in a GridField context.

[michalkleiner]

Should the logic then be the other way around? The 4th param of write is $writeComponents, so if we add a custom config to skip that, it would need to default to false, and be applied with !getSkipWriteComponents().

[NightJar]

It's named for a boolean, but accepts an array.

     * @param boolean|array $writeComponents Call write() on all associated component instances which were previously
     *                      retrieved through {@link getComponent()}, {@link getComponents()} or
     *                      {@link getManyManyComponents()}. Default to `false`. The parameter can also be provided in
     *                      the form of an array: `['recursive' => true, skip => ['Page'=>[1,2,3]]`. This avoid infinite
     *                      loops when one DataObject are components of each other.

When "truthy" (true or a correctly configured array for which the correct configuration is 100% opaque API) it ends up getting passed as the $skip parameter to writeComponents and then on down to skipWriteComponents (private).

https://github.com/silverstripe/silverstripe-framework/blob/4/src/ORM/DataObject.php#L1743

... Although, I guess the configuration also controls the recursive parameter passed to writeComponents, but I guess the intention in my case was soley on utilising skip. 🤔
Let me hmm on this a bit. Was perhaps a bit myopic in consideration.

[michalkleiner]

Fair point. How about private $customWriteComponents = true or private $customWriteComponentsConfig and you can pass whatever you want?

[GuySartorelli]

This pull request hasn't had any activity for a while. Are you going to be doing further work on it, or would you prefer to close it now?

[MrJamesEllis]

I feel this is related to silverstripe/silverstripe-userforms#1249 - similar to what's been discussed in this PR about sluggish userform performance. Struggling to see a use case for an editable form field to write components recursively but happy to be enlightened.

The commit this landed in is here, for reference:
d357479

This has been enabled since 2019 by default. An immediate option would be to allow the writeComponents argument to be configurable at a project level. An array is allowed as the parameter to specify which components to skip.

At the least it's a performance issue.

Cheers
James

[GuySartorelli]

It seems like there's going to be no further activity on this pull request, so we’ve closed it for now. Please open a new pull-request if you want to re-approach this work, or for anything else you think could be fixed or improved.