The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.Problem/Motivation
This issue originally surfaced in Entity Browser where, in certain configurations used with Inline Entity Form or Layout Builder, there can be unexpected results:
#2764889: Entity Browser widget loses selected images in inline entity form
#3046416: Remove button conflict wrong triggering element
#3104901: Entity Browser used in a entity referenced field of a layout builder custom block is not working
But it seems the underlying issue may actually be a core FormBuilder bug.
Comment #7 in #2764889: Entity Browser widget loses selected images in inline entity form documents what's happening:
I have investigated further what makes inconsistent form state -> and I have ended in IEF module.
So what is going on (in "short"): for example when remove button is pressed without IEF
- in processing of action (FormBuilder::processForm()), rebuild is triggered -> FormBuilder::rebuildForm() and then FormBuilder::retrieveForm() -> that will execute EntityReferenceBrowserWidget::formElement()
- setting of new state is done in EntityReferenceBrowserWidget::formElement()
- and after FormBuilder::retrieveForm() in FormBuilder::rebuildForm() form state will be saved (cached)
- so we end with correct form state (when I say form state I mean form state relevant for Entity Browser)
Other case: for example when remove button is pressed within IEF
- in processing of action (FormBuilder::processForm()), rebuild is triggered -> FormBuilder::rebuildForm() and then FormBuilder::retrieveForm() -> that will execute creating of IEF form and -> it will generate #process callback for inner form
- and after FormBuilder::retrieveForm() in FormBuilder::rebuildForm() form state will be saved (cached)
- after saving of form state inside FormBuilder::rebuildForm() -> FormBuilder::doBuildForm() will be triggered. That will pick up all defined #process callbacks and execute them
- execution of defined #process callback will execute EntityReferenceBrowserWidget::formElement() and new form state will be set
- But!!! Problem is that form state is already saved (cached) and form state set during execution of #process callback will not be saved and actions after that will have wrong form state
Inline block forms in Layout Builder work similarly to IEF, in that the entity form is built in a #process callback (see \Drupal\layout_builder\Plugin\Block\InlineBlock::blockForm()
The root issue seems to be that in \Drupal\Core\Form\FormBuilder::rebuildForm(), the form and form state is cached before the call to doBuildForm, where the #process callbacks are run. If we compare how the form cache is set in rebuildform() to processForm, we can see that in the latter method, caching is done after the doBuildForm() call, using a copy of the form array created before the doBuildForm().
While the identified issues are specific to Entity Browser, this could affect any code where form state storage is updated in #process callbacks.
Steps to reproduce
See #13
My setup is Drupal 10.1.7 with PHP 8.1.
The patch solved this bug:
1. Create a custom block with a required reference field, e.g. for nodes
2. Configure its form display to use an entity browser to display the field. Display the Remove button.
3. Configure the entity browser to use modal windows.
4. Edit the layout of a basic page and add the custom block.
5. Use the entity browser to add a couple of nodes.
6. Remove one of the nodes.
7. Remove another one. Instead of removing the node, the modal window to add more nodes opens.It also solves this bug:
1. Create a custom block with a required reference field, e.g. for nodes
2. Configure its form display to use an entity browser to display the field.
3. Configure the entity browser to use modal windows.
4. Edit the layout of a basic page and add the custom block.
5. Use the entity browser to add a couple of nodes.
6. Try to save the block, but provoke a validation error, e.g. by saving the block without a title. The Layout Builder is reloaded, but the previously selected nodes are not displayed anymore. They are back again if you add another node or if you simply save the block without seeing them.
Proposed resolution
Use similar form caching code in rebuildForm() to what is done in processForm():
- Save a copy of the form array before doBuildForm() call
- Call doBuildForm()
- Cache if necessary with unprocessed copy of form array saved before doBuildForm()
- return built and processed form array
Remaining tasks
Write patch and tests.
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #33 | 3177977-nr-bot.txt | 90 bytes | needs-review-queue-bot |
| #24 | 3177977-nr-bot.txt | 1.86 KB | needs-review-queue-bot |
Issue fork drupal-3177977
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:
changes
Comments
Comment #2
godotislateHere's a patch - running tests to see if anything breaks.
@todo Add test to demonstrate bug.
Comment #4
tlo405 commentedI was having issues with a required entity reference field in layout builder, and this patch fixed the problem for me. What the patch is doing makes sense to me and the code looks good. I know this still needs tests, but I would like to get some more feedback on this approach.
Comment #5
effulgentsia commentedNice find! I had to do some archaeology to see why we have the inconsistency to begin with.
I think part of it is that initially (2009 and earlier), we did not intend for #process callbacks to put things into $form_state['storage'] (back then we had explicit
$form_state['storage'], whereas now it's all stored except for$form_state->setTemporaryValue()). So, thesetCache()inbuildForm()did not include$form_state, and only the one inrebuildForm()did, and that was cached prior to invoking #process callbacks.#302240-25: button broken due to fix various problems when using form storage started including $form_state in the pre-rebuilt setCache(), and then #634440-33: Remove auto-rebuilding magic for $form_state['storage'] changed rebuilds to not be auto-triggered whenever there's new $form_state to store. Between those two changes (and maybe others as well), we got into the situation of divergent code paths between caching the initial form build and caching the rebuild.
I want to do some more research on why #process callbacks should (or shouldn't) change $form_state in a way that requires persistence. But if we accept the idea that they do (and have good reason to), then I think what #2 proposes is correct.
More than a decade after I first wrote that @todo line, I wonder if it's feasible to fully resolve it by centralizing the call to setCache() into one place, so that we don't in the future again run into this kind of divergence of when it's called relative to other steps.
Comment #6
effulgentsia commentedI'm conflicted between setting this to Needs work (for tests) or Needs review (for more discussion on what are legitimate use cases for #process callbacks to persist $form_state changes). Especially considering that #process callbacks get run again after the form is retrieved from cache.
However, the test itself could be a good way to elucidate the use case, so setting to Needs work for that.
Comment #7
tlo405 commentedThis is already covered in great detail above, however I figured I would give my exact use case where I am seeing this issue just in case it helps anyone..
So I have a few block types, each one with its own media field. The media field is an entity reference field which opens up a modal with all the available images (this modal is a custom entity browser). In layout builder, when I go into an existing block (by clicking 'configure') a modal pops open with all the block fields (this modal is provided by the layout_builder_modal module). if I go down to the media field, I am able to remove the existing media item without any issue. However, clicking 'Select media' again does not open up the modal, therefore I can't select a new image. This is happening only when the media field is required. If the field is not required, I can add and remove as many times as I want. If I create a new block, I can add and remove as many times as I want. I only see the error when modifying an existing block that already has a media item selected.
Comment #8
swentel commentedBeen bitten with something alike.
Setup
- custom block type, with a paragraph (multiple), which contains an image field using the entity browser widget
- go to layout on a node, add a block
- upload one image
- click add another
- uploaded image is gone
With the patch, everything is fine.
This did work though on latest D8 and entity browser 2.5, so not exactly sure what happened between latest D8 and D9.2.9 - and where exactly the problem might be.
Comment #13
ammaletu commentedI just tested the patch from #2 and all kinds of bugs went away. It's hard to imagine how much headscratching and confusion could have been avoided with this being included in core three years ago!
My setup is Drupal 10.1.7 with PHP 8.1.
The patch solved this bug:
1. Create a custom block with a required reference field, e.g. for nodes
2. Configure its form display to use an entity browser to display the field. Display the Remove button.
3. Configure the entity browser to use modal windows.
4. Edit the layout of a basic page and add the custom block.
5. Use the entity browser to add a couple of nodes.
6. Remove one of the nodes.
7. Remove another one. Instead of removing the node, the modal window to add more nodes opens.
It also solves this bug:
1. Create a custom block with a required reference field, e.g. for nodes
2. Configure its form display to use an entity browser to display the field.
3. Configure the entity browser to use modal windows.
4. Edit the layout of a basic page and add the custom block.
5. Use the entity browser to add a couple of nodes.
6. Try to save the block, but provoke a validation error, e.g. by saving the block without a title. The Layout Builder is reloaded, but the previously selected nodes are not displayed anymore. They are back again if you add another node or if you simply save the block without seeing them.
It also apparently solves a bug we had where a cached form_state was reused when different anonymous users opened the form at the same time. Only the fastest of these users then could save the form, the other's got a "duplicate UUID" error.
So, how do we add a test for this in a generic way, independent of the entity browser module? And are we sure that the approach in the patch is the right way? Is there a way to get a core committer to have a look at this? To me, this seems like quite a fundamental issue, causing numerous seemingly unrelated and hard to debug bugs. Not some minor issue that can easily be patched by community members.
Comment #14
godotislateeffulgentsia is a Form API subsystem maintainer, and from his comments a couple years ago, he was hoping to see a test to determine next steps.
I was in the process of creating an automated test for this back then, but the test I wrote was not working as expected, and then I moved to other project. I happened to re-visit this a couple weeks ago, but the challenge is that there are no forms in core that I can find that sets form storage after the cache happens, and it's non-trivial trying to devise a test form that does.
This issue only has 10 followers, so I'm guessing there isn't a lot of awareness of how it affects entity browser or similar other contrib, but perhaps if there were more eyes on this issue, things could move forward.
Comment #15
wim leersComment #17
godotislateAdded a test and created an MR. Test doesn't really demonstrate a use case, but it does demonstrate that form state changes in #process callbacks aren't cached during rebuild.
Comment #18
godotislateComment #19
godotislate@Ammaletu Is the bug reproducible without contrib or custom code? If so, can you provide steps to reproduce?
Comment #20
godotislateComment #21
wim leersVery interesting change! 🧐 👏
This indeed needs sign-off from a Form API maintainer, because it has significant potential repercussions. Pinged @tim.plunkett.
Comment #22
ammaletu commentedI'm afraid not. I had another look, and we got the "duplicate UUID" problem because we are caching the form state (to solve yet another bug). Invoking the cache kill switch on the pages with the form solved this for guest users, except for one form which is displayed via AJAX. For this last form with the "duplicate UUID" problem, the patch from this issue helped. I'am afraid I don't have the time and expertise to boil this down into a generic use case.
Comment #23
godotislateOK, thanks for checking. Mostly I was asking because there wasn't context on what type of form/use case was surfacing the "duplicate UUID" issue, but I'm assuming from your last comment that anonymous users have access to an entity form that has an entity browser widget.
Comment #24
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #25
godotislateRebased and addressed new code sniff issue.
Comment #26
smustgrave commentedGoing to bump this to framework manager.
Comment #27
smustgrave commentedGoing to take a leap on this one.
Shows the test coverage for the change.
Reviewing the code I don't see anything glaring wrong.
Issue summary appears complete.
Fingers crossed.
Comment #28
quietone commented@godotislate, thanks for the issue summary updated and the test.
I read the IS, comments and MR (but I have a headache - so really a skim). Of note is the analysis in #5. Leaving at RTBC
Comment #29
catchI'm adding back needs subsytem maintainer review. Just because a subsystem maintainer hasn't reviewed an issue yet doesn't mean they never will. The escalation to framework manager from subsystem maintainer is as a fallback. Framework managers are hugely overstretched so it does not make things happen faster artificially taking subsystem maintainers out of the loop.
fwiw this looks sensible to me and the comments/logic are as straightforward as I could imagine them getting considering it's a complex area.
Comment #30
larowlanTook a look at the code and agree with catch, seems reasonable - I could only find some nits, I've left them, but I don't think addressing them will advance the issue.
What we need here is some input from the Form API maintainers that there's no downsides to this change.
@effulgentsia is one of those and said this earlier
, but I don't think that helps alleviate the issue. There are some genuine places where the only tool you have in your toolbox to alter a form is a process callback - examples of this would be e.g. as a plugin where you're building a plugin settings form but need to alter the larger form.
I'll ping Alex and Tim. Leaving at Needs review to reflect the actual status.
Comment #31
godotislateRebased and applied suggestions from #30. Leaving in Needs Review for subsystem maintainer review.
Comment #32
bkosborneI added #3495881: Firefox retains form_build_id on form reloads, causing old form cache entry to be used and creating weird behavior for the Media Library widget which I thought might be related, but the patch here does not resolve it.
Comment #33
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #34
godotislateRebased for merge conflict.
Comment #35
larowlanThis looks good to me. In #5 @effulgentsia asked if we could address the todo here, so that we only set form cache in one spot and don't have divergent paths.
Can we explore to see if that is possible here?
Comment #36
godotislateTook a shot at consolidating the cacheing code, but it broke other tests, so I reverted. Might try again tomorrow.
Comment #37
godotislateOK, after a couple failed attempts, I'm not sure that consolidating the cache set code is possible without a larger refactor of form processing.
The challenge is that between
processForm()andrebuildForm(), in each we want to cache the form array as it is before the call todoBuildForm()with the$form_stateobject after the call todoBuildForm.The flow looks like this:
If the caching step is removed from rebuildForm is removed, and caching is done at the end of processForm regardless of whether the form is rebuilding, this works in most cases, except in batch processing.
Because for batch processing, there's this in
buildForm():So
rebuildForm()is called directly without a call toprocessForm(), and the form/form state don't get cached.If all the caching is done within
doBuildForm(), with the form array being saved at the beginning of the method, and the caching being done right before the return statement, the form state that gets cached does not include the changes made from input processing after the doBuildForm call in processForm, if the form is not rebuilding.I think the MR is probably left as is, though I can remove this:
@todo Find a way to avoid caching code duplication.Comment #38
godotislateRemoved the @todo about code duplication since it was investigated in 37.
Also, since @larowlan has looked at the MR, and he's a Form API maintainer now, I think this is ready.
Comment #39
godotislateRefactored from a functional test to a kernel test.
Comment #41
smustgrave commentedBelieve feedback from @larowlan has been addressed, test-only still fails the same #27 so believe we are good. Going to take a swing.
Comment #42
godotislateAdded to the new kernel test and rebased. Nothing else has changed, so I think it's fair to leave in RTBC.
Comment #45
catchNow that @larowlan has reviewed (covers both framework and subsystem maintainer), and there's been more than enough time for @effulgentsia and @timplunkett to object I'm happy to go ahead here. Given this is very, very low level, going to commit to main/11.x but not backport to 11.3.x though.
Committed/pushed to main and 11.x, thanks!