Problem/Motivation

inline_entity_form + layout_builder + entity_reference_revisions

Fields are revisionable, new revision is on.

So once you opening existing whatever (block in our case which is represented by entity_reference_revisions)
if you click any "update" button - all is OK.
But if you click "Cancel" results will go to upper conxtext entity_reference_revisions with ['target_revision_id'] === ''.
And this is because of modules/contrib/inline_entity_form/src/Element/InlineEntityForm.php::processEntityForm() having a lines

    // Handle revisioning if the entity supports it.
    if ($entity_type->isRevisionable() && $entity_form['#revision']) {
      $entity_form['#entity']->setNewRevision($entity_form['#revision']);

which enforce ['#entity']['revision_id'] to be erased at the begining of lifecycle where process where form is build.
Unlike "update" buttons "cancel" does not invoke

$inline_form_handler->save($entity_form['#entity']);

which populates empty revision_id with new value, so page gets broken.

We have tested this bug down to v1 of inline_entity_form till 2023 - it is present in all versions.

Steps to reproduce

Described above +
steps

Proposed resolution

not to $entity_form['#entity']->setNewRevision($entity_form['#revision']); if triggering_element is 'cancel'.
Thanks Mindaugas Svirskas (AKA @mindaugas-svirskas AKA @msfranklydk ) for suggesting and testing this simple and working idea.

As a patch https://git.drupalcode.org/issue/inline_entity_form-3543315/-/commit/ffd...

Remaining tasks

none.

User interface changes

none.

API changes

none.

Data model changes

none.

CommentFileSizeAuthor
e53cf445-beb7-4f4d-b2b2-4289c0eef5b6.png71.25 KBmykola dolynskyi

Issue fork inline_entity_form-3543315

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mykola dolynskyi created an issue. See original summary.

mykola dolynskyi’s picture

mykola dolynskyi
Primary language Ukrainian
Location Poltava
commented
Issue summary: View changes

mykola dolynskyi opened merge request !145

benstallings’s picture

benstallings commented
Version: 3.0.0-rc21 » 3.x-dev
Status: Needs review » Needs work

Claude Code says:

Concerns

1. Accesses a form-system internal instead of the documented API. Every other getTriggering* call in this module uses $form_state->getTriggeringElement() (see EntityInlineForm.php:266, ElementSubmit.php:76, InlineEntityFormComplex.php:708/981/1002). This commit is the only place reaching into
$form_state->getUserInput()['_triggering_element_name'], which is a FormBuilder internal key — not stable API. Should be:

  $trigger = $form_state->getTriggeringElement();
  $is_cancelling = isset($trigger['#name']) && strpos($trigger['#name'], '-cancel-') !== FALSE;

2. Substring match is too loose. strpos(..., '-cancel-') matches any button name containing -cancel- anywhere. That covers IEF's own cancel buttons (ief--cancel-, ief-remove-cancel-, ief-reference-cancel-), but also any custom button from the outer form or a contrib module that happens to contain that substring. Should anchor to ief- prefix at minimum, or enumerate the three known names.

3. Scope is the whole form, not the cancelled element. buildEntityForm() runs for every IEF element on the form during rebuild. If there are multiple IEF widgets and the user cancels one, this suppresses new-revision setup on all of them — including forms the user is still actively editing. May or may not cause user-visible problems depending on how ERR extracts values downstream, but it's the wrong unit of granularity. The check should be "is this element the one being cancelled," not "is any cancel happening."

4. PHP idiom is dated. isset($x) ? $x : '' → $x ?? ''. Module already requires PHP 7.1+.

5. Fix lives at the wrong layer. The root cause is that Cancel's submit chain (closeForm / submitCloseRow / inline_entity_form_cleanup_row_form_state) tears down widget state but the subsequent rebuild still re-runs buildEntityForm, which unconditionally calls setNewRevision(TRUE). More targeted fix ideas:
- Have the cancel submit handlers clear the entity from widget state so the rebuild doesn't re-enter buildEntityForm for it, or
- Skip new-revision setup only for the specific #ief_row_delta being cancelled (the cancel buttons set #ief_row_delta), or
- Fix the ERR-side extraction to not read a zeroed revision ID during cancel-triggered rebuilds.

6. Comment style. Drupal coding standards: sentences start uppercase and end with a period; 80-col limit on comments. Four of the five new comment lines violate one or both (lowercase "in case", "because", long URL line, "'cancel' should match" is stale — the actual string is -cancel-).

7. No test. tests/src/FunctionalJavascript/ComplexWidgetRevisionsTest.php is the obvious home for a regression test covering "open edit row, press cancel, verify target_revision_id is preserved."

8. Commit message is misleading. "revision_id is submitted as empty" — but this fix is in the build path, not the submit path. The commit body should explain that cancelling causes a rebuild where setNewRevision(TRUE) corrupts the revision pointer used by ERR during subsequent value extraction.

Recommendation

Don't merge as-is, but the fix direction is probably correct. Reasonable path to land:
1. Swap to $form_state->getTriggeringElement()['#name'] ?? ''.
2. Tighten the match — either exact names or at least prefix with ief-.
3. Decide whether to fix per-element (better) or accept the whole-form scope with a documented tradeoff.
4. Fix comment style.
5. Add a regression test in ComplexWidgetRevisionsTest.php using the ERR-backed fixtures.

benstallings’s picture

benstallings commented
Assigned: mykola dolynskyi » benstallings
benstallings’s picture

benstallings commented
Assigned: benstallings » Unassigned
Status: Needs work » Needs review

dww made their first commit to this issue’s fork.

dww’s picture

dww
we/he/they
commented
Status: Needs review » Needs work

Thanks for working on this! Sounds like a major bug. Was hoping to merge this before the 3.0.0 release. However, the test-only job still passes, so the test needs work. Left a review on the MR with some other threads.

If it's ready, this can definitely go out in 3.0.1.

While I very much appreciate the effort on triage, reviews and MRs from @benstallings, if you're going to use the addictive and destructive LLM technology, I'd rather hear your summary of what it's telling you than having you parrot its output as d.o comments. It's bad enough that all these models are scraping d.o as the source of truth. Once more and more of that content is verbatim LLM output, the robots start trying to "learn" from themselves. 😬 Please be human, disclose that you're using an LLM, but put in some cognitive effort on the results and tell us all what you think we should know. 🙏

Thanks again!
-Derek