Island forms share one form id and load each other's cached state

beta9 has been released. Changing the tag.

Is there an overlap with #3623623: Contextual form loses clicks and invalid values silently? Which already gives each island has its own form ID, display_builder_island_ {island_id}.

Commited the 17th of September, so the day after the creation of this ticket: https://git.drupalcode.org/project/display_builder/-/commit/cda4f8336de4...

#3622928: UX of the asynchronous component form has been commited to UI Patterns the same day and visibility.spec.ts run fine since.

I don't see much to do here, except an additional, and optional, check in ApiController, comparing the Form IDs. Am I missing something?

I can do a MR with the additional check, it may help.

Check and related test added by a LLM. I am still not 100% sure we need it.

❓ Needs more info — could not reproduce the 500 from the reported steps: visibility.spec.ts passes on current 1.0.x without MR !515, with UI Patterns 2.0.23 (which includes #3622928). The kernel test added by MR !515 does fail without the fix and passes with it. Which setup still triggers the 500 in the browser?

Before / after (kernel test)

vendor/bin/phpunit -c web/core/phpunit.xml.dist --filter testThirdPartySettingsUpdateIgnoresAnotherIslandCachedForm web/modules/contrib/display_builder/tests/src/Kernel/ApiControllerTest.php

# Without the fix:
The save is refused.
Failed asserting that '...' contains "sent by another panel".
  (stored index: third_party_settings: visibility_conditions: value: original)

# With MR !515:
OK (1 test, 15 assertions)

Notes

• In the builder UI, Visibility saves after a Config "Change source" also succeeded without the fix; no form was stored in the form cache.
• The MR rejects the foreign form with an error message instead of the per-island form id the issue summary proposes.

Needs more info — could not reproduce the 500 from the reported steps

Indeed, the issue was already fixed by #3623623: Contextual form loses clicks and invalid values silently, this work only add some safeguards.

The review of the current MR is more an expression of personal taste.

Are the precautions added to ApiController something sensible, something we may need? Are are they a bit too much?

The MR rejects the foreign form with an error message instead of the per-island form id the issue summary proposes.

Is it something we need to work on?

@pdureau The kernel test only reaches the guard by posting the Visibility form_id together with the Config form's form_build_id, a pair the builder no longer sends since #3623623. I would close this as fixed by #3623623 rather than add the guard; if anything, a test asserting that island forms keep distinct form ids would protect the actual fix.

Reviewed MR !515 and ran the kernel tests. Agree with @csakiistvan: the bug is already fixed by #3623623.

• Guard in validateIslandForm(): I'd drop it, with its test. The builder can't send that request anymore: each island form only posts its own form_id and form_build_id.
• try/catch in thirdPartySettingsUpdate(): I'd keep it. Without it, a validation error in the Styles or Visibility form isn't caught. I tested this by posting a role that doesn't exist: it throws FormValidationException instead of showing the message.
• Form id test: not needed. If getFormId() goes back to one shared id, 4 tests in ContextualFormPanelRequestTest already fail.

Suggestion: keep only the try/catch, ideally with a small test for it.

Thanks @smovs, agreed on all three. I called the guard a cheap safety net before, but since the builder can no longer send that request, dropping it with its test is fine with me. Keeping only the try/catch in thirdPartySettingsUpdate() plus a small test for the invalid-role case sounds like the right scope for this issue. Change to Postponed (maintainer needs more info) status, waiting for the decision

OK, i will have a look

Thanks both of you. I am still struggling to have a personal opinion here, but your agreement about "Keeping only the try/catch in thirdPartySettingsUpdate() plus a small test for the invalid-role case" is enough for me and I am blindly following you.

I have amended the commit and a little review will be welcomed.

This commit still has a different purpose than the ticket description (because the original task has already been shipped in #3623623: Contextual form loses clicks and invalid values silently).

@pdureau The new testThirdPartySettingsUpdateWithInvalidRole passed without the try/catch: the island forms get their values through FormState::setValues(), so core's "illegal choice" check never runs and an unknown role raises no error. I pushed a commit replacing it with testThirdPartySettingsUpdateWithValidationError, which forces a validation error through a test hook in display_builder_test. It fails without the try/catch (FormValidationException: Test validation error.) and passes with it, as does the full ApiControllerTest. Leaving it in Needs review.

I will review the added commit

It may be the display_builder-1.0.0-beta ticket I am less knowledgeable about but the changes look legit.