dev/core#2985 Original value is displayed after setting custom event field blank

[braders]

Overview

This resolves https://lab.civicrm.org/dev/core/-/issues/2985:

Attempting to change the value of a custom event field from non-blank to blank results in the original value being populated in the form after the form is saved. If the form is subsequently saved again, then the original non-blank value is written to the DB.

Before

Reproduction steps

1. Create a custom event fieldset.
2. Add a custom field of type Alphanumeric, Single line input field. Accept all default settings.
3. Edit an event.
4. Set the new custom field to a non-blank value and save the event.
5. Set the new custom field to a blank value and save the event.

Current behaviour
The custom field displays the non-blank value. If the form is saved, the blank value currently in the DB is overwritten with the non-blank value.

After

The custom field displays the submitted value (the blank value).

Technical Details

I traced this issue down to the customData cache in CRM_Core_BAO_CustomGroup::formatGroupTree. Digging revealed this to be introduced to resolve the issue described in https://issues.civicrm.org/jira/browse/CRM-16676 and https://issues.civicrm.org/jira/browse/CRM-18427:

The bug is that no custom data values are redisplayed on the form, which means they've got to be reentered. I thought I'd fixed this by introducing a check of $_POST for values in for CRM_Core_BAO_CustomGroup::formatGroupTree, which seems to be how values pulled from the database are passed into the form. But my fix relies on CRM_Custom_Form_CustomData, which is called at the end of the preProcess if there are $_POST values, but it seems like the AJAX handling of custom data now uses CRM_Custom_Form_CustomDataByType at which point $_POST no longer contains those submitValues.

I initially removed the cache code $submittedValues = Civi::cache('customData')->get($qfKey); and verified that it resolved dev/core#2985, but that the issue described CRM-16676 regressed. This eventual solution retains the cache, which is needed in the case of validation errors, but more strictly checks for null values (rather than false'y values), treating an empty string as a valid submitted value.

Comments

The original Jira tickets are a bit light on detail, so whilst it's quite easy to test the issue described on dev/core#2985, it's harder to fully test all the permatations that could be impacted by this change.

It's worth noting that reproducing the issues on Jira seems quite tricky as required fields are now client-side validated. I was only able to reproduce if I manually removed the required class in devtools on a custom field.

[civibot]

No issue was found matching the number given in the pull request title. Please check the issue number.

[civibot]

(Standard links)

• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[civicrm-builder]

Can one of the admins verify this patch?

[larssandergreen]

I have reviewed.

r-run passes, works as advertised.
Code looks good.

I think this is ready to merge.