- First commit to issue fork.
- π«π·France Grimreaper France π«π·
Encountered the same problem in π [1.0.0-alpha2] Play nice with layout builder Active
Will create a MR from last patch. and use entity type manager service.
- Merge request !12680Issue #3127026 by mohit_aghera, john.nie, tim.plunkett:... β (Open) created by Grimreaper
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.
- π«π·France Grimreaper France π«π·
MR updated.
Code quality done, Tests improved.
Review comments addressed.
Unrelated nightwatch failure, I retriggered the job.
- π©πͺGermany Christian.wiedemann
I tested the layout_builder installation process. Works as expected.
- πΊπΈUnited States tim.plunkett Philadelphia
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 Nicaragua
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 Nicaragua
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.
- π³πΏNew Zealand danielveza Brisbane, AU
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.
- Merge request !12799Issue #3127026 by mohit_aghera, john.nie, tim.plunkett:... β (Open) created by Grimreaper
- π«π·France Grimreaper France π«π·
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...
- πΊπΈUnited States tim.plunkett Philadelphia
#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 Nicaragua
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.
- πΊπΈUnited States tim.plunkett Philadelphia
+1 for RTBC, thanks @pdureau and @heddn for being patient with my pushback, and @grimreaper for making it happen.
- π«π·France Grimreaper France π«π·
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!