Problem/Motivation

Instance ID in State Manager (Display Builder instances, not the sources instances, we will clarify this confusing vocabulary in an other issues) are supposed to be opaques except for the class implementing WithDisplayBuilderInterface().

However we are checking them:

$ grep -r "str_starts_with.*__" modules/
modules/display_builder_devel/src/Form/EditForm.php:    if (\str_starts_with($builder_id, 'page_layout__') || \str_starts_with($builder_id, 'view__') || \str_starts_with($builder_id, 'entity_view__')) {
modules/display_builder_page_layout/src/Plugin/DisplayVariant/DisplayBuilderPageVariant.php:      if (\str_starts_with($renderable_id, 'page__') || \str_starts_with($renderable_id, 'region__')) {
modules/display_builder_entity_view/src/EventSubscriber/DisplayBuilderSubscriber.php:    if (\str_starts_with($builder_id, 'entity_view_override__')) {
modules/display_builder_entity_view/src/EventSubscriber/DisplayBuilderSubscriber.php:    elseif (\str_starts_with($builder_id, 'entity_view__')) {
modules/display_builder_views/src/Controller/ViewsManagementController.php:      if (!\str_starts_with($builder_id, 'view__')) {

And manipulating them:

$ grep -r "explode..__" modules/
modules/display_builder_devel/src/Form/ImportForm.php:      $display_id = \explode('__', $builder_id)[2];
modules/display_builder_page_layout/src/Entity/PageLayout.php:    $page_layout = \explode('__', $instance_id)[1];
modules/display_builder_entity_view/src/Field/DisplayBuilderItemList.php:    [, $entity_type_id, $entity_id, $field_name] = \explode('__', $instance_id);
modules/display_builder_entity_view/src/EventSubscriber/DisplayBuilderSubscriber.php:      [, $entity_type_id, $entity_id, $field_name] = \explode('__', $builder_id);
modules/display_builder_entity_view/src/Entity/EntityViewDisplayTrait.php:    [, $entity, $bundle, $view_mode] = \explode('__', $instance_id);
modules/display_builder_views/src/Form/ConfirmViewsBuilderDeleteForm.php:    $view_id = \explode('__', $this->builderId)[1];
modules/display_builder_views/src/Form/ConfirmViewsBuilderDeleteForm.php:    $display_id = \explode('__', $this->builderId)[2];
modules/display_builder_views/src/Plugin/views/display_extender/DisplayExtender.php:    $view = \explode('__', $instance_id)[1];
modules/display_builder_views/src/Plugin/views/display_extender/DisplayExtender.php:    $display = \explode('__', $instance_id)[2];
modules/display_builder_views/src/EventSubscriber/DisplayBuilderSubscriber.php:    $display_id = \explode('__', $builder_id)[2];
modules/display_builder_views/src/Controller/ViewsManagementController.php:    $view_id = \explode('__', $builder_id)[1];
modules/display_builder_views/src/Controller/ViewsManagementController.php:    $display_id = \explode('__', $builder_id)[2];

This is messy and risky.

Proposed resolution

Add a WithDisplayBuilderInterface::checkInstanceId() to extract parameters from Instance ID.

It is called like that and it is nullable, because the primary use of this method is to check the string is handled by the interface implementation.

It will also be a way to simplify the implementations of WithDisplayBuilderInterface::getUrlFromInstanceId()

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:

Comments

pdureau created an issue. See original summary.

pdureau’s picture

Status: Active » Needs work

Work started: https://git.drupalcode.org/project/display_builder/-/merge_requests/78

To be tested more locally and we need to wait the last merges of alpha4 anyway.

pdureau’s picture

pdureau’s picture

Status: Needs work » Needs review

Finished. The logic is now only located in classes impelmtning WithDisplayBuilderInterface:

$ grep -r "str_starts_with.*__" modules/
modules/display_builder_page_layout/src/Entity/PageLayout.php:    if (!\str_starts_with($instance_id, 'page_layout__')) {
modules/display_builder_entity_view/src/Field/DisplayBuilderItemList.php:    if (!\str_starts_with($instance_id, 'entity_view_override__')) {
modules/display_builder_entity_view/src/Entity/EntityViewDisplayTrait.php:    if (!\str_starts_with($instance_id, 'entity_view__')) {
modules/display_builder_views/src/Plugin/views/display_extender/DisplayExtender.php:    if (!\str_starts_with($instance_id, 'view__')) {
 $ grep -r "explode..__" modules/
modules/display_builder_page_layout/src/Entity/PageLayout.php:    [, $page_layout] = \explode('__', $instance_id);
modules/display_builder_entity_view/src/Field/DisplayBuilderItemList.php:    [, $entity_type_id, $entity_id, $field_name] = \explode('__', $instance_id);
modules/display_builder_entity_view/src/Field/DisplayBuilderItemList.php:    [, $entity_type_id, $entity_id, $field_name] = \explode('__', $instance_id);
modules/display_builder_entity_view/src/Entity/EntityViewDisplayTrait.php:    [, $entity, $bundle, $view_mode] = \explode('__', $instance_id);
modules/display_builder_entity_view/src/Entity/EntityViewDisplayTrait.php:    [, $entity, $bundle, $view_mode] = \explode('__', $instance_id);
modules/display_builder_views/src/Plugin/views/display_extender/DisplayExtender.php:    [, $view, $display] = \explode('__', $instance_id);

But before sending that to review to others, let's test a bit more locally

pdureau’s picture

Status: Needs review » Needs work

Tests:

  • Entity view: Save through DisplayBuilderSubscriber >> OK
  • Entity view overrides: Save through DisplayBuilderSubscriber >> OK
  • Page Layout: Save through DisplayBuilderSubscriber >> OK
  • Views: Save through DisplayBuilderSubscriber >> OK
  • Views: Load through ViewsManagementController >> OK
  • Views: Delete through ConfirmViewsBuilderDeleteForm >> OK
  • Devel with ImportForm >> OK
  • Devel with EditForm >>OK
pdureau’s picture

Assigned: pdureau » mogtofu33
Status: Needs work » Needs review

mogtofu33 made their first commit to this issue’s fork.

mogtofu33’s picture

Assigned: mogtofu33 » Unassigned
Status: Needs review » Fixed

  • mogtofu33 committed 5ba4b6d5 on 1.0.x authored by pdureau
    #3541423 fix: add WithDisplayBuilderInterface::checkInstanceId()
    
pdureau’s picture

Status: Fixed » Closed (fixed)