Move DisplayBuildableInterface to a new plugin type

Let's play a bit with this subject.

Works have started: https://git.drupalcode.org/project/display_builder/-/merge_requests/148

With:

1. Add a new "display_buildable" plugin type.
2. Add a first plugin: modules/display_builder_page_layout/src/Plugin/DisplayBuildable/PageLayout.php
3. Move the DisplayBuildableInterface implementation from modules/display_builder_page_layout/src/Entity/PageLayout.php to this plugin

Tests are still green

Next steps:

1. do the same for Entity View, Entity View Overrides, and View extender
2. check dependency injection and don't create too much plugin instances
3. simplify DisplayBuildableInterface as much as possible
4. move as much logic in DisplayBuildablePluginBase
5. Replace display_builder_provider_info by the plugin type manager
6. simplify the controllers, event subscibers...
7. Move configFormBuilder to this plugin type?
8. Merge similar ::getInstance() methods
9. Update documentation and graphical schemas

Work in progress: https://git.drupalcode.org/project/display_builder/-/merge_requests/148

Phase 1: Add new API

Done:

1. Add a new "display_buildable" plugin type.
2. Move DisplayBuildable logic for Entity View, Entity View Overrides, and View extender, to plugins
3. Replace display_builder_provider_info by the plugin type manager

Next steps:

1. Rename plugin manager (and plugin types?) for more consistency (but we need to discuss about this first)
2. check dependency injection and don't create too much plugin instances
3. Update documentation and graphical schemas

Phase 2: Enjoying the new API

Now we have the new plugin type, we can do some simplification and cleaning:

1. simplify DisplayBuildableInterface as much as possible
2. move as much logic in DisplayBuildablePluginBase
3. simplify the controllers, event subscribers...
4. Move configFormBuilder to this plugin type?
5. Merge similar ::getInstance() methods
6. Rename: getBuilderUrl() -> getUrl(), getDisplayUrlFromInstanceId() -> getDisplayUrl(), getInitialContext() > getContext()

This may be done in a follow-up issue.

Too big and too late for beta1.

#3545474: Security: Set Instance access handler is adding a new method to DisplayBuildableInterface.

Before wrapping up (fixing phpunit, tidying up, documentation...), I would be glad to have your input:

• about the general idea: Is it relevant? Is it the right time to do such a task? Is it bringing benefits without making too much mess?
• about the implementation itself: Am I going the right way?
• about the opportunity of simplification and streamlining it may bring later
• about naming and discovery of the plugins (the current implementation is temporary)

(this is not a review before merge, just chit-chatting)

Sure

There is still conflict on rebase with current 1.0.x.

There will always have conflicts because the MR is altering a lot of files and everytime we are committing to 1.0.x a conflict may occurs.
I am rebasing twice a week to keep track of the 1.0.x changes.

There are 3 phpunit errors:

• 1 in EntityViewDisplayTest::testSaveSources
• 2 in ConfigFormBuilderTest::testConfigFormBuilderBuild

about the general idea: Is it relevant? Is it the right time to do such a task? Is it bringing benefits without making too much mess?

Because of #3562991: Instances list from config and not storage and the challenges of the upcoming #3555110: Symmetric translation, I am now convinced this change is relevant.

Before wrapping up (fixing phpunit, tidying up, documentation...) and planning the follow-up (simplify DisplayBuildableInterface, move some logic to DisplayBuildablePluginBase...), I would be glad to have some input:

• about the implementation itself: Am I going the right way?
• about the opportunity of simplification and streamlining it may bring later
• about naming and discovery of the plugins (the current implementation is temporary)

(this is not a review before merge, just chit-chatting)

Rebased.

Oops, I may have forgot to remove display_builder.api.php file.

Playwright seems OK.

There are still 2 little fails in a single phpunit test Drupal\Tests\display_builder\Kernel\ConfigFormBuilderTest::testConfigFormBuilderBuild:

• with data set "user with profile same as buildable"
• with data set "user with profile different from buildable profile"

Unfortunately, /admin/structure/display-builder/instances is now broken. It may be related to a change made recently: https://git.drupalcode.org/project/display_builder/-/commit/f413a9a392e8...

So it may be the opportunity to replace DisplayBuilderUiHelpers::getInstancesFromProviders() (and related private methods, so the full DisplayBuilderUiHelpers file may be removable) by a cleaner DisplayBuildableInterface::getInstances() static method.

Let's do rebase and rename the plugin, and try to fix the last phpunit fails.

Christian will review and once in RTBC, Jean will merge when he will be back.

Rebased. So, what have been done?

1. DisplayBuildableInterface as a new plugin type

Added:

• src/DisplayBuildablePluginBase.php
• src/DisplayBuildablePluginManager.php (replacing the custom hook)
• src/Attribute/DisplayBuildable.php
• 5 plugin implementations of the interface:
  • From modules/display_builder_entity_view/src/Entity/EntityViewDisplayTrait.php to EntityView plugin
  • From modules/display_builder_entity_view/src/Field/DisplayBuilderItemList.php (removed) to EntityViewOverride plugin
  • From modules/display_builder_page_layout/src/Entity/PageLayout.php to PageLayout plugin
  • From modules/display_builder_views/src/Plugin/views/display_extender/DisplayExtender.php to ViewDisplay plugin
  • From tests/src/Kernel/DisplayBuildableMock.php (removed) to DisplayBuildableMock plugin

2. Add DisplayBuildableInterface::collectInstances()

To fully replace DisplayBuilderUiHelpers:

  /**
   * Collect instances related to this buildable.
   *
   * Null values are returned so the caller can decide to create the missing
   * Instance entities.
   *
   * @return array
   *   A associative array of Instance entities or null values.
   */
  public static function collectInstances(): array;

Implemented in all plugins:

• modules/display_builder_entity_view/src/Plugin/display_builder/Buildable/EntityView.php
• modules/display_builder_entity_view/src/Plugin/display_builder/Buildable/EntityViewOverride.php
• modules/display_builder_page_layout/src/Plugin/display_builder/Buildable/PageLayout.php
• modules/display_builder_views/src/Plugin/display_builder/Buildable/ViewDisplay.php
• tests/modules/display_builder_test/src/Plugin/display_builder/Buildable/DisplayBuildableMock.php

Updated logic:

• modules/display_builder_ui/src/Form/InstanceListFilterForm.php
• modules/display_builder_ui/src/InstanceListBuilder.php

/admin/structure/display-builder/instances is still working OK.

3. Replace ConfigFormBuilder by DisplayBuildableInterface

ConfigFormBuilder service is heavily dependent of DisplayBuildableInterface, and now we are implementing DisplayBuildableInterface as plugins the service is always loaded alongside DisplayBuildablePluginManager.

We can heavily reduce complexity and (in a follow-up issue) reduce DisplayBuildableInterface surface, by moving ConfigFormBuilder to DisplayBuildablePluginBase.

It will also allow plugins to override some form related methods.

Remaining work

Playwright i still OK.

There are still 2 little fails in a single phpunit test Drupal\Tests\display_builder\Kernel\ConfigFormBuilderTest::testConfigFormBuilderBuild:

• with data set "user with profile same as buildable"
• with data set "user with profile different from buildable profile"

Unfortunately, I don't know how to fix them and I would be happy to have assistance here.

Also: Update documentation and graphical schemas

By the way, those plugins must be discovered as:

• "Buildable" (the current proposal in the MR, based on the existing Interface)? Namespace discovery: \Plugin\display_builder\Buildable\
• "Integration" (the word we use the most while talking together)? Namespace discovery: \Plugin\display_builder\Integration\
• "Provider" (the word used in InstanceListBuilder)? Namespace discovery: \Plugin\display_builder\Provider\
• something else?

Follow-up

The new architecture opens many opportunities to simplify the codebase. This MR is mostly DispalyBuildableInterface implementations to a new plugin type, keeping the same logic and the same methods. Very naively, without altering too much.

We will create a follow-up ticket with:

1. simplify DisplayBuildableInterface as much as possible
2. remove the logic around instance ID prefixes...
3. move as much logic in DisplayBuildablePluginBase
4. simplify the controllers, event subscribers...
5. Merge similar ::getInstance() methods
6. Rename: ::getBuilderUrl() ⮕ ::getUrl(), ::getDisplayUrlFromInstanceId() ⮕ ::getDisplayUrl(), ::getInitialContext() ⮕ ::getContext(), ::saveSources() ⮕ ::publish()...

pipeline is green :)

Documentation updated.

The MR is adding 92 lines of PHP code but architecture is simpler and clearer:

$ git co 1.0.x
$ tokei -e tests .
 Language    Files    Lines     Code Comments   Blanks
 PHP           136    23249    11753     8099     3397
$ git co 3549266-move-displaybuildableinterface-to
$ tokei -e tests .
 Language     Files   Lines     Code Comments   Blanks
 PHP           137    23419    11845     8150     3424

I am very excited about what we can do as simplifications in a follow-up ticket.

Also, this change is necessary for #3555110: Symmetric translation