ENH performance improvement for writing editableformfields

[S-GrabhamMadden]

Description

The onBeforeWrite function for EditableFormField checks whether the frontend form field has data or not, and if not, sets Required to false. Getting the frontend form field can be expensive in performance for a large number of fields in a single form, but we only really need to do that if we're currently trying to set Required to true.

Adding a check for this brought the write time of a userform in one of our sites (with about 80 fields) down from 60+ seconds into the range of ~5 seconds.

Manual testing steps

On a userform and/or elementform, compare the time it takes to save and/or publish with a large number of fields.

Issues

• Relevant to Saving UserForms with many fields causes timeouts #773

Pull request checklist

• The target branch is correct
  • See picking the right version
• All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
  • Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
• The commit messages follow our commit message guidelines
• The PR follows our contribution guidelines
• Code changes follow our coding conventions
• This change is covered with tests (or tests aren't necessary for this change)
• Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
• CI is green

[ChloeSartorelli]

What are the implications of checking if the value is being changed? For example if Required was already true, the existing code used to explicitly set it to false if there was no data, but will now leave it as true.

[S-GrabhamMadden]

Only checking if ($this->Required) would still produce some performance savings, but significantly less where there are valid required fields in the form, as those would all still need to be checked again each time.

As you say, the gap in the logic here with the isChanged check is that if Required is set true already, from some past write, this won't set it false again on future writes.

For an existing field in a database after making this change, this state should never be the case in theory - the previous logic has been running before every write, so Required would be correctly set to false for all fields it should be already.

For a new field, the only way I can think of that a field might be set with Required as true, without a write where it is changed, might be on creation if the default is set to true for that field? But I feel if a variable has a default of a value that it should never have, that's an issue in the field definition.

There may be other implications I've missed but I don't see a clear way a field could be in a state where Required is true without it being checked by this beforewrite while being changed at least once. Perhaps manually changing DB with SQL queries without writing could do it.

[ChloeSartorelli]

I'm thinking about edge cases, such as an editable field that dynamically changes what its underlying FormField class is.

It could have Required set to true in one write, but then in a subsequent write whatever determines its underlying FormField class could be changed. The new FormField might return false from the hasData() method.

In that scenario, prior to this PR the Required value would be set false, but with this PR it won't be.

[S-GrabhamMadden]

That scenario makes sense, that does seem to be an uncovered edge case.

To account for dynamically changing FormField classes without shifting some of the responsibility out of this function we'd probably need to check getFormField() anyway, so performance gain would be lost.

One way to support this might be to add a function returning a list of DB Fields that might impact whether a field can be required/needs to be checked, which custom EditableFields that dynamically change the FormField class based on other field settings can extend and add to.

EG something roughly like

function getHasDataAffectingFields()
{
    return ['Required', 'ClassName', etc];
}

function checkPossibleHasDataChange()
{
    foreach $this->getHasDataAffectingFields()
    check this field isChanged
    if any are, return true, otherwise return false
}

if ($this->Required && $this->checkPossibleHasDataChange()) {

That would allow editablefields that have dynamically changing getFormField results to account for this edge case, though if the result is dependent on something else entirely (eg not based on a field on the editablefield) this wouldn't account for it, and the checkPossibleHasDataChange function would need to be overwritten to cover whatever the specific behaviour is.

The simple option which avoids this new requirement on custom editablefields with dynamic FormFields is to drop some of the performance improvements (scaling with how many fields on the form are required) and just add if ($this->Required) which doesn't have the risk of such edge cases, and is still better than nothing.

[ChloeSartorelli]

The simple option which avoids this new requirement on custom editablefields with dynamic FormFields is to drop some of the performance improvements (scaling with how many fields on the form are required) and just add if ($this->Required) which doesn't have the risk of such edge cases, and is still better than nothing.

Let's go with that for now - if you want to look into ways to handle the dynamic field situation we can discuss that separately, but at least the performance will be somewhat improved in the meantime. I think it will be difficult to do in a way that doesn't require making changes to any existing dynamic fields though which would constitute a breaking change.