Problem/Motivation

Now #3549266: Move DisplayBuildableInterface to a new plugin type has been merged. We can now simplify DisplayBuildableInterface and related mechanisms as much as possible.

Proposed resolution

1. Rename ::saveSources() by ::publish()

So we are consistent all along the codebase.

2. Remove ::initInstanceIfMissing()

Works in the instance collection page has introduced a new principle: the automatic creation of missing instances entity.
See InstanceListBuilder::filterEntities(). We bet on the fact those entities are very light to be created on the fly.

That means we can remove ::initInstanceIfMissing() calls which are fragile and spread out a bit everywhere.

3. Remove ::getInstanceId()

During the early days of the development, instead of considering Instance entity ID as an opaque string, we have put meaning into it, and we rely on this meaning in DisplayBuildableInterface::checkInstanceId(), ::getDisplayUrlFromInstanceId(), ::getUrlFromInstanceId().

It is not necessary anymore since the introduction of this field in Instance entity:

    $fields['buildable'] = BaseFieldDefinition::create('plugin')
      ->setSetting('plugin_manager_id', 'plugin.manager.display_buildable')
      ->setRequired(TRUE)
      ->setReadOnly(TRUE);

We can load the buildable directly now. Let's consider this Instance entity ID string opaque, as it must be.

We will only remove ::getInstanceId() here because:

This is also the opportunity to remove an infinite loop between ViewDisplay::getProfile() calling Instance::getProfile() which is calling ViewDisplay::getProfile()....

4. Replace DisplayBuildableInterface::checkAccess() by Drupal\Core\Access\AccessibleInterface::access()

Leverage existing interface instead of creating/existing our own.

Issue fork display_builder-3573905

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

pdureau created an issue. See original summary.

mogtofu33’s picture

mogtofu33 commented
  • remove the logic around instance ID prefixes...

Has already been done.

mogtofu33’s picture

mogtofu33 commented
Issue summary: View changes
pdureau’s picture

pdureau commented

Hi Jean,

remove the logic around instance ID prefixes...

Has already been done.

I was thinking about a bigger change of logic.

At the beginning of the project, things were more losely defined than today:

  • Instance ID in State Manager was not an Entity ID yet
  • DisplayBuildableInterface implementation were not a plugin type yet

So, we were relying a lot on guessing the DisplayBuildableInterface implementation of an Instance by analyzing the instance ID string.

I have the feeling we now can start considering the instance ID string as opaque, by:

  • attaching the instance object in protected DisplayBuildableInterface::instance property (it is already the case)
  • attaching the DisplayBuildableInterface implementation in protected InstanceInterface::buildable (must be added, with EntityWithPluginCollectionInterface)

Anyway, I will play safe in my proposal. So if the benefit of such change is not obvious, i will not include in the MR.

pdureau’s picture

pdureau commented

It is also the opportunity to remove EntityViewOverride::initInstanceIfMissing() which doesnt seem to do much than the method it overrides.

pdureau’s picture

pdureau commented
Issue summary: View changes

pdureau opened merge request !235

pdureau’s picture

pdureau commented
Issue summary: View changes
pdureau’s picture

pdureau commented
Issue summary: View changes
pdureau’s picture

pdureau commented
Issue summary: View changes
pdureau’s picture

pdureau commented

DisplayBuildableInterface::getInitialContext() and DisplayBuildableInterface::getInitialSources() will already be removed here: #3576918: Specific first log messages according to the data source

pdureau’s picture

pdureau commented
Issue summary: View changes
pdureau’s picture

pdureau commented
Status: Active » Needs work

Work has started

pdureau’s picture

pdureau commented
Issue summary: View changes
pdureau’s picture

pdureau commented
Issue tags: -display_builder-1.0.0-beta4 +display_builder-1.0.0-beta5
pdureau’s picture

pdureau commented
Issue summary: View changes
Issue tags: -display_builder-1.0.0-beta5 +display_builder-1.0.0-beta

Some work will be done in #3578469: Make instance entities aware of display buildables, lets postpone this one.

mogtofu33’s picture

mogtofu33 commented
Status: Needs work » Postponed
pdureau’s picture

pdureau commented
Issue summary: View changes

The MR has been rebased on #3562989: Implements RevisionLogInterface for Instance entity because it is a better starting point for simplifications.

pdureau’s picture

pdureau commented
Issue summary: View changes
pdureau’s picture

pdureau commented
Status: Postponed » Needs work

#3562989: Implements RevisionLogInterface for Instance entity has been merged. We can resume.

pdureau’s picture

pdureau commented

The static methods expecting instance ID : ::getUrlFromInstanceId(), ::checkInstanceId(), ::getDisplayUrlFromInstanceId() and ::getUrlFromInstanceId()

Also in the scope of #3616313: Check static methods on DisplayBuildableInterface, so no need to work on this here.

pdureau’s picture

pdureau commented

Let's implements AccessibleInterface::access() and call it a day.

pdureau’s picture

pdureau commented
Issue summary: View changes
pdureau’s picture

pdureau commented
Issue summary: View changes
pdureau’s picture

pdureau commented
Issue summary: View changes
mogtofu33’s picture

mogtofu33 commented
Assigned: pdureau » mogtofu33

I put myself here as it seems a bit stopped to see with #3618945: Tidy Instance methods let me know if it's an issue.

pdureau’s picture

pdureau commented

Sure, go fot it.

Here is the current status:

d438c54c Rename ::saveSources() by ::publish()

✅ Done

9baac728 Replace some calls to ::getInstanceId() by ::getInstance()

✅ Done, but i initially wanted to do more.

5f0e16ce Remove ::iniInstanceIfMissing()

✅ Done

55d45cca Implements AccessibleInterface::access()

❌ I have reverted it because it was not working as expected.

pdureau’s picture

pdureau commented

For information, we reached PHPMD class complexity limit for ViewDisplay & DisplayBuildablePluginBase in an other ticket, so any simplification of buildables would be welcomed.

  • mogtofu33 committed 6a6996ae on 1.0.x 
    task: #3573905 Replace some calls to ::getInstanceId() by ::getInstance...
mogtofu33’s picture

mogtofu33 commented
Assigned: mogtofu33 » Unassigned

Your first two commits are on 1.0.x now, merged with #3618945: Tidy Instance methods
The third, folding creation into `getInstance()`, I did not take: it is called from read paths (`calculateDependencies()`, the pinned preview, the Instances panel) and #3617570: Instances list enhancements made those read-only on purpose. A getOrCreateInstance()` used only where a write is wanted would do the same job. Dropping `getInstanceId()` is #3616313: Check static methods on DisplayBuildableInterface.

What's left here is `AccessibleInterface::access()`.

What was not working as expected?

mogtofu33 changed the visibility of the branch 3573905-simplify-displaybuildableinterface to active.

pdureau’s picture

pdureau commented

What's left here is `AccessibleInterface::access()`.

What was not working as expected?

It was a bit mysterious. I will rebase this MR so we can focus on this task and check.

pdureau reopened merge request !235

pdureau opened merge request !384

pdureau’s picture

pdureau commented

I opened a second MR with a Claude try: https://git.drupalcode.org/project/display_builder/-/merge_requests/384/...

With similar results than the first manual try:

  • It has fails I am struggling to understand. When I read and follow the data flow, everything looks logic to me.
  • Only some phpunit tests fails, playwright are OK

Could it means we have some wrong phpunit tests here?

pdureau’s picture

pdureau commented
Title: Simplify DisplayBuildableInterface » Replace DisplayBuildableInterface::checkAccess() by AccessibleInterface::access()
Assigned: Unassigned » pdureau
Status: Needs work » Needs review
Issue tags: +AI-accelerated

Second Claude try, this time with Opus 5.5, the pipeline are green.

Needs review.

pdureau changed the visibility of the branch 3573905-simplify-displaybuildableinterface to hidden.

pdureau’s picture

pdureau commented

Review is starting.

Review with 3 different roles

Limited profile is a profile created for this tests in order to check the users can't access the default profile.

Role 1: with all permissions (but not the administrator)

Permissions:

  • Use the Limited Display Builder profile (Display Builder module): YES
  • Use the Default Display Builder profile (Display Builder module): NO
  • {entity_type}: Administer display (Field UI module): YES
  • Administer views (Views module): YES
  • Administer page layouts (Display Builder for page layout module): YES
  • Article: Edit any content (Node module, to test entity view display overrides): YES

Page Layout:

  • Can create a page layout. Expected: YES. Results: ???
  • Can load Display Builder for page layout with Limited profile. Expected: YES. Results: ???
  • Can load Display Builder for page layout with Default profile. Expected: NO. Results: ???<
  • Can alter the display builder instance with Limited profile. Expected: YES. Results: ???
  • Can alter the display builder instance with Default profile. Expected: No. Results: ??

Entity view display:

  • Can switch an entity view display to Display Builder. Expected: YES. Results: ???
  • Can load Display Builder for entity view display with Limited profile. Expected: YES. Results: ???
  • Can load Display Builder for entity view display with Default profile. Expected: NO. Results: ???<
  • Can alter the display builder instance with Limited profile. Expected: YES. Results: ???
  • Can alter the display builder instance with Default profile. Expected: No. Results: ???

Entity view override:

  • Can pick an existing field. Expected: YES. Results: ???
  • Can create a new field. Expected: NO. Results: ???
  • Can switch a views display to Display Builder. Expected: YES. Results:???
  • Can load Display Builder for override with Limited profile. Expected: YES. Results: ???
  • Can load Display Builder for override with Default profile. Expected: NO. Results: ??? tab not visible + HTTP 403
  • Can alter the display builder instance with Limited profile. Expected: YES. Results: ???
  • Can alter the display builder instance with Default profile. Expected: No. Results: ???

Views:

  • Can switch a views display to Display Builder. Expected: YES. Results: ???
  • Can load Display Builder for views display with Limited profile. Expected: YES. Results: ???
  • Can load Display Builder for views display with Default profile. Expected: NO. Results: ???
  • Can alter the display builder instance with Limited profile. Expected: YES. Results: ???
  • Can alter the display builder instance with Default profile. Expected: NO. Results: ???

Role 2: has the "classic" permissions of Drupal display building, but not the ones from Display Builder

Permissions:

  • Use the Limited Display Builder profile (Display Builder module): NO
  • Use the Default Display Builder profile (Display Builder module): NO
  • {entity_type}: Administer display (Field UI module): YES
  • Administer views (Views module): YES
  • Administer page layouts (Display Builder for page layout module): YES
  • Article: Edit any content (Node module, to test entity view display overrides): YES

Page Layout:

  • Can create a page layout. Expected: NO. Results: ???
  • Can load Display Builder for page layout. Expected: NO. Results: ???
  • Can alter the display builder instance. Expected: NO. Results: ???

Entity view display:

  • Can switch an entity view display to Display Builder. Expected: NO. Results: ???
  • Can load Display Builder for entity view display. Expected: NO. Results: ???
  • Can alter the display builder instance. Expected: NO. Results: ???

Entity view override:

  • Can pick an existing field. Expected: YES. Results: ???
  • Can create a new field. Expected: NO. Results: ???
  • Can switch a views display to Display Builder. Expected: YES. Results: ???
  • Can load Display Builder for override. Expected: NO. Results: : ???
  • Can alter the display builder instance. Expected: YES. Results: ???

Views:

  • Can switch a views display to Display Builder. Expected: NO. Results: ???
  • Can load Display Builder for views display. Expected: NO. Results: ???
  • Can alter the display builder instance. Expected: NO. Results: ???

Role 3: has the Display Builder permissions, but not the ones from classic Drupal display building

Permissions:

  • Use the Limited Display Builder profile (Display Builder module): YES
  • Use the Default Display Builder profile (Display Builder module): NO
  • {entity_type}: Administer display (Field UI module): NO
  • Administer views (Views module): NO
  • Administer page layouts (Display Builder for page layout module): NO
  • Article: Edit any content (Node module, to test entity view display overrides): NO

Page Layout:

  • Can create a page layout. Expected: NO. Results: ???
  • Can load Display Builder for page layout. Expected: NO. Results: ???
  • Can alter the display builder instance. Expected: NO. Results: ???

Entity view display:

  • Can switch an entity view display to Display Builder. Expected: NO. Results: ???
  • Can load Display Builder for entity view display. Expected: NO. Results: ???
  • Can alter the display builder instance. Expected: NO. Results: ???

Entity view override:

  • Can pick an existing field. Expected: NO. Results: ???
  • Can create a new field. Expected: NO. Results: ???
  • Can switch a views display to Display Builder. Expected: YES. Results: ???
  • Can load Display Builder for override. Expected: NO. Results: ???
  • Can alter the display builder instance. Expected: YES. Results: ???

Views:

  • Can switch a views display to Display Builder. Expected: NO. Results: ???
  • Can load Display Builder for views display. Expected: NO. Results: ???
  • Can alter the display builder instance. Expected: NO. Results: ???