Form state storage changes in #process callbacks are not cached during form rebuild.

I 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.

effulgentsia 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.

Added 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.

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.

@Ammaletu Is the bug reproducible without contrib or custom code? If so, can you provide steps to reproduce?

Very interesting change! 🧐 👏

This indeed needs sign-off from a Form API maintainer, because it has significant potential repercussions. Pinged @tim.plunkett.

@Ammaletu Is the bug reproducible without contrib or custom code? If so, can you provide steps to reproduce?

I'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.

I'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).

OK, 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 using entity browser.

The 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.

Rebased and addressed new code sniff issue.

Going to bump this to framework manager.

Going to take a leap on this one.

1) Drupal\Tests\system\Functional\Form\FormStatePersistTest::testFormStatePersistence
Element matching xpath "//div[@data-drupal-messages]//div[contains(., "Rebuild state cached.")]" not found.
/builds/issue/drupal-3177977/core/tests/Drupal/Tests/WebAssert.php:856
/builds/issue/drupal-3177977/core/modules/system/tests/src/Functional/Form/FormStatePersistTest.php:53
/builds/issue/drupal-3177977/vendor/phpunit/phpunit/src/Framework/TestResult.php:729
FAILURES!
Tests: 1, Assertions: 5, Failures: 1.

Shows the test coverage for the change.

Reviewing the code I don't see anything glaring wrong.

Issue summary appears complete.

Fingers crossed.

@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 🐛 Form state storage changes in #process callbacks are not cached during form rebuild. Needs review . Leaving at RTBC

I'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.

Took 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

Especially considering that #process callbacks get run again after the form is retrieved from cache

, 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.

Rebased and applied suggestions from #30 🐛 Form state storage changes in #process callbacks are not cached during form rebuild. Needs review . Leaving in Needs Review for subsystem maintainer review.

I added 🐛 Form cache causes issues with media library widget Active which I thought might be related, but the patch here does not resolve it.