Original summary

The Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay class does not correspond to an entity type.

Purpose:
1. To fix the custom field weight on the bottom of the page that is in the field manage page at 'admin/structure/types/manage/page/fields'

Sometimes, use add some custom fields at above page, some string fields, some table fields, and so on. Then we want to order the big fields on the bottom of page and the light weight fields like string fields on the top of page in the view page.

Then, I would like to add a alter method, as below; override some function in the base class.

reproduce steps:
1. Add a hook in module, as below:
2. drush en any other module.

/**
 * Implements hook_entity_type_alter().
 */
function custom_module_entity_type_alter(array &$entity_types) {
  /** @var \Drupal\Core\Entity\EntityTypeInterface[] $entity_types */
  if (\Drupal::moduleHandler()->moduleExists('layout_builder')) {
    $entity_types['entity_view_display']
      ->setClass(\Drupal\custom_module\LayoutBuilderEntityViewDisplay::class);
  }
  else {
    $entity_types['entity_view_display']
      ->setClass(\Drupal\custom_module\Entity\Entity\EntityViewDisplay;::class);
  }
}

This `LayoutBuilderEntityViewDisplay` is override init function.

And then refresh the page, the page will broken a debug message like the title.

Current summary

If an entity class is overridden by multiple modules, then static call of intermediary overriding class will raise error as not possible to find matching entity type.

Example with entity_view_display overridden by Layout Builder LayoutBuilderEntityViewDisplay then overridden by another contrib or custom module.

In layout_builder.install, the following code will raise an error:

function layout_builder_install(): void {
...
  $displays = LayoutBuilderEntityViewDisplay::loadMultiple();
...

Proposed resolution

The entity type should be aware of all intermediary class overrides.

CommentFileSizeAuthor
#51 3127026-nr-bot_swhbzj3v.txt5.93 KBneeds-review-queue-bot
#33 3127026-nr-bot.txt5.85 KBneeds-review-queue-bot
#19 interdiff-3127026-18-12.txt4.18 KBmohit_aghera
#19 3127026-originalclass-18.patch5.08 KBmohit_aghera
#19 test-only-3127026-originalclass-18.patch4.06 KBmohit_aghera
#11 3127026-originalclass-12.patch920 bytestim.plunkett
#2 3127026-layout_builder-1.patch709 bytesjohn.nie

Issue fork drupal-3127026

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

John.nie created an issue. See original summary.

john.nie’s picture

john.nie commented
StatusFileSize
new 3127026-layout_builder-1.patch709 bytes
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Active » Postponed (maintainer needs more info)
Issue tags: +Blocks-Layouts, +Needs steps to reproduce

It does when layout_builder_entity_type_alter() runs correctly. Which should be before hook_install(), if I remember correctly.

This needs clear steps to reproduce the bug. We have test coverage for this code, and it's been in core since before LB was stable, so it works for a majority of cases.

john.nie’s picture

john.nie commented
Issue summary: View changes
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

But why would you have that code? There is no such class as Drupal\node\LayoutBuilderEntityViewDisplay.
And LB already has it's own entity_type code to change this.
And even if you were trying to change things in the if block, the else block makes no sense to me.

john.nie’s picture

john.nie commented
Issue summary: View changes
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Category: Bug report » Support request

Keeping the issue summary up-to-date is good, but does not replace the value of writing an actual comment to answer the questions I'm asking.

But from parsing the revision history of the IS, it seems like you are trying to alter the Field UI, and the LB override is getting in the way?

john.nie’s picture

john.nie commented

yes,

john.nie’s picture

john.nie commented 
 ds en -y item_status
The following module(s) will be enabled: item_status, eabax_workflows, charts_c3, charts, rest, serialization, entity_log, item, color_icon, hal, migrate_source_csv, organization, currency, datetime_range, layout_builder, entity_filter, location, pinyin, views_plus, ajax_links_api, views_template, supplier, person, code, job, grade, lookup, quick_code, reference_table_formatter, uom

 // Do you want to continue?: yes.                                                                                      


In EntityTypeRepository.php line 98:
                                                                                                                
  The Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay class does not correspond to an entity type.  
                                                                                                               

/**
 * Implements hook_entity_type_alter().
 */
function custom_module_entity_type_alter(array &$entity_types) {
  /** @var \Drupal\Core\Entity\EntityTypeInterface[] $entity_types */
  if (\Drupal::moduleHandler()->moduleExists('layout_builder')) {
    $entity_types['entity_view_display']
      ->setClass(\Drupal\custom_module\LayoutBuilderEntityViewDisplay::class);
  }
  else {
    $entity_types['entity_view_display']
      ->setClass(\Drupal\custom_module\Entity\Entity\EntityViewDisplay;::class);
  }
}

The custom module redefined 

LayoutBuilderEntityViewDisplay

Class, So the default class not works.

/**
 * Implements hook_install().
 */
function layout_builder_install() {
  $display_changed = FALSE;

  $displays = LayoutBuilderEntityViewDisplay::loadMultiple();
.....
}
john.nie’s picture

john.nie commented
Status: Postponed (maintainer needs more info) » Needs review
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Issue tags: -Needs steps to reproduce +Needs tests
StatusFileSize
new 3127026-originalclass-12.patch920 bytes

Thanks for the steps to reproduce!

\Drupal\Core\Entity\EntityTypeInterface::getOriginalClass() and all the code that uses it only tracks the original class, not the decorated class. So decoration only works one level deep. This is an unfortunate bug, and wouldn't be easy to fix at the entity type level.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Version: 8.9.x-dev » 9.1.x-dev
Category: Support request » Bug report
Status: Needs review » Needs work

NW for tests

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

nikhil_drupal’s picture

nikhil_drupal commented

Is this issue resolved, i'm facing same issue when installing the module. any help would appreciated. i tried the above patches none were working my drupal version is 8.9.7

slayer722’s picture

slayer722 commented

Hello,

I have the same error message with Drupal 9.1.4 on one of my websites after LB install.
The last patch (#11) solved the error message but I cannot use LB with Display Suite installed on the same time.
It seems that this issue is related to DS because if I uninstall it before, no problem nor error message.
But I don't understand the reason and why it is not committed.

Regards

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

But I don't understand the reason and why it is not committed.

Per #11, it needs test coverage.

mohit_aghera’s picture

mohit_aghera commented
Assigned: Unassigned » mohit_aghera
mohit_aghera’s picture

mohit_aghera commented
Status: Needs work » Needs review
Issue tags: -Needs tests +DrupalFest2021
StatusFileSize
new test-only-3127026-originalclass-18.patch4.06 KB
new 3127026-originalclass-18.patch5.08 KB
new interdiff-3127026-18-12.txt4.18 KB

@tim.plunkett I've added a simple test cases to install a new module after custom module that is overriding layout is installed.
Is it sufficient enough for the use case?

The last submitted patch, 19: test-only-3127026-originalclass-18.patch, failed testing. View results

mohit_aghera’s picture

mohit_aghera commented
Assigned: mohit_aghera » Unassigned

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

edmargomes’s picture

edmargomes commented

Doesn't work for me.

Show this error: Error: Call to undefined method Drupal\Core\Entity\Entity\EntityViewDisplay::enableLayoutBuilder() in layout_builder_install() (line 27 of core/modules/layout_builder/layout_builder.install).

enableLayoutBuilder is only in LayoutBuilderEntityViewDisplay

edmargomes’s picture

edmargomes commented
Status: Needs review » Needs work

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

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

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Assigned: Unassigned » grimreaper
Related issues: +#3529127: Play nice with layout builder

Encountered the same problem in #3529127: Play nice with layout builder

Will create a MR from last patch. and use entity type manager service.

grimreaper opened merge request !12680

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Assigned: grimreaper » Unassigned
Status: Needs work » Needs review
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3127026-nr-bot.txt5.85 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Assigned: Unassigned » grimreaper
grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Assigned: grimreaper » Unassigned
Status: Needs work » Needs review

MR updated.

Code quality done, Tests improved.

Review comments addressed.

Unrelated nightwatch failure, I retriggered the job.

christian.wiedemann’s picture

christian.wiedemann commented
Assigned: Unassigned » christian.wiedemann
christian.wiedemann’s picture

christian.wiedemann commented
Assigned: christian.wiedemann » Unassigned
Status: Needs review » Reviewed & tested by the community

I tested the layout_builder installation process. Works as expected.

pdureau’s picture

pdureau commented
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Reviewed & tested by the community » Needs work

Copying from a Slack conversation today:

My concern is that this is just fixing the symptom, not the cause. This is pointing to a bug in \Drupal\Core\Entity\EntityTypeRepository::getEntityTypeFromClass()
which is called from the static \Drupal\Core\Entity\EntityBase::loadMultiple()
meaning this is a bug for ANYONE using that static method on an overridden class, during install time
so why fix it only for LB?

I pointed this out 5 years ago, at the same time as proposing the workaround. #11

If the core committers would like to include this workaround, that's fine, but a major follow-up should be opened for the underlying bug.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented

I'm not sure how this would be resolved any other way than fixing the hook_install for layout_builder to use the entityTypeManager service to get the storage. What is #11 and #39 suggesting we should do instead?

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented

Also, something got triggered recently because there's a lot more traffic on this issue. Anecdotally, layout_builder_st depends on layout builder has all of its functional tests falling over because it override the the view display class and this breaks all of its installs. I don't recall this happening even yesterday.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Issue tags: +Contributed project blocker

Tagging as a contrib blocker.

danielveza’s picture

danielveza
he/him
Location Brisbane, AU
commented
Status: Needs work » Reviewed & tested by the community

Did a code review of this earlier, and the feedback has been addressed. I think if this is a contrib blocker it's worth committing this in it's current state, however I do think we should open an issue based on #39 to explore the underlying code around this and see if there is any generic improvements we can make.

grimreaper opened merge request !12799

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

Is the long term solution will be something like in the other MR I created for POC?: https://git.drupalcode.org/project/drupal/-/merge_requests/12799/diffs?c...

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Reviewed & tested by the community » Needs review

#44 is exactly what I was looking for, thank you @grimreaper!
This speaks directly to the bug we're experiencing here, and retains the tests from the previous MR.

heddn changed the visibility of the branch 3127026-drupallayoutbuilderentitylayoutbuilderentityviewdisplay-class-does to hidden.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Needs review » Reviewed & tested by the community

I've been testing with the patch here from MR 12799 and did a code review as well of said MR. I think this does a better job than the previous approach. Contrib module testing can confirm the root issue is gone now. LGTM.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

+1 for RTBC, thanks @pdureau and @heddn for being patient with my pushback, and @grimreaper for making it happen.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

Hi,

Thanks to have pushed the MR forward! And glad to have helped!

+1 for RTBC too, I have retested on my contrib project, working fine too!

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3127026-nr-bot_swhbzj3v.txt5.93 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

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

dcam’s picture

dcam commented
Status: Needs work » Reviewed & tested by the community

I fixed the PHPStan failure that caused the bot to complain. It was a minor change from @covers to @legacy-covers, so I'm restoring the RTBC status.

kmonty’s picture

kmonty
He/Him
Location Berlin, DE + SF, CA
commented
Status: Reviewed & tested by the community » Needs review
grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Status: Needs review » Reviewed & tested by the community

Last commit is ok, back to RTBC.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Any chance to merge this into core soon, as this is RTBC and still blocking #3431600: Automated Drupal 11 compatibility fixes for layout_builder_st for example and therefore block Drupal 11 upgrades for many users. Would be great to have this fixed.

Thank you all!

mondrake’s picture

mondrake
Location 🇮🇹
commented

Nits added inline to the MR.

kmonty’s picture

kmonty
He/Him
Location Berlin, DE + SF, CA
commented
Status: Reviewed & tested by the community » Needs work
grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Assigned: Unassigned » grimreaper

Rebasing and taking review into account.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Assigned: grimreaper » Unassigned
Status: Needs work » Needs review

MR rebased and last review taken into account, back to needs review.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC, thanks for taking care of my nits

catch’s picture

catch
he/him
Primary language English
commented
Component: layout_builder.module » entity system
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs issue summary update

I was expecting to see only layout_builder changes, but this is quite a low level change to the entity API - could we get an issue summary update? Also seems like it might need a change record.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

Proposed new title:

Different modules can't override the same entity type class

Proposed summary:

If an entity class is overridden by multiple modules, then static call of intermediary overriding class will raise error as not possible to find matching entity type.

Example with entity_view_display overridden by Layout Builder LayoutBuilderEntityViewDisplay then overridden by another contrib or custom module.

In layout_builder.install, the following code will raise an error:

function layout_builder_install(): void {
...
  $displays = LayoutBuilderEntityViewDisplay::loadMultiple();
...

Proposed resolution

The entity type should be aware of all intermediary class overrides.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Title: Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay class does not correspond to an entity type. » Not possible to override an entity type class multiple times
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update

Updating issue title and summary.

Back to RTBC.

dcam’s picture

dcam commented

The MR for this issue was identified as having a new Kernel/Functional test class that did not have the #[RunTestsInSeparateProcesses] attribute. A deprecation warning is now issued in the case of these omissions. I've rebased the MR added the attribute to prevent this from being committed as-is and accidentally breaking tests on HEAD. Because this is a minor change to test metadata and the tests are passing I am leaving the issue's status as RTBC.

pdureau’s picture

pdureau commented
Issue tags: +11.3.0 release priority

That's great we are that close to 11.3 merge ;)

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs review

Left some comments on the MR
Fine to self RTBC after updating/replying

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Assigned: Unassigned » grimreaper
grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Assigned: grimreaper » Unassigned
Status: Needs review » Reviewed & tested by the community

Hello,

Last code review taken into account.

Per comment 67, back to RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue tags: +Needs followup

Added a comment to MR. We need a follow-up to replace \Drupal\Core\Entity\EntityType::getOriginalClass()

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

We should also add a change record to detail what is changed here and what is now supported.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Assigned: Unassigned » grimreaper
grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Issue tags: -Needs followup, -Needs change record

Follow up created: #3557461: Deprecate EntityTypeInterface::getOriginalClass

Draft change record created: https://www.drupal.org/node/3557464

TODO: finish updating tests.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Assigned: grimreaper » Unassigned
Status: Needs work » Needs review

Hi,

All latest code review comments had been addressed.

Thanks for the new review round!

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

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Hiding old patches

Rebased the MR as it was about 190 commits back and it's still all green

Test coverage can be found here https://git.drupalcode.org/issue/drupal-3127026/-/jobs/7617730

Looking at the CR I'm going on the assumption this can still land in 11.3 but if not that will have to be bumped to 11.4

Rest of the feedback here appears to be addressed.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 11.x-dev » 11.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 22134b6d23d to 11.x and 47aac11708d to 11.3.x. Thanks!

Backported to 11.3.x as a release priority and a non trivial bug caused by modules overlapping.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • alexpott committed 47aac117 on 11.3.x 
    fix: #3127026 Not possible to override an entity type class multiple...

  • alexpott committed 22134b6d on 11.x 
    fix: #3127026 Not possible to override an entity type class multiple...

Status: Fixed » Closed (fixed)

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

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented

Any chance of this bug fix going back to 11.2 as well? 11.2 is still supported and is the active branch for executing testing. Causing https://www.drupal.org/project/layout_builder_st/issues/3431600 to still fail.

grimreaper closed merge request !12799