Problem/Motivation

The Layout Builder module has no explicit dependency on the Block module, but relies on it in two (known) places:

  1. '#theme' => 'block' is used in \Drupal\layout_builder\EventSubscriber\BlockComponentRenderArray::onBuildRender(), but there is no guarantee the block module will be installed.
  2. The Layout Builder UI relies on local tasks (typically provided by a placed block) for navigation, until #2936655: Layout Builder should use the toolbar modes system once it exists. is resolved

Proposed resolution

Add block.module as a dependency, with an @todo pointing to #3003610: Remove block.module dependency from Layout Builder for potential future removal

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#16 2934223-block-16.patch533 bytestim.plunkett
#12 2934223-12.patch8.79 KBMerryHamster
#11 2934223-11.patch91.71 KBMerryHamster
#4 2934223-4-1.patch8.65 KBjibran
#4 interdiff-20f4b3.txt3.23 KBjibran
#3 2934223-4.patch8.29 KBjibran
#2 2934223-3.patch935 bytesjibran
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jibran created an issue. See original summary.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran at PreviousNext commented
Status: Active » Needs review
Issue tags: +Blocks-Layouts
FileSize
2934223-3.patch935 bytes

Here is the patch.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran at PreviousNext commented
FileSize
2934223-4.patch8.29 KB

Moved some other stuff.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran at PreviousNext commented
FileSize
interdiff-20f4b3.txt3.23 KB
2934223-4-1.patch8.65 KB

The last submitted patch, 2: 2934223-3.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 4: 2934223-4-1.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented

Moving code is just the first step. There are tests that need to be changed too.

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented

I think it makes more sense to do one of the following:

  • Make the dependency from the Block module explicit
  • Decouple the Layout Discovery module from the Block module, which means avoiding the first uses a theme implemented by the second
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Issue tags: +Layout Builder stable blocker
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Component: layout.module » layout_builder.module
MerryHamster’s picture

MerryHamster CreditAttribution: MerryHamster at Skilld commented
FileSize
2934223-11.patch91.71 KB

only reroll #4 patch to 8.7.x

MerryHamster’s picture

MerryHamster CreditAttribution: MerryHamster at Skilld commented
FileSize
2934223-12.patch8.79 KB

sorry, #11 I forgot to remove ".orig" files after the patch had applied
here only reroll #4 patch to 8.7.x

MerryHamster’s picture

MerryHamster CreditAttribution: MerryHamster at Skilld commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 12: 2934223-12.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Version: 8.4.x-dev » 8.7.x-dev
Status: Needs work » Needs review

I opened #3003610: Remove block.module dependency from Layout Builder as a follow-up issue. Let's just declare the dependency to resolve the stable blocker, and work on removing it later.
Because in addition to the block template, we also require the local tasks block be available, at least until #2936655: Layout Builder should use the toolbar modes system once it exists. is resolved.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
FileSize
2934223-block-16.patch533 bytes
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Issue summary: View changes
jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran at PreviousNext commented
Status: Needs review » Reviewed & tested by the community

#15 makes sense to me. I think removing dependencies after stable release is not a BC break.

EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc at Acquia commented

FWIW, I 100% support this approach.

Eclipse

  • webchick committed 60b88a7 on 8.7.x 
    Issue #2934223 by jibran, MerryHamster, tim.plunkett, kiamlaluno:...
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Fixed

Makes sense! In #3003610: Remove block.module dependency from Layout Builder it'd be good to provide some rationale as to why this is a good thing to do, but that's separate from this issue which just states the current situation factually. :)

Committed and pushed to 8.7.x. Thanks!

Status: Fixed » Closed (fixed)

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

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented

Sorry; also backported to 8.6.x.