eca_form: Ajax handler does not work for checkboxes in content entity forms

Unfortunately I had to touch several places in the eca_form module. The bug itself comes from a chain of mutliple places in code where I had to adjust it accordingly.

I also had

...to reduce the possibilities of automated entity builds within the EcaExecutionFormSubscriber, because there are situations where this might fail due to incomplete or not yet performed form processing. When someone needs the most recent entity state of submitted form values, the action "Entity form: build entity" is supposed to take over this job when needed.

...to adjust some existing form tests, especially regards the "Entity form: build entity" action, because this one should be more flexible now - and it now always respects form fields as configured in the entity form display, also after form validation was applied.

• MR301 is for 1.2.x
• MR302 is for 1.1.x (backport)

Please note that MR301 will contain one failed test, which is not related to this issue. That failed test comes from ✨ Extend cache actions beyond ECA Fixed and must be fixed there.

This is blocking the realization of our requirements for building multi-step forms using Ajax.

Testing this internally for another time.

i did tests with following 'Form: add Ajax handler' configs:
Test case 1:
- Disable validation errors: YES|NO
- Validate form fields: EMPTY
result: SUCCESS


Test case 2:
- Disable validation errors: YES|NO
- Validate form fields: NO EMPTY
result: ERROR
TypeError: Argument 2 passed to Drupal\Component\Utility\NestedArray::getValue() must be of the type array, string given, called in /var/www/html/web/core/lib/Drupal/Core/Form/FormValidator.php on line 154 in Drupal\Component\Utility\NestedArray::getValue() (line 69 of /var/www/html/web/core/lib/Drupal/Component/Utility/NestedArray.php)

Thanks for reviewing this. Will take a look what might be wrong with test case 2.

Test case 2 should be now fixed too. That case included a subset of fields to validate on Ajax submits. The '#limit_validation_errors' array expects an array of arrays (these are sections in the form that are being mapped to #parents element keys), and until now it was wrongly built up with an array of strings.

I've merged in the current state of 1.2.x into MR301.

• MR301 is for 1.2.x
• MR302 is for 1.1.x (backport)

I've made a couple of small updates to MR301 and left a few comments. Haven't looked into the backport yet, I guess we keep that for later, when MR301 is being completed.

Thanks for your notes. I took a second look into them, and carefully resolved them. I cherry-picked your commit into the 1.1.x backport branch and added an isset check into the finally block.

You've mentioned some concerns regarding the reference variables and the access of protected properties and methods (where I'm using the \Closure). I'm aware that especially the latter one may not be best practice, but I don't see any other way to solve it like this. And currently all tests are green, plus our manual tests should approve the changes. Of course there may be more problems in theory, but if there is really one, please feel free to add a test that proves the problem.

Found another small coding improvement to probably be made and have some follow-up question on one of the threads.

Regarding jumpToFirstFieldChild I need to think about a little more and will get back to it later. I'm not yet happy with the current state as it is unclear what it does and why it needs to be done that way. Getting back to such code in a couple of months will be a nightmare, when nobody recalls what was going on. It doesn't get better just because it was in there already before the current MR.

Looks like this issue is being blocked for reasons I don't understand. Maybe I also don't understand the scope of this issue (which is about fixing a bug). Yes, there are implementations involved here that don't look good. But I don't see a concrete problem. As I don't understand what's going on here, I need to bypass this and give up.

I'm sorry that my comment #17 left room for interpretation. I won't post subjective statements again.

Carefully resolved all threads, feel free to re-open / continue discussion.

Regarding jumpToFirstFieldChild I need to think about a little more and will get back to it later. I'm not yet happy with the current state as it is unclear what it does and why it needs to be done that way. Getting back to such code in a couple of months will be a nightmare, when nobody recalls what was going on. It doesn't get better just because it was in there already before the current MR.

I suggest to treat this one as a follow-up issue.

This looks great now, thanks @mxh for doing this loop as well. For the jumpToFirstFieldChild topic, I've created 📌 Refactor \Drupal\eca\Plugin\FormFieldPluginTrait::jumpToFirstFieldChild Active , so we can ignore that here.

The solution in FormBuildEntity is much better now, thanks a lot. Only one remaining question: now that we only have try / finally this mean that the function could still throw an exception, but would still execute everything in the "finally" block. Is that what you intended here? I'm asking because if no exception at all could be thrown in this context, then the try/finally would be redundant.

WRT MR302, does that also require a review with these details?

Leaving the status on NR for now, as it does not require more work right now, but review probably hasn't been completed yet either.

now that we only have try / finally this mean that the function could still throw an exception, but would still execute everything in the "finally" block. Is that what you intended here?

The finally block is simply there as a security consideration. If - for whatever reason - an exception is being thrown inside the simulation of the form submit, then this finally block makes sure, that the "real" form state won't contain any error from the simulated one. That's what the comment inside the finally block indicates: It makes sure that the state is being properly reset to its original state. And the finally block does not do anything more than that. Not every possible exception is documented because there is always a risk of a runtime exception, especially when it comes to form processing, which may involve building up render arrays. Therefore, we should stay with the finally block.

WRT MR302, does that also require a review with these details?

No it's not necessary, it contains identical commits to MR301, except that I had to manually resolve some merge conflicts. I've created that backport branch so you don't have to spend time resolving the merge conflicts when backporting.

That's what the comment inside the finally block indicates: It makes sure that the state is being properly reset to its original state. And the finally block does not do anything more than that.

Yes, I understand that. Just wanted to make sure that it's OK that a potentially thrown exception would still be thrown upstream after the finally block has been completed. I mean to recall that you wanted to prevent that from happening, but my memory may be wrong.

Setting to RTBC now and starting to merge.

Successfully merged both MRs.

Automatically closed - issue fixed for 2 weeks with no activity.