Problem/Motivation

Child 6 of #3624396: Plan: Harden InstanceInterface, IslandInterface and DisplayBuildableInterface as public API before RC1. An island with a form, the four Contextual panels today, gets it through a pattern of its own:

  • IslandWithFormInterface declares static getFormClass() and hasFormClass(). Core's PluginWithFormsInterface has the same names as instance methods taking an operation, so an island cannot implement both, and the statics cannot be deprecated in place.
  • ApiController calls getFormClass() on a class name, twice.
  • The form gets its data in three shapes: on render, on update and on a third party settings update. The keys mislead: builder_id holds the instance ID, instance_id the node ID, and instance the node data.
  • setBuilderId() and setNodeId() pass state the build info already carries. setNodeId() has no caller.
  • One form ID per island, the fix of #3623715: Island forms share one form id and load each other's cached state, has no test.

Proposed resolution

  • #[Island] gains forms. Islands implement PluginWithFormsInterface through PluginWithFormsTrait. configure stays the profile form. The in-builder form gets its own operation, contextual, named after where it shows, as core names the settings_tray form of blocks.
  • IslandFormBase builds the plugin form through plugin_form.factory, with one build info shape on every path: island ID, instance ID, node ID, node data, contexts. Named as #3627256: IslandInterface: name the instance $instance, and settle the island API before RC1 names them.
  • The static getFormClass() and hasFormClass() leave IslandWithFormInterface before RC1, with a change record. ApiController asks the island plugin instead of its class.
  • setBuilderId() and setNodeId() removed: the build info carries what they set.
  • One form ID per island stays, display_builder_island_{island_id}: plugin_form.factory returns a subform, so IslandFormBase stays the wrapper that owns the ID.
  • Tests: an island form gets the same build info on render and on submit, and two island forms on one page never share cached state, which closes #3623715: Island forms share one form id and load each other's cached state.

Remaining tasks

User interface changes

None.

API changes

  • IslandWithFormInterface::getFormClass() and hasFormClass() removed. Islands with a form implement PluginWithFormsInterface, and declare forms on #[Island].
  • One build info shape for island forms, with renamed keys.
  • setBuilderId() and setNodeId() removed: the build info carries what they set.

Data model changes

None.

Issue fork display_builder-3627260

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

mogtofu33 created an issue. See original summary.

mogtofu33’s picture

mogtofu33 commented
Issue summary: View changes
mogtofu33’s picture

mogtofu33 commented
Related issues: +#3627256: IslandInterface: name the instance $instance, and settle the island API before RC1, +#3623715: Island forms share one form id and load each other's cached state, +#3621008: Move DisplayBuildable form logic to a dedicated class
mogtofu33’s picture

mogtofu33 commented
Priority: Normal » Major
mogtofu33’s picture

mogtofu33 commented
Issue summary: View changes
mogtofu33’s picture

mogtofu33 commented
Issue tags: -display_builder-1.0.0-beta9 +display_builder-1.0.0-beta
pdureau’s picture

pdureau commented

It worth considering such change, especially if it moves methods from our interface to a Core one.

It will also be the opportunity of not suffixing by "Base" a final class:

 final class IslandFormBase extends FormBase

I am not sure about this specific part though:

#[Island] gains forms. Islands implement PluginWithFormsInterface through PluginWithFormsTrait..

In #3621008: Move DisplayBuildable form logic to a dedicated class, we did this change without such attribute (and without its PluginWithFormsTrait companion) which is not necessary when all plugins shared the same form by default.

Also, PluginWithFormsInterface would be better positioned as the parent of IslandWithFormInterface to avoid the weight of such an interface on all island plugins.

One form ID per island stays, display_builder_island_{island_id}: plugin_form.factory returns a subform, so IslandFormBase stays the wrapper that owns the ID.

This was already done in #3623623: Contextual form loses clicks and invalid values silently as far as I know.

pdureau opened merge request !518

pdureau’s picture

pdureau commented
Status: Active » Needs review

I did a try.

What is good:

  • No test was needed to be updated. This is only a refactor of internals, nothing breaks
  • IslandWithFormInterface's ::hasFormClass(), ::getFormClass(), ::setBuilderId() and ::setNodeId() has been removed
  • ApiController is updated
  • Only 10 more PHP LOC

What is not great:

  • There are now 2 form classes: IslandFormBase is the Drupal form. IslandContextualForm is the plugin form. It's the same split as core's block forms, with BlockForm as the Drupal form and the block plugin's configure form inside. They can't be merged.

A second commit is renaming IslandFormBase to IslandForm.

pdureau’s picture

pdureau commented
Issue tags: +Breaking Change (BC)
csakiistvan’s picture

csakiistvan
Primary language Hungarian
Location Budapest, Hungary
commented
Assigned: Unassigned » csakiistvan
csakiistvan’s picture

csakiistvan
Primary language Hungarian
Location Budapest, Hungary
commented
Assigned: csakiistvan » Unassigned

✅ Tested and works — applied MR !518 on top of current 1.0.x, rebuilt caches, opened the builder of a new page layout: the Config and Visibility tabs of a selected component render, Config edits update the preview, a Request Path visibility value saves to History, and two components keep their own form values when switching between them. The tests the proposed resolution asked for were missing, so I pushed them to the MR; keeping it in Needs review.

Tests added
  • IslandFormTest::testSameBuildInfoOnRenderAndSave: a test island with a form gets the same build info when rendered and when saved through ApiController::thirdPartySettingsUpdate().
  • IslandFormTest::testIslandFormsDoNotShareFormId: the Config form and a second island form render with their own form ID and form build ID (#3623715).
Notes
  • The save path only builds and validates the island form, it never calls submitForm(), so the test compares the two builds.
  • contexts differ between the paths: thirdPartySettingsUpdate() passes [] (existing @todo), so the test leaves them out.
pdureau’s picture

pdureau commented
Assigned: Unassigned » mogtofu33
Status: Needs review » Reviewed & tested by the community
mogtofu33’s picture

mogtofu33 commented
Assigned: mogtofu33 » Unassigned
Status: Reviewed & tested by the community » Fixed

Thanks for the work. Changelog tracked for next release.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.