Throw exception when saving afforms with mandatory values missing #31479
Conversation
|
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
|
@jensschuppe I think the "what to do here?" is still an open question. Your change makes sense for one scenario, but consider another: |
| @@ -353,6 +353,9 @@ public static function processGenericEntity(AfformSubmitEvent $event) { | |||
| // What to do here? Sometimes we should silently ignore errors, e.g. an optional entity | |||
| // intentionally left blank. Other times it's a real error the user should know about. | |||
| \Civi::log('afform')->debug('Silently ignoring exception in Afform processGenericEntity call for "' . $event->getEntityName() . '". Message: ' . $e->getMessage()); | |||
| if ('mandatory_missing' === $e->getErrorCode()) { | |||
There was a problem hiding this comment.
| if ('mandatory_missing' === $e->getErrorCode()) { | |
| if (1 === count($event->records) && 'mandatory_missing' === $e->getErrorCode()) { |
This would'nt be a problem if we were still in times when a spouse wasn't optional for a household 😉
Ok, this was obviously too optimistic. I think there should be a configurable option for making entities mandatory in FormBuilder, or at least consider the only entity in a form mandatory by default - what do you think?
There was a problem hiding this comment.
Typically that's done by making fields required. For example, if you make the spouse first/last name fields required, then effectively the form requires a spouse. Otherwise, if all fields are optional, then the entity is not required.
There was a problem hiding this comment.
Hm, but this is reaching limits in case you want to require a spouse but not a specific attribute (let's say either first/last name or e-mail should be required). Marking the entire entity required would leave the validation of mandatory attributes to the API and allow FormBuilder to display the mandatory_missing error message.
Would you agree that a form with a single entity (count($event->records) === 1) would justify mandatory_missing errors to be displayed?
There was a problem hiding this comment.
Would you agree that a form with a single entity (
count($event->records) === 1) would justify mandatory_missing errors to be displayed?
Probably yes, in most cases. But perhaps it would be better to make it explicit on the entity and give the option for an entity to be required.
Overview
Do not silently fail FormBuilder submissions when the entity actually couldn't be saved due to missing mandatory values.
Before
FormBuilder forms silently fail with any submission error in
\Civi\Api4\Action\Afform\Submit::processGenericEntity(), only logging the error. The code has this comment:After
For exceptions with the error code
mandatory_missingthe exception is thrown forward in order for the form to show an error message to the user, instead of silently failing and showing Saved.Technical Details
There might be more error codes which throwing forward the exception might be useful for, but
mandatory_missingseems to always justify an error message.Comments
cross-referencing systopia/de.systopia.eck#138 (comment)