Overview

The first thing in \Drupal\experience_builder\Storage\ComponentTreeLoader::getXbFieldName() is a LogicException that is thrown if you try to use XB on anything except an article node.

This is not going to work for Drupal CMS. Although we do not need any ability to use XB to edit individual nodes right now -- nor do we need the ability to edit content templates in the UI -- we do need the ability to render any entity type through an existing content template (which will we hand-craft). The LogicException is a brick wall preventing that.

This blocks site templates, and therefore the full adoption of XB by Drupal CMS 2.x.

Proposed resolution

Fully remove the exception.

User interface changes

None.

Issue fork experience_builder-3539108

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

phenaproxima created an issue. See original summary.

phenaproxima opened merge request !1397

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Active » Needs review
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

Alternative approach: if we don't want to remove the exception outright, another option that will work for us is to remove this from ContentTemplateAwareViewBuilder:

      try {
        $this->componentTreeLoader->getXbFieldName($entity);
      }
      catch (\LogicException) {
        // This entity isn't opted into XB, so there's nothing else to do.
        continue;
      }

This would mean we don't care if any entity is "opted into" XB or not -- only that a template exists. However, we'd certainly want to restore this logic, or equivalent logic, at a later time.

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

isholgueras’s picture

isholgueras commented

The only difference that this MR brings is the exception logged in the database and is step forward. We would have to delete this code sooner or later, so I'm OK with this.

About #4, I think it's better to leave the code as is. IMO, that would be, or kind of, the final code.

I've fixed one phpunit test and all should be green now.

isholgueras’s picture

isholgueras commented
Component: … to be triaged » Config management
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#3518272: Support all entity types with configurable displays for content templates

need the ability to render any entity type through an existing content template

As of https://git.drupalcode.org/project/experience_builder/-/commit/843e7b0b1..., ContentTemplates are only allowed for node content entities, as proposed by @effulgentsia as a realistically achievable goal for 1.0. @lauriii and I both +1'd this proposal, and made it so.

That did not implement all of #3518272: Support all entity types with configurable displays for content templates, but allowed us to make it no longer stable-blocking.

(Fun fact: you created #3518272, @phenaproxima! :D)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Component: Config management » Data model
Assigned: Unassigned » phenaproxima
Status: Needs review » Needs work

+1 to removing this exception.

But let's convert rather than delete existing test coverage to expect the fallback exception? i.e. this one:

throw new \LogicException("This entity does not have an XB field!");

I might be missing something obvious, but I don't understand why we wouldn't keep that.

Ah … I guess that is what #4 is implicitly referring to — I think removing that catch (and the other one in the same file) is the right thing to do.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Assigned: phenaproxima » Unassigned
Status: Needs work » Needs review
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Thanks — looks great now! 👍

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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