Ensure pages are supported when existing limits are set to articles

That seems like another temporary work-around. I think our time would be better spent thinking about a long-term, permanent solution.

Situation

1. For months now, to get XB off the ground/start somewhere, we got by with hardcoding explicit support for the article NodeType, specifically in the way that XB experience_builder_install() + config/optional/field.field.node.article.field_xb_demo.yml set it up.
2. 📌 Create Page entity type Active and subsequent issues/MRs landed, so now there's >1 content entity type/bundle to support
3. This issue is about supporting both, and the simplest choice is to expand hard-coded bits like those in point 1.

Proposal: use the first (and only) XB field on a given content entity

We should add validation logic to ensure that there can only ever be one XB field per content entity (so: validation logic at the content entity type level and/or bundle level).

But rather than spending time on implementing this (which AFAIK Drupal core does not have infrastructure for), I'd be pragmatic, and just assume for now that this rule is not violated. So then we can just pick the first XB field for a given content entity.

So, something like:

  private static function getXbFieldName(FieldableEntityInterface $entity): ?string {
    $field_definitions = $entity->getFieldDefinitions();
    foreach ($field_definitions as $field_name => $field_definition) {
      if (is_a($field_definition->getClass(), ComponentTreeItem::class, TRUE)) {
        return $field_name;
      }
    }
    return NULL;
  }

(in a trait)

which should then allow:

 experience_builder.module              | 11 ++++-------
 src/ClientDataToEntityConverter.php    |  3 +--
 src/Controller/ApiLayoutController.php | 21 +++++++++++++--------
 3 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/experience_builder.module b/experience_builder.module
index df0c102f..f53b1cfb 100644
--- a/experience_builder.module
+++ b/experience_builder.module
@@ -57,17 +57,14 @@ function experience_builder_toolbar(): array {
 
   // @see experience_builder.routing.yml
   if ($user->hasPermission('access administration pages')) {
-    $node = \Drupal::routeMatch()->getParameter('node');
-    if (!$node instanceof Node || !$node->hasField('field_xb_demo')) {
-      $node = Node::load(1);
-    }
-    if ($node && $node->hasField('field_xb_demo')) {
+    $entity = \Drupal::routeMatch()->getParameter('entity');
+    if ($entity && self::getXbFieldName($entity) !== NULL) {
       $items['experience_builder'] += [
         '#type' => 'toolbar_item',
         'tab' => [
           '#type' => 'link',
-          '#title' => t('Experience Builder: %title', ['%title' => $node->label()]),
-          '#url' => Url::fromRoute('experience_builder.experience_builder', ['entity_type' => 'node', 'entity' => $node->id()]),
+          '#title' => t('Experience Builder: %title', ['%title' => $entity->label()]),
+          '#url' => Url::fromRoute('experience_builder.experience_builder', ['entity_type' => $entity->getEntityTypeId(), 'entity' => $entity->id()]),
           '#attributes' => [
             'title' => t('Experience Builder'),
             'class' => ['toolbar-icon', 'toolbar-icon-edit'],
diff --git a/src/ClientDataToEntityConverter.php b/src/ClientDataToEntityConverter.php
index 95b44064..e92bbe74 100644
--- a/src/ClientDataToEntityConverter.php
+++ b/src/ClientDataToEntityConverter.php
@@ -32,7 +32,6 @@ class ClientDataToEntityConverter {
   ) {}
 
   public function convert(array $client_data, FieldableEntityInterface $entity): EntityConstraintViolationListInterface {
-    assert($entity->hasField('field_xb_demo'));
     // @todo Security hardening: any key besides `layout` and `model` should trigger an error response.
     // @todo Allow more keys when allowing data other than the XB component tree to be edited through the XB UI! See the `2.1. Content editing of meta fields` requirement, due for the 0.2 milestone: https://www.drupal.org/project/experience_builder/issues/3455753
     ['layout' => $layout, 'model' => $model] = $client_data;
@@ -42,7 +41,7 @@ class ClientDataToEntityConverter {
       return new EntityConstraintViolationList($entity, iterator_to_array($violations));
     }
 
-    $item = $entity->get('field_xb_demo')->first();
+    $item = $entity->get(self::getXbFieldName($entity))->first();
     assert($item instanceof ComponentTreeItem);
     $item->setValue([
       'tree' => json_encode($tree, JSON_UNESCAPED_UNICODE | JSON_FORCE_OBJECT),
diff --git a/src/Controller/ApiLayoutController.php b/src/Controller/ApiLayoutController.php
index 37cfab6d..1e49735c 100644
--- a/src/Controller/ApiLayoutController.php
+++ b/src/Controller/ApiLayoutController.php
@@ -14,21 +14,26 @@ final class ApiLayoutController {
 
   use NotTheGoodAutoSaveTrait;
 
+  private static function getXbFieldName(FieldableEntityInterface $entity): ?string {
+    $field_definitions = $entity->getFieldDefinitions();
+    foreach ($field_definitions as $field_name => $field_definition) {
+      if (is_a($field_definition->getClass(), ComponentTreeItem::class, TRUE)) {
+        return $field_name;
+      }
+    }
+    return NULL;
+  }
+
   public function __invoke(FieldableEntityInterface $entity): JsonResponse {
-    if ($entity->getEntityTypeId() !== 'xb_page' && $entity->bundle() !== 'article') {
-      throw new \LogicException('For now, this assumes the entity is an xb_page or an article node!');
+    $field_name = self::getXbFieldName($entity);
+    if ($field_name === NULL) {
+      throw new \LogicException('This entity does not have an XB field!');
     }
 
     if ($body = $this->getAutoSaveData($entity)) {
       return new JsonResponse($body);
     }
 
-    if ($entity->getEntityTypeId() === 'xb_page') {
-      $field_name = 'components';
-    }
-    else {
-      $field_name = 'field_xb_demo';
-    }
     $item = $entity->get($field_name)->first();
     assert($item instanceof ComponentTreeItem);
     $tree = $item->get('tree');

I'll keep assigned to me. amangrover90 from my team is picking this up and I pinged him to make sure he reviews. I like the approach, good idea @wim leers!

Reviewing but see uses of `field_xb_demo` in src/ClientDataToEntityConverter.php for the error messages

A couple suggestions but I think this looks pretty good

The only place I see left where have hard-coded articles besides setting up the field_xb_demo field is in
\Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\SingleDirectoryComponent::getClientSideInfo:

$suggestions = $this->fieldForComponentSuggester->suggest($component_plugin->getPluginId(), EntityDataDefinition::create('node', 'article'));</code>

But I don't think is a quick fix because there is no $entity there to remove the hard-coded `article`. My guess is that practically won't have an effect because field.value.component_tree doesn't all dynamic under ComponentTreeMeetRequirements so won't get fields that based on entity definition anyways but it has been a while since I have looked at that so I am not positive. I will create an issue for that

only empty-canvas.cy.js is failing which is known issue . see 📌 Remove leftover wait() in empty canvas e2e test Active

actually I going to fix a couple things

LGTM except that I do not quite get why

// @todo remove this once we support more entity types/bundles.

is still there, if the logic just a few lines later already has generalized it beyond the Page content entity type and the article bundle for Node?

Is it to avoid spending ANY development time on other content entity types and bundles?

If so: I agree, but the exception message should explicitly state that.

If not: can you enlighten me? 😇

(Reviewed and written on my phone. Browsing the dozens of GitLab comments is a nightmare. I reviewed the diffs.)

Re #12

I was going to say I had tested this and didn't work with Basic Page but now I tried one more time and it actually does.

Interesting the problem I saw before was that the XB edit work but then the "Publish" button only worked, the changes actually showing on viewing the node, only worked for Articles. Not sure

But I think if want remove this restriction I think we should add a test in this issue that uses with another entity type to make sure everything works correct, not just rely on the manual testing.

Interestingly there are no e2e test for publishing because know was added in ✨ Connect the "Publish" button with the entity update controller Active . So do we just remove the article restriction now when all of our tests use article?

I tested terms and it doesn't seem to work to publish

Personally I think we should leave the restriction in for now and open a follow-up to add test coverage.

I think this issue is good to get in because it unblocks the Page work but I don't we should hastily assume everything work just because we solved the field name problem

created follow-up 🌱 Milestone 0.2.0: Experience Builder-rendered nodes Active

To me this is not a top priority because it is not needed for 🌱 Milestone 0.2.0: Experience Builder-rendered nodes Active but I think would good others wanted to take it on

Personally I think we should leave the restriction in for now and open a follow-up to add test coverage.

Agreed. The main goal was to ensure we have two testable paths, as there is some upcoming work which will be made a lot easier by targeting pages only.

The 1 e2e test that failed `contextual-panel.cy.js`, is now passing on re-run. I think I answered @wim leers question in #12 but since it is just the difference of changing a comment we can always open an another MR on this issue to fix.

Great to see this having landed! 👍

I did say in #12 that I'm fine with the restriction, just wanted to see the message refined to avoid confusion 😊

Unpostponed 📌 [PP-1] Create tests for other types of content entities and bundles using the XB field Active ! 🚀

Automatically closed - issue fixed for 2 weeks with no activity.