ECA Form breaks complex IEF widget

The error is caused by \Drupal\eca_form\HookHandler::inlineEntityFormAfterBuild. When I disable that callback by inserting a return $form; into the first line of it, the error disappears.

The problem is that IEF is not creating the correct embedded entity, at the same time there are no error messages or exceptions whatsoever.

Also, selecting an existing entity in such complex IEFs doesn't work either. See also 🐛 Nested IEF (complex form) stopped working for inner form Active .

A matter of fact, IEFs with the simple widget are not affected.

I'll start with an MR to disable the hook handler for complex IEFs so that people can use that patch until we've found a proper solution for this.

Turn out, when we revert the "bugfix" from 🐛 An error occurred while trying to build an inline entity from submitted form values Fixed then the issue disappears as well.

I've now disabled that code block from the other bug fix, but only for complex IEF widgets. This is just a workaround, but that can be applied to avoid that issue for now.

I have now disabled that complex widget hook entirely until we get a solution for this.

Turns out, disabling complex widgets entirely breaks some existing models, that otherwise with the workaround from the MR would be OK.

So, we're now merging and back porting the MR, publish another bug fix release and then hope that we can still find a proper solution for the underlying problem. But at least we would not be breaking anything in the meantime.

@jurgenhaas

Upgrading ECA from version 1.x to 2.x on Drupal 10.3.7 generates a warning related to the use of eca_form alongside inline_entity_form 3.x when initializing content entities using the complex widget with AJAX. The warning message displayed is:

“An error occurred while trying to build an inline entity from submitted form values. This might be a problem if you are using ECA to extend inline entity forms. Please report this to the ECA issue queue to help us improve it.”

Upon investigation, it appears that the condition in the code checks for:

if ($form['#eca_ief_info']['widget_plugin_id'] !== 'inline_entity_form_complex') {

However, in our case, we are using the IEF Table View Mode module https://www.drupal.org/project/ief_table_view_mode → . I would like to suggest modifying this condition to use a regular expression that matches inline_entity_form_complex, which would also account for custom plugins. This change could help ensure broader compatibility.

lexsoft → changed the visibility of the branch 3469697-eca-form-breaks to hidden.

I'm having the same issue as @jurgenhaas described in https://www.drupal.org/project/drupal/issues/3405115 🐛 Argument #1 ($values) must be of type array, null given. Postponed: needs info
Is there a Drupal core patch fix for it?

We are hitting this issue on a bunch of sites -- or rather 🐛 Integrity constraint violation when trying to add multiple blocks in a complex IEF field Active , which I did not think ECA had anything to do with until someone left a comment there about ECA. All of the affected sites have ECA enabled. Here's the gist of what we're doing:

1. Install IEF, ECA Form modules
2. Create a "featured block" entity reference field on a content type, e.g. "Basic Page", referencing content blocks.
3. Set the field to use the complex IEF widget
4. Create a new page, and create a new block using the IEF widget.

If you save now, everything is fine. If you edit the page and add another block, it works fine -- so long as you only ever add one block at a time.

5. If you add a second block without saving the node, it appears correct -- it adds another row to the IEF widget, and you can keep adding more.
6. But if you save, you get the Integrity Constraint Violation, duplicate entry 'xxx' for key 'node__field_uuid_value'.

This is affecting node and taxonomy term references as well as block references -- anywhere the IEF complex widget is used with multiple cardinality, if eca_form module is enabled. Doesn't matter if there's an ECA model using it.

Thanks @freelock for the analysis. I will have to debug this, and it reminds me of 🐛 Entity Form: Build Entity can't process unlimited values Active which also has some strange behaviour for multi value fields.

That issue does sound very similar. One difference is that we were seeing the issue with limited cardinality, too -- although a cardinality of 6... but I'm guessing if ECA is replacing the form_state for some reason that might do it...

I tried to fix this several times with no success. Complexity of Form API plus complexity of IEF makes this impossible for me to find a way to properly resolve this. Also it was a terrible mistake trying to manually rebuild an entity from a simulated form submission in "Form: build entity". Since it looks like no one is able to fix this critical bug, we may want to consider dropping support for IEF (and Paragraphs) entirely from ECA 3.x, and only stick with core's Form API events?

Given that IEFs are always broken with this active, disabling it until it actually works makes total sense to me.

That said, I just spent a little time debugging further, and I think I've at least identified the cause of the issue -- but no solution.

What I'm seeing is that during an Ajax call to refresh the form element, when opening a specific row in IEF, the $context['delta'] is always set to 0 no matter which delta you're actually trying to edit. IEF assumes that the delta is set for the row being edited, but this is not true -- pretty much ever.

It does end up correct like a stopped clock is correct twice per day -- it's correct if the first row edited is the row you're currently editing.

Here's where the delta gets set to zero:

... this is because the delta is not passed in EntityFormDisplay::buildForm to InlineEntityFormBase->form(), so $get_delta is null, and then defaults to 0 in WidgetBase->form().

Meanwhile, by the time we get to the ECA HookHandler->fieldWidgetSingleElementInlineEntityFormComplexFormAlter method, we expect to have the $context['delta'] set correctly, and the entity available in the FormState originally set up under 'eca_ief' -- but it always stores the same entity under the same 0 delta.

So I went to see how IEF determines the delta, and found this:

$delta = $form_state->getTriggeringElement()['#ief_row_delta'];

Plugging that into the ECA hook handler, seems to work! At least for existing rows. If that delta comes back NULL, and we didn't return early, you can't get a new blank form. So I've added a check for if $delta is null, and this at least makes IEF act normally.

Is there a test scenario that actually uses this ECA? I think the MR I'm adding at least fixes IEF, and I would assume would work for alters that act on existing entities -- not sure what scenario might need access to a non-existing entity.

Ok hope I did that right -- I had to merge in changes from the main dev branch and resolve a conflict before it showed mergeable. But this at least fixes the conflict with IEF. Not sure if it breaks any ECA functionality.

Thanks for having another look at this. Tests are not passing yet, thus setting to NW for now.

Not sure if it breaks any ECA functionality.

Given from what we've tried in the past at this place and given that the $context variable once held a delta that worked on our sites (but not anymore?) I guess either way something will change or break at a different place.

Hi,

Ok. So this all made me set up phpunit and figure out how to run these tests, something I've neglected up to now. Got that working, but... I don't understand this test?

It's failing because the "Title" field of the referenced content in the inline entity form is not disabled. But this doesn't make any sense -- most of our uses of inline entity form allow you to edit the title of the referenced entity, and even without the patch applied, the title is editable -- the test is checking for a "#disabled" value that appears to have no effect on the UI. It is set without the patch, but titles are still editable. With the patch, this value is not set -- and the UI is identical here.

So I think the test is invalid -- it doesn't correspond to what it claims to be testing, and what it claims to be testing is not even a relevant test.

I think the test does show that there was no entity attached to the form -- which would be correct, because the form state would not include the IEF triggering event. But we know the old code attaches the wrong entity if you are trying to add an entity using IEF.

We need a better scenario to test.

Ok. Without changing what is actually being tested, I updated the test to set the delta the way IEF actually sets it, so the test now passes (because it is loading the referenced entity into the form, now that the test specifies the delta).

I don't think this is a very good test though...

Regarding the mentioned test: reading from the code it's using a paragraph provided by the Paragraphs → module in an article node and it asserts whether the title field got disabled. You may have missed that the test class is installing the eca_test_inline_entity_form which includes an ECA configuration that disables the title field of the paragraph: https://git.drupalcode.org/project/eca/-/blob/2.1.x/tests/modules/inline...

... this reflects a pretty often used scenario, which is to disable fields in forms.

The test failure proved that the suggested changes in the code break currently existing functionality. The test is using Paragraphs as module, and not Inline Entity Form. The Paragraphs module does not use IEF at all, it has its own implementation of "inline forms". It doesn't make sense to change the test in the suggested way because then it would be "tricked" to have some IEF logic in there.

I recommend to remove the suggested change in the test itself and try to make it green again without changing the test itself.

Ah, ok, thanks for the further information -- that explains why the test is checking that the title field is disabled.

But I think you're mis-understanding the test setup -- the test name is InlineEntityFormTest -- but confusingly, it is testing BOTH InlineEntityForm and Paragraph forms:

    $this->assertTrue($form['field_pages']['widget'][0]['inline_entity_form']['title']['widget'][0]['#disabled'], "The model has set the title field to be disabled.");
    $this->assertTrue($form['field_paragraphs']['widget'][0]['subform']['field_text']['widget'][0]['#disabled'], "The model has set the field_text field to be disabled.");

... the first test checks field_pages, which is set up as an entityreference field to a "page" node, the second is an entityreferencerevision field set to a Paragraph.

The second test passed before and after I changed the test -- the first was failing, because the test was not loading the IEF widget (because of the delta issue).

So I think the test has always been incorrect for the IEF field, and now it's correct (at least for testing that ECA is making the title field of the first referenced item disabled). It's just that IEF itself does not use the normal delta at all, and relies upon this special form_state property instead.

Setting back to NR.

Ok, the test is testing for both and yes the naming of the test could have been improved in that regard.

But there is no triggering element when a form is not submitted. The model is disabling the field when the form is being built. That's why the suggested change in the test doesn't make sense and the current suggested change in the code is incompatible with Paragraphs. The test is only green because the triggering element with that special IEF key was manually set by the test itself. That's not what's happening when building a form though.

Once again setting to NW as we still need to get the test green without changing it. The test itself was fine.

So the code should maybe check whether that special IEF key exists - if yes use it, if not then fallback to that delta key set by the context variable.

Ok, re-reading once more to make sure I've not missed something and I clearly see that we have different arguments why the test was failing. The main problem I see is the current suggested code. It must be changed, otherwise it will not be compatible with Paragraphs anymore. See my comment in #32 why.

Here is the current suggested code:

$delta = $form_state->getTriggeringElement()['#ief_row_delta'];
    if (is_null($delta)) {
      return;
    }

Before, it was using $context['delta'], which is what at least I know for sure this worked with Paragraphs. And it least worked for the first item of an IEF element.

Now it's just using that IEF key and that will for sure not be available with Paragraphs, and for sure it won't be available when there is no triggering element. $form_state->getTriggeringElement() may return NULL, especially when the form is not submitted.

Let's first try to make the suggested code change not break Paragraphs and still work when form is not submitted. Then let's see whether the test needs a possible adjustment. I'm quite sure the test was fine, because it rightly tested whether both fields were disabled by the ECA configuration.

Paragraphs has an entirely different method in HookHandler:

... 1 is what gets executed for IEF, where the code change is. I'm pretty sure Paragraphs uses 2 instead.

Maybe we should split this into two different tests, one for IEF, and an entirely new one for Paragraphs?

Even when split, I don't understand why this is a valid change in the existing test:

// IEF delta is found using the triggeringElement --
// without this, no default entity is loaded.
$form_state->setTriggeringElement(['#ief_row_delta' => 0]);

It doesn't make sense because a triggering element does not logically exist when a form is not submitted. Can you please explain?

... and going back to the original problem, as I see it with IEF, if there is no triggering element, then the subform doesn't load at all, so ECA can't alter it.

It's a fundamental problem that the test does not properly test IEF -- instead before this update ECA is arbitrarily attaching whatever entity it sees to the form, which is exactly why it breaks IEF. The "complex" widget uses Ajax to load the inner form, and the triggering event to determine which delta to load -- if there is no delta, and you load an existing entity, it also breaks. I'm not sure if this prevents ECA from acting on it -- we might be able to create a new entity of the appropriate type to add.

The "complex" widget uses Ajax to load the inner form, and the triggering event to determine which delta to load

The testing module is using the simple IEF widget and not the complex one. So the test was properly covering the use case for simple IEF and now breaks with the current suggested changes.

Here is the part that contains the simple IEF widget configuration: https://git.drupalcode.org/project/eca/-/blob/2.1.x/tests/modules/inline...

Keeping the scope of this issue, you could create a separate new test for complex IEF widget instead of changing the existing one.

The "complex" widget uses Ajax to load the inner form, and the triggering event to determine which delta to load

I gave an explanation above why it doesn't make sense to have a triggering element when a form is not submitted yet. You could be right, that on IEF complex widgets there is an Ajax submit handler involved. If so, do you know the place in IEF where exactly this happens?

We could then simulate that behavior for a complex IEF, but as said, it would need to be a separate new test besides the test for the simple widget.

... to reach this conclusion, I spent a chunk of time debugging to find that the $delta is always set to 0 when the ECA hook handler gets called, regardless of the number of items in the field. So I went spelunking to figure out what IEF uses, and ended up here:

https://git.drupalcode.org/project/inline_entity_form/-/blob/3.x/inline_...

Thanks for the link. I have checked the linked callback and it turns out this is exclusively added as a submit handler to 3 submit buttons inside of IEF. Example:

$row['actions']['ief_entity_edit'] = [
            '#type' => 'submit',
            '#value' => $this->t('Edit'),
            '#name' => 'ief-' . $this->getIefId() . '-entity-edit-' . $key,
            '#limit_validation_errors' => [],
            '#ajax' => [
              'callback' => 'inline_entity_form_get_element',
              'wrapper' => $wrapper,
            ],
            '#submit' => ['inline_entity_form_open_row_form'],
            '#ief_row_delta' => $key,
            '#ief_row_form' => 'edit',
          ];

So it would be only available when one of the three submit buttons was clicked. I don't see any Ajax or JS behavior that automatically submits one of these buttons so I suspect that this key is not available when the form is not yet submitted. And in fact, the existing test does confirm this. It's only green when the key is manually added as highlighted in #35.

If there is no place that shows that this is triggered automatically, then the test definitively remains valid as it was before and we need to find a different way.

I recommend once more to revert the change in the test and instead try to adjust the suggested code change so that it also works for the simple IEF. I think it's worth trying what I've mentioned in #32: So the code should maybe check whether that special IEF key exists - if yes use it, if not then fallback to that delta key set by the context variable.

Ok, got it working for both the simple IEF widget AND the complex one. It's now checking for the specific plugin in use -- should we throw an exception if it's a different plugin? (Since we won't know how to handle it...) For the time being it's just returning unless it's the simple or complex widget.

If it's the simple widget, it works the same as before -- and I've verified that at least the test model works, it disables the title field on the IEF form, for all deltas and new ones.

For the complex widget, to get it to work I had to move the delta retrieval above the section that finds the right form to use -- the original fix worked for the first existing item in the list but not new items or any beyond the first.

Getting the $delta set above the part where the $entity_form is found fixed other deltas, but not adding new ones. Setting the $delta to the length of the EntityReferenceList (so it's always 1 bigger than the current length) made it work for new items as well.

So this is now working for me -- not sure how to set up a test for the Complex widget.

It's now better since it's not breaking the existing test anymore. I don't think we need to specifically check for the plugin being used. It's probably fine to just do this: "So the code should maybe check whether that special IEF key exists - if yes use it, if not then fallback to that delta key set by the context variable."

We could additionally preliminary return and/or log an error like it was done before when even the fallback delta key from the context variable doesn't exist.

Setting to NW because of some PHPCS complaints. We should also have a test covering this, but since it's a bug report the can see first whether the code change fixes the bug and doesn't break existing tests.

Fixed phpcs issues.

This looks reasonable to me, I only had a couple of tiny code style fixes and applied them to the MR right away.

Setting this to RTBC, the changes just make sense. But I'd like to get @mxh's opinion on it as well before merging it.

@freelock thank you so much for diving that deep into this topic, which is really not so pleasant since form API is so vaguely defined, and IEF doesn't make this any better, just to the contrary.

We don't need to restrict by plugin IDs. See my comment #42 first two sections for details. tldr:

• We don't need to check for the plugin IDs. We basically just need to check for the existence of that special IEF key and otherwise fall back to the delta from the $context variable.
• When we still don't get any delta for some reason we should log it as an error in the same way it was done before.

@mxh sorry, I don't fully understand what you're saying in #45. It looks to me that we now have different approaches if it's a simple or a complex IEF. It doesn't look like we can just lookout for the inline_entity_form key. But I may have gotten this wrong? Maybe you could add a code suggestion into the MR so that we can see what you're proposing?

The current merge requests prints out following error on my test site (using the steps to reproduce as described in the IS):

Warning: Trying to access array offset on null in Drupal\eca_form\HookHandler->fieldWidgetSingleElementInlineEntityFormComplexFormAlter() (line 275 of modules/contrib/eca/modules/form/src/HookHandler.php).

Also when applying the merge request, the error is still the same on my site. My steps to reproduce

• Drupal with standard installation profile
• Enabled modules eca_form, inline_entity_form, bpmn_io, eca_user, eca_base, eca_content
• Added entity reference field on basic page to Article content type with cardinality = 6
• Configured using complex IEF widget on that entity reference field in basic page's form display configuration, allowing users to create new content.
• Wen editing an existing basic page and now try to create new article within that basic page form -> the first one succeeds
• Now try to create another article in there -> you will see that field values from the first submitted one are still there - this is not happening when eca_form is uninstalled
• Anyways no error shown yet, but try to save the whole basic page form now -> you will see integrity constraint violation as fatal error.

Getting this same error either when running on 2.1.7 or when applying the merge request.

The results you got are exactly what we see without this patch.

Are you sure you're applying the correct MR (487), and not the old one that was already merged?

Are you sure you're applying the correct MR (487), and not the old one that was already merged?

Warning: Trying to access array offset on null in Drupal\eca_form\HookHandler->fieldWidgetSingleElementInlineEntityFormComplexFormAlter() (line 275 of modules/contrib/eca/modules/form/src/HookHandler.php).

https://git.drupalcode.org/project/eca/-/merge_requests/487/diffs#bd67f3...

Also the main logic for rebuilding the entity for complex IEF widget hasn't been addressed at all, but that was the original changed part in scope of this issue: https://git.drupalcode.org/project/eca/-/blob/2.1.x/modules/form/src/Hoo...

So from the code logic itself the complex widget is being excluded, I wonder why the delta fix would suddenly resolve that part. It doesn't make sense.

I have committed into the MR my suggested logic which works on my test site now.

Even when that would seem to work now, still this issue needs to stay on NW, because of what I mentioned in #50. The code logic there is not enabled for complex widgets. The linked commit belongs to this issue: https://git.drupalcode.org/project/eca/-/commit/6e183c9ebefb8f8b7331b8e9...

So we need to clarify what we need to do with that code part.

So aside from detection logic, it looks like the one substantive change in your MR commit is when adding a new item to the list, you're checking the $form_state '#ief_id' element entities and counting those, and falling back to $context['items'] if that isn't set. I'm wondering what scenario leads to that being correct -- I thought I saw some of those lazy-loaded, so I thought $context['items'] seemed more reliable... is that where you're getting null instead? I'm thinking this might be due to a difference in the field or widget config...

As for comment #50 I don't know what the goal of that is -- it looks like it's triggering validation? That doesn't appear to be required to fix the complex widget -- but maybe necessary to support models that add validation to the subform?

When the current state of the MR487 doesn't work on your site, please provide steps to reproduce so we can have a look at it together and see for a possible solution. Best starting point would be Drupal with a standard installation profile and list additional modules you've installed. I have provided steps in #47 to reproduce the problem I had with the previous state of the MR and pointed to the place where the error is happening in #49.

As for comment #50 I don't know what the goal of that is

I won't lookup the code now but I am certain it was used, otherwise the code wouldn't exist. So we need to clarify in what way we need to address it, as that particular one was originally changed within the scope of this issue. It seems we've been looking at a different problem with MR487 and should have addressed it in a separate issue. Probably one of the reasons we have some confusion here.

I'm wondering what scenario leads to that being correct

When either using the simple widget or possibly when using any other widget implementation for IEF.

I thought I saw some of those lazy-loaded, so I thought $context['items'] seemed more reliable... is that where you're getting null instead? I'm thinking this might be due to a difference in the field or widget config...

I got this error with the steps described in #47. Letting the user create a new node was enabled, but that should've been enabled by default anyway. So I basically changed nothing in the default widget config. Are you using different settings there?

The steps I used were basically:

1. Install Drupal CMS, and install the "project" and "case study" recipes, and IEF. (also needed paragraphs and err for the tests).
2. Add an entityreference field on project type to case study with unlimited cardinality
3. Configure it to use the complex IEF widget, and allow creating new entities/reference existing entities
4. Edit a project node and add a new case study using the widget
5. Without saving yet, attempt to add another case study -- and see that the IEF form has the same details as the first case study
6. Save the project node, and you get a whitescreen.

I did have to change the config on the IEF widget -- I think to allow adding existing entities? I have them set to both allow new and existing, which was not the default.

With my patch, you get the correct case study loaded when you edit, and if you add a new entity it gives you a fresh blank form.

Configuring an ECA model to alter the IEF and disable the title field works correctly everywhere as well.

I'm on vacation this week, so won't be spending time on this until next week...

We do see this on 6+ entirely different sites. I wonder if the difference between your setup and my vanilla CMS setup is cardinality... I did debug/develop this patch on the vanilla Drupal CMS site, haven't applied it yet to the other broken sites, most of which do limit cardinalilty.

Until now, the starting point as mentioned in #47 is always:

Drupal with standard installation profile

Just using the current stable versions of Drupal core and every most recent version of involved contrib module at this time.

Maybe there's a difference in the combination of the theme being used, or even other contrib modules involved since Drupal CMS involves a lot more. So I recommend using Drupal core with standard installation profile as a common basis for this issue.

With my patch, you get the correct case study loaded when you edit, and if you add a new entity it gives you a fresh blank form.

Since the previous state of the MR didn't work for me as described in the previous comments, and I took the time to make it work for the standard installation profile (as mentioned), taking care that it doesn't break existing test coverage, the more interesting question to answer now is, whether the current state of the MR also fixes the problem with the steps to reproduce as you described in #55.

> the more interesting question to answer now is, whether the current state of the MR also fixes the problem with the steps to reproduce as you described in #55.

It is working fine after your changes.

This looks good to me and reading the comments sounds like it's RTBC. Any objections?

So far applying this patch has fixed the issue on each of the sites we've touched, so I'm good with that!

Please see #51 🐛 ECA Form breaks complex IEF widget Active

Regarding #50, it doesn't seem like any of us understands what that code block is for, and there doesn't seem to be any tests for it either.

I suspect it's triggering some validation functionality -- but as it is it is not silently breaking issues on existing sites -- it's already not functional -- and it has nothing to do with the current title of this issue, "ECA Form breaks complex IEF widget" -- I would think the comment in #50 would be for a different issue, say "validation not performed on IEF complex widget subforms" or something like that -- but it seems possible that's not even true, in my debugging the subform was hitting a different section of this code, so maybe that's only necessary for simple IEF widgets already?

Not having this fix in means there's an unknown number of sites actively broken, simply by having the eca_form module enabled and a complex IEF widget anywhere on the site. It took months for me to even track down that eca_form had anything to do with the broken IEF form.

Can we create a new issue for that code, and invite somebody to create a test around whatever it's trying to do?

The code part I linked in #50 is referring to this issue. The code change was applied in the scope of this issue. Therefore, this issue is about clarifying that code part.

I now take the time to explain what the code part does that was disabled within the scope of this issue. Here is a comment that describes what it does: https://git.drupalcode.org/project/eca/-/blob/2.1.x/modules/form/src/Hoo...

Only needed when coming from the inline_entity_form module, because the embedded entity does not automatically hold most recent form input.

That means, the embedded entity is not automatically updated on its values when further Ajax is happening on the form. That code part has made sure that the entity values were updated in order to properly work with most recent submitted input used on the entity.

Here is the comment that refers to this issue: https://git.drupalcode.org/project/eca/-/blob/2.1.x/modules/form/src/Hoo...

So as long as that code part has not been clarified and properly fixed, this issue is not fixed.