- 🇺🇸United States smustgrave
Now that we have moved block_content we may be able to remove the dependency.
- Merge request !4212Issue #3206952: Why block_content depends on block? → (Open) created by smustgrave
- last update
almost 2 years ago 29,462 pass, 17 fail - last update
almost 2 years ago 29,462 pass, 17 fail - Status changed to Closed: works as designed
almost 2 years ago 10:45pm 19 June 2023 - 🇺🇸United States smustgrave
So tested removing block as a dependency and can already see failures happening.
Seems like it would be more effort to remove block. There's also a preDelete call on the entity
parent::preDelete($storage, $entities); /** @var \Drupal\block_content\BlockContentInterface $block */ foreach ($entities as $block) { foreach ($block->getInstances() as $instance) { $instance->delete(); } }Which seems dependent on block.
Going to say this works as designed if anyone disagrees please reopen explaining why
Thanks!
- 🇦🇺Australia mstrelan
I think it should still be possible to remove the dependency on block.module, it just may need a bit of refactoring.
The \Drupal\block_content\BlockContentInterface::getInstancesmethod (used inpreDelete) provides special handling for block.module but completely ignores layout builder. It probably makes more sense to have a separate service for this integration, and the block instance deletion should probably be implemented in a hook. In other words, theBlockContententity should have no awareness ofBlockentities. - Status changed to Active
almost 2 years ago 12:27am 20 June 2023 - Status changed to Needs review
over 1 year ago 4:13pm 21 December 2023 - Status changed to Needs work
over 1 year ago 5:46am 3 January 2024 - 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
We haven't addressed #11 and #12 yet.
Which leads me to believe we're missing test coverage for that when block.module is disabled.
\Drupal\block_content\Entity\BlockContent::getInstances should hard fail when the block entity-type doesn't exist.
There's also a reference to block.admin_library in block_content.links.action.yml - it would be good to confirm that doesn't cause any side-effects.
block_content_add_action: route_name: block_content.add_page title: 'Add content block' appears_on: - block.admin_library ## here - entity.block_content.collection class: \Drupal\block_content\Plugin\Menu\LocalAction\BlockContentAddLocalAction - Assigned to dpi
- 🇦🇺Australia acbramley
The BlockContentViewBuilder is also tightly coupled to Block module.
And more info in 🐛 Block Content entities have no Contextual links when rendered outside of Block config entity Closed: won't fix
- 🇦🇺Australia acbramley
More coupling in BlockContentForm https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/block...
- 🇦🇺Australia dpi Perth, Australia
Created a new MR @ https://git.drupalcode.org/project/drupal/-/merge_requests/12156
- Generally a lot of reduction and dependency on Block between the two modules, and especially in tests.
- A new small service for usage.
- A new core test module to test navigation blocks, without needing block.module. See also 📌 Reduce dependency on block.module in tests Active .
Implementation notes
BlockContentBrokenTestin block content is testing against\Drupal\Core\Block\Plugin\Block\Broken(in core), this core class relies onadminister blockspermission defined by block. So I've createdadminister_blocks_permission_testtest module to also define aadminister blockspermission so we can test without block.module. I've created 🐛 Dependency from core block plugin to block module Active to address this problem where core is depending on a block.module permission, as of writing currently postponed on ✨ Add a Production/Development Toggle To Core Needs work- A new hooks class added to block.module (
BlockBlockContentHooks) which is conditionally enabled depending on whether block_content.module is enabled. This hooks class has form alter and button callbacks, which functionally was formally located in Block Contents'\Drupal\block_content\BlockContentForm. Associated tests have been moved to Block.module'sBlockContentSaveFormRedirectTest.- The functionality formerly provided by
\Drupal\block_content\Entity\BlockContent::getInstancesand\Drupal\block_content\Form\BlockContentDeleteFormmoved to the three classes adjacent to\Drupal\block_content\BlockUsage\BlockContentBlockUsage- I considered making this a feature of block.module, but decided that BlockContent should still be responsible for these messages, but split out of the entity class.
- A service was created, accessible at
Drupal\block_content\BlockUsage\BlockContentBlockUsageInterface - The service is conditionally available, depending on whether block.module is also installed (
BlockContentCompilerPass). See new test coverage atBlockContentBlockUsageTest - A mulipurpose iterable/countable class provides the necessary block config producing and efficient counting functionality that
BlockContentDeleteFormandBlockContent::preDeleterequire. BlockContentDeleteFormandBlockContent::preDeleteare still retain responsibility for calling out to this service.- Ive made
BlockContentUsageListfinal for now. It should be un-finalised only if an interface is extracted, in the future. \Drupal\block_content\Entity\BlockContent::getInstancesdeprecated, change notice at https://www.drupal.org/node/3523951 →
- A new hooks class added to block_module.module (
BlockContentBlockHooks) which is conditionally enabled depending on whether block.module is enabled. This hooks class provides the existing hooks to modify theblockhook_theme/template and modify block config entity operations. Hopefully the block preprocess hook can be moved out when 📌 Remove block.module dependency from Layout Builder Needs work finishes decouplingblockhook_theme/template from block.module to core/system.module.- BlockContent's
#[Hook('help')]modified with various decouplings and variable-izing permission names. One pieces of help text used to referenceadminister blocks, but was incorrect since permission changes in ✨ Add more granular block content permissions Fixed .- Changed
BlockContentBlock::buildto no longer rely onadminister blocks, but onUrl->access(). Looks like the permission was no longer correct anyway, since permission changes in ✨ Add more granular block content permissions Fixed .- Generally, many Block Content Kernel and Functional tests no longer need block.module to be installed, both explicitely and implicitely by
\Drupal\Tests\block_content\Functional\BlockContentTestBase.- I re-evaluated all Kernel/Functional tests in Block Content, and updated them the explicit permissions the test user needs, instead of blanket permissions from
BlockContentTestBase. This includesadminister blocksspecifically. - There were many uses of
placeBlockto place title/actions/tasks on pages, which tests used to click links. However this method requires block.module, which we're trying to prevent unnecessary dependencies on. So I've introduced thenavigation_variant_testtest module which can render navigation blocks without block.module. I've created 📌 Reduce dependency on block.module in tests Active to expand usage of this method so other parts of core can reduce need of block.module viaplaceBlock.
- 🇦🇺Australia dpi Perth, Australia
Clarifying title: its block module (not block config or theme_hook:block).
I think though we're not "removing" a dependency, but rather block or block content are conditionally enhancing the other module depending on install state.
-
Updated issue summary a bit.
- 🇦🇺Australia acbramley
I've gone through the MR quite thoroughly and generally +1 this overall approach although it is quite complicated and may be hard for some devs to grok, especially the compiler pass stuff (queue many "why isn't this working?!")
My main concerns are:
1. The administer_blocks_permission_test module, although very minor it just doesn't feel right to add a duplicate permission, this is a big nit picky though
2. navigation_variant_test - now tests that just needed 1 block get all the additional elements rendered. Again this may be minor. I haven't ever looked into DisplayVariant plugins before though so thanks for the learning :) - 🇦🇺Australia dpi Perth, Australia
A couple q's for @larowlan. Remaining open threads I dont see as actionable/my implementation notes.
