Implements RevisionLogInterface for Instance entity

Work has started. Interface added but implementations not complete yet.

As the author of the issue, I am proposing to move this task after beta2.

Back to beta2 scope because may be needed for #3570382: Fatal: Compatibility with JSON API

Rebased.

Since beta2, we have a fake, temporary, implementation of ContentEntityInterface, so of RevisionableInterface, in Drupal\display_builder\Entity\ContentEntityBase

We are still hesitating:

• do we keep this file and fill it with our own custom implementations of the expected interfaces?
• do we adopt the real \Drupal\Core\Entity\ContentEntityBase as a base class and adapt the custom logic in Instance and InstanceStorage only ?

Anyway, the addition of this interface may be interesting for the current work.

Moved to beta4 so I can focus on #3549266: Move DisplayBuildableInterface to a new plugin type

Since beta3, Instance extends the real \Drupal\Core\Entity\ContentEntityBase and InstanceStorage extends \Drupal\Core\Entity\ContentEntityStorageBase.

That means most of the logic is already implemented, we just must target the specific part to override:

RevisionableInterface on Instance entity:

• isNewRevision() : bool; >> we can keep ContentEntityBase implementation
• setNewRevision($value = TRUE) : bool; >>
• getRevisionId() : int|null|string; >> we can keep ContentEntityBase implementation
• getLoadedRevisionId(): int; >> we can keep ContentEntityBase implementation
• isDefaultRevision(bool $new_value = NULL): bool;
• wasDefaultRevision(): bool; >> we can keep ContentEntityBase implementation
• isLatestRevision(): bool; >> call storage::getLatestRevisionId(), we can keep ContentEntityBase implementation
• preSaveRevision(EntityStorageInterface $storage, \stdClass $record);

RevisionableStorageInterface on InstanceStorage handler:

• createRevision(RevisionableInterface $entity, $default = TRUE): RevisionableInterface;
• loadRevision($revision_id): RevisionableInterface; >> call to loadMultipleRevisions(), we can keep ContentEntityStorageBase implementation
• loadMultipleRevisions(array $revision_ids): array;
• loadRevisionUnchanged($revision_id) : ?EntityInterface; >> call to loadRevision()
• deleteRevision($revision_id);
• getLatestRevisionId($entity_id);

I will give a try

In this MR, let's try to not change the storage data structure. InstanceStorage will convert the data. We can create a follow-up later if wished.

Part 1 is nearly done, only some phpunit faiils to fix in LayoutBuilderConfigMigrationTest::testLayoutBuilderConfigImport()

A key aspect of Content Moderation is the ability to have future (or forward) revisions. This means that the default revision is not necessarily the most recent one anymore.

https://www.lullabot.com/articles/basics-drupal-revisions-and-content-mo...

We need a similar mechanism.

Remaining work:

• undo/redo
• saved state: hasSave(), saveIsCurrent(), setSave(), restore()

This issue is now rebasing #3578224: Add ::getPast() & ::getFuture() to HistoryInterface which is rebasing #3577791: Adopt Field API for Instance entity data which is rebasing 1.0.0 ;)

This issue is now rebasing #3578469: Make instance entities aware of display buildables which is rebasing 1.0.0 ;)

Chained issues: This issue is now rebasing #3578469: Make instance entities aware of display buildables which is rebasing #3579299: Tidy context management which is rebasing 1.0.x

Remaining work:

• A few todos...
• Failing tests: InstanceHistoryTest & InstancePublishingTest

Notes

1. This was dropped from Instance::setNewPresent():

If it's the very first action, we want a NULL in the past to be able to undo to initial empty state.

Is it something we want to keep?

2. Is it possible to avoid the manual reload of the instance entity after undo/redo i have added to ApiController and InstanceHistoryTest?

     $instance->redo();
+    $instance = $instance::load($instance->id());
     self::assertSame($state2, $instance->getCurrentState());

Related UI Patterns issue: #3584856: Source field model and storage

We can do without, but it will be better to have it soon (so the storage is safer and more solid)

Nearly finished.

2 MR:

• one without the removal of profileId: https://git.drupalcode.org/project/display_builder/-/merge_requests/192
• one with the removal of profileId: https://git.drupalcode.org/project/display_builder/-/merge_requests/270 (nearly 100 lines removed, not bad)

Both have the same playwright fails.

• tests/src/Playwright/Tests/config.spec.ts
• tests/src/Playwright/Tests/preset.spec.ts
• tests/src/Playwright/Tests/style.spec.ts
• tests/src/Playwright/Tests/views.spec.ts: (The "usual" error with AJAX modal in view admin page, nothing to worry about)

OK, the pipeline is finally OK on both branches:

• 3562989-implements-revisionloginterface-for is green
• 3562989-without-profileId has the already known occasional Views AJAX fail

2 last stuff to check (which are not caught by phpunit and playwright) before sending to review:

• There is something wrong with PUBLISH event and real-time collaboration, the updated move the published indicator in LogsPanel to the wrong revision. We will do a follow-up of #3579299: Tidy context management
• The initial revision of an instance has sometimes a different hash than the published state but the same data, because UI Patterns Source field type force a property order and the presence of third party settings property.

Also, a follow-up issue will be created to propose the addition of the computed hash in the permanent storages (which will also allow some simplification of LogsPanel)

3562989-without-profileId is green and ready to review.

This MR is also embedding #3579299: Tidy context management and #3578469: Make instance entities aware of display buildables, whcih can be reviewed together or separately.

3 follow-up ticket will be created after merge:

• Something about cached entity context value in SSE controller, which sometimes mark the wrong revision as published. See Florent comment in #3579299: Tidy context management
• Addition of the computed hash in the permanent storages for (big?) performance gain (which will also allow some simplification of LogsPanel)
• Remove \Drupal\display_builder_entity_view\EventSubscriber\DisplayBuilderSubscriber : no need to listen ON_REVERT because we have already the instance entity to manipulate in ApiPublishingController::revert()

Both Florent (with SSE) and Christian (with Translations) struggle because of the srong caching of the context API.

I take the ticket back and try a little something.

A new branch has been pushed: https://git.drupalcode.org/project/display_builder/-/merge_requests/272

With the replacement of a field storing the display buildable contexts

    $fields['buildable'] = BaseFieldDefinition::create('string')->setRequired(TRUE)->setReadOnly(TRUE);
    $fields['contexts'] = BaseFieldDefinition::create('context')->setRequired(TRUE)->setCardinality(-1)->setReadOnly(TRUE);

class ContextItem extends FieldItemBase {

  public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition) {
    $definitions = [];
    $definitions['id'] = DataDefinition::create('string')->setRequired(TRUE);
    $definitions['type'] = DataDefinition::create('string')->setRequired(TRUE);
    $definitions['value'] = DataDefinition::create('any')->setRequired(TRUE);

    return $definitions;
  }

By a field storing the the display buildable config:

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

class PluginItem extends FieldItemBase {

  public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition): array {
    $definitions = [];
    // @see FactoryInterface::createInstance()
    // The ID of the plugin being instantiated.
    $definitions['plugin_id'] = DataDefinition::create('string')->setRequired(TRUE);
    // An array of configuration relevant to the plugin instance.
    $definitions['configuration'] = MapDataDefinition::create();

    return $definitions;
  }

Unfortunately, it doesn't solve Florent's issue with SSE, but I believe it makes the architecture clearer and the work easier.

Phpunit fails. I check

Pipeline is green

The last pipeline run ("WIP" commit) is a mistake and was testing something else. Please don't consider it. The pipeline is still green.

Because the issue is not merge yet, I am allowing myself to propose 4 tiny last-minute changes. Jean, what do you think about them?

1. Add the metadata needed by #3555110: Symmetric translation

  entity_keys: [
+    'langcode' => 'langcode',
+    'default_langcode' => 'default_langcode',
  ],

and:

+  translatable: TRUE,

So, we alter the storage only once. The SQL schema will not change when we will merge #3555110: Symmetric translation.

2. Remove the useless bundle col

The entity type is not "bundlable", why is it present in some tables?

Edit: its mandatory with SqlContentEntityStorage : #2163735: Remove the 'bundle' column from the field schema

3. Make table naming more consistent

Sometimes we have display_builder, sometimes display_builder_instance:

4. Rename data field by sources?

Maybe data is too generic.

-    $fields['data'] = BaseFieldDefinition::create('ui_patterns_source')
+    $fields['sources'] = BaseFieldDefinition::create('ui_patterns_source')

Also, it would be nice to add no_ui to src/Plugin/Field/FieldType/PluginItem.php

Rebased, and new commit added with:

• no_ui in src/Plugin/Field/FieldType/PluginItem.php
• data field renamed sources
• More consistent table naming
• metadata needed by #3555110: Symmetric translation added

The pipeline may be red because of phpstan:

  99     Cannot assign new offset to list<mixed>|string.                                           
         🪪  offsetAssign.dimType                                                                  
         ✏️  modules/display_builder_page_layout/src/Plugin/UiPatterns/Source/PageLayoutSource.php  

This is also happenign in main branch and not related to the work in this MR.

Discussed with Jean (@mogtofu33), we need an update process.

Proposal 1

We stay on branch 1.x and we add a hook update with 2 steps:

1. display_builder_instance entity delete all
2. content entity schema rebuild

Proposal 2

We propose the user to:

1. export config
2. uninstall display_builder module
3. import config (so module reinstallation)

For this, Jean suggest to open a 2.x branch.

Hi Jean,

Proposal 1

Done in a commit pushed in the MR: https://git.drupalcode.org/project/display_builder/-/merge_requests/272/...

Proposal 2

Switch at the first position:

$ git co 3562989-plugin-config-instead-of-contexts
$ drush cex
$ drush pmu display_builder
> Error: The storage handler of the "display_builder_instance" entity type specifies a non-existent class "Drupal\display_builder\InstanceStorage"

Switch at the second position:

$ drush cex
$ git co 3562989-plugin-config-instead-of-contexts
$ drush pmu display_builder
> Error: The storage handler of the "display_builder_instance" entity type specifies a non-existent class "Drupal\display_builder\InstanceStorage"

Switch at the third position:

$ drush cex
$ drush pmu display_builder
$ git co 3562989-plugin-config-instead-of-contexts
$ drush cim
> Error: Call to a member function setNewPresent() on null in LayoutBuilderEntityViewDisplay->postSave()

Am I doing something wrong?

#3584856: Source field model and storage which is making the sources field type more robust for our use, has been merged in UI Patterns 2.0.x branch

One issue when creating a page layout and letting the 'page source' wrapper, nothing is displayed when saved.

With UI Suite Bootstrap ? It happens sometimes already, even with the main Display Builder branch (1.0.x) according to Florent (@grimreaper).

One issue when creating a page layout and letting the 'page source' wrapper, nothing is displayed when saved.

Weird, I didn't notice that during my tests. A cache issue?

But I struggle with rebase for `EntityViewOverride::__construct` related to 283, could you have a look?

Sure, I will open a new MR (#288) with:

1. a squash of the last commit with a green pipeline
2. a fresh rebase from 1.0.x
3. my own take on the IsnatnceList issue.

Hey Jean,

I have proposed a InstanceListBuilder fix by altering the logic of DisplayBuildableInterface::collectInstances() implementations, moving away from Instance ID string parsing to rely more of the existing logic of the buildable plugin.

This is the direction we are moving to, in my opinion, less and less magic around those strings which are becoming more opaque.

I have also removed some of the EntityListBuilder overrides you suggested.

There are more ambitious changes to do, more from your suggestions and some I would like to propose, but I have preferred to keep this additional commit small, and move this work to #3573905: Simplify DisplayBuildableInterface

Issue with ui_patterns dev branch.

If you test my proposal with entity view overrides and ui_patterns-2.0.x-dev, you may encounter a fatal error, recently introduced at UI Patterns (because NULL main property breaks entity queries). You can apply this patch to test properly:

--- a/modules/ui_patterns_field/src/Plugin/Field/FieldType/SourceValueItem.php
+++ b/modules/ui_patterns_field/src/Plugin/Field/FieldType/SourceValueItem.php
@@ -33,7 +33,7 @@ class SourceValueItem extends FieldItemBase {
    */
   public static function mainPropertyName() {
-     // A source item has no main property.
-    return NULL;
+    return 'source';
   }

It will be fixed with #3548884: SourceValueItem field synchronized translations. You can merge this MR if you are OK with my proposal, because this bug is not related to the current change, it will appear with or without this ticket to be merged.

You didn't put back install update part

Oops, i may have messed up with my squash. I will fix that and investigate.

Good news, i reproduce the error.

Install:

$ git co 1.0.x
$ make install (including test modules)

✅ /admin/structure/display-builder/instances is OK (with the instances provided by the test modules)

Update:

$ git co 3562989-pierre-rebase 
$ ddev drush updb

❌ Error: Call to a member function label() on null in InstanceListBuilder.php

Not sure why it works for entity_view...

Maybe because it is the only one where I didn't forger to execute $buildable->initInstanceIfMissing() :)

So, I have 2 options:

• the safe change: just add the missing ::initInstanceIfMissing() calls
• the clean, but risky change: get rid of all ::initInstanceIfMissing() calls and move the logic to ::getInstance()

I chose the safe path :)

The pipeline is greener than green