Repository navigation
fix: Layout Builder context handling + split responsibilities - #31
Conversation
Mirror the target plugin's context definitions on the proxy so Layout Builder applies section-storage contexts to it, then forward those contexts to the target at build time. Route the nested block-config sub-form through SubformState during validate/submit so target plugins see the correct values, and read block_config from its actual nested path. Honor Layout Builder's gathered_contexts form-state value in the context manager to keep proxied placements in parity with native ones.
Audit + improvements to context handling in ProxyBlock, learning from the Lullabot/ab_tests ab_blocks submodule. - ProxyBlock::build() forwarding loop now guards each setContext() with a per-context try/catch (ContextException) and a logged notice, so one type mismatch can't tear down the whole render. - TargetBlockFormProcessor::submitTargetBlock() no longer wipes a saved context_mapping to [] when the inner mapping element was intentionally suppressed inside Layout Builder. - TargetBlockContextManager narrows the broad catch(\Exception) to catch(ContextException) with logger notices; non-context errors now surface instead of being silently swallowed. Empty else and dead branches removed. - Added resolveRootEntityContext() helper that probes ['layout_builder.entity', 'entity', 'node'] and is used as a build()-time fallback so targets needing the conventional 'entity'/'node' name still resolve under LB. - Memoized blockManager->getDefinition() via the new getTargetPluginDefinition() helper; getContextDefinitions() and the admin-mode label lookup now share one call per build. - Injected a logger.channel.proxy_block on ProxyBlock and TargetBlockContextManager. Tests: - New kernel test ProxyBlockContextDefinitionsTest covering target-mirrored names, missing/stale target, parent fallback, unknown-name ContextException. - Updated unit-test base for the new constructor dependencies; replaced the swallow-broad-Exception tests with three sharper ones pinning the new ContextException-only semantics.
Splits responsibilities out of the ProxyBlock plugin so the constructor goes from 11 args to 5. - New ProxyBlockRenderer service absorbs build() pipeline, cache-metadata collection, target plugin definition lookup (memoized), Layout Builder admin-mode preview, and context forwarding with the root-entity fallback. - TargetBlockFormProcessor gains getAvailableBlockOptions() so the plugin no longer depends on BlockManagerInterface. - ProxyBlock now holds only TargetBlockFormProcessor and ProxyBlockRenderer; build(), getCacheContexts/Tags/MaxAge and getContextDefinitions() delegate. - Renderer takes plain arguments (config, contexts, parent cache contributions, plugin id, CacheableDependencyInterface) instead of the final ProxyBlock instance so it can be unit-tested in isolation. - New ProxyBlockRendererTest exercises render() and the cache collection methods; ProxyBlockTest rewritten to assert pure delegation through mocks of the two collaborators.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the ProxyBlock plugin into a thin shell, delegating rendering and form processing to new ProxyBlockRenderer and TargetBlockFormProcessor services. It also enhances context handling and adds comprehensive unit and kernel test coverage. The reviewer provided actionable feedback regarding optimizing AJAX configuration updates, reducing log noise during context forwarding, improving the robustness of Layout Builder route detection, and refining the configuration fallback logic in the form processor.
| $config = $this->getConfiguration(); | ||
|
|
||
| $block_definitions = $this->blockManager->getDefinitions(); | ||
| $block_options = array_map( | ||
| static fn ($definition) => $definition['admin_label'] ?? $definition['id'], | ||
| array_filter( | ||
| $block_definitions, | ||
| fn ($definition, $plugin_id) => $plugin_id !== $this->getPluginId(), | ||
| ARRAY_FILTER_USE_BOTH | ||
| ) | ||
| ); | ||
| $block_options = array_unique($block_options); | ||
| asort($block_options); | ||
|
|
||
| $block_options = $this->formProcessor->getAvailableBlockOptions($this->getPluginId()); |
There was a problem hiding this comment.
When the target block is changed via AJAX in the configuration form, the plugin instance's configuration should be updated immediately. This ensures that getContextDefinitions() (which is called during the form rebuild) returns the correct definitions for the newly selected target. Without this, Layout Builder's root-level context mapping UI will continue to show contexts for the previously saved target block until the form is saved.
if ($selected = $this->formProcessor->getSelectedTargetFromFormState($form_state)) {
$this->setConfiguration(['target_block' => $selected] + $this->getConfiguration());
}
$config = $this->getConfiguration();
$block_options = $this->formProcessor->getAvailableBlockOptions($this->getPluginId());| foreach ($proxy_contexts as $name => $context) { | ||
| try { | ||
| $target_block->setContext($name, $context); | ||
| } | ||
| catch (ContextException $e) { | ||
| $this->logger->notice('Proxy block could not forward context "@name" to target @plugin: @message', [ | ||
| '@name' => $name, | ||
| '@plugin' => $target_block->getPluginId(), | ||
| '@message' => $e->getMessage(), | ||
| ]); | ||
| } | ||
| } |
There was a problem hiding this comment.
To avoid excessive log noise, check if the target block actually defines the context before attempting to set it. Since the proxy block often has access to more contexts (e.g., from Layout Builder section storage) than the target block requires, calling setContext() for every available context will trigger a ContextException and a logged notice for every mismatch on every render.
$target_definitions = $target_block->getContextDefinitions();
foreach ($proxy_contexts as $name => $context) {
if (isset($target_definitions[$name])) {
try {
$target_block->setContext($name, $context);
}
catch (ContextException $e) {
$this->logger->notice('Proxy block could not forward context "@name" to target @plugin: @message', [
'@name' => $name,
'@plugin' => $target_block->getPluginId(),
'@message' => $e->getMessage(),
]);
}
}
}| protected function isLayoutBuilderAdminRoute(): bool { | ||
| $request = $this->requestStack->getCurrentRequest(); | ||
| if (!$request) { | ||
| return FALSE; | ||
| } | ||
| $path = $request->getPathInfo(); | ||
| if (str_contains($path, '/admin/structure/types/') | ||
| && str_contains($path, '/display/') | ||
| && str_contains($path, '/layout') | ||
| ) { | ||
| return TRUE; | ||
| } | ||
| if (str_contains($path, '/layout_builder/update/block/') | ||
| || str_contains($path, '/layout_builder/add/block/') | ||
| ) { | ||
| return TRUE; | ||
| } | ||
| $destination = $request->query->get('destination'); | ||
| return is_string($destination) && str_contains($destination, '/layout'); | ||
| } |
There was a problem hiding this comment.
Detecting Layout Builder admin routes via path string matching is fragile and incomplete. It currently only supports Node types and will fail for other entity types (e.g., Taxonomy terms, Media, Users) where Layout Builder might be enabled. A more robust approach is to check the route name or the presence of the section_storage attribute in the request.
protected function isLayoutBuilderAdminRoute(): bool {
$request = $this->requestStack->getCurrentRequest();
if (!$request) {
return FALSE;
}
$route_name = $request->attributes->get('_route');
if ($route_name && (str_starts_with($route_name, 'layout_builder.') || $request->attributes->has('section_storage'))) {
return TRUE;
}
$destination = $request->query->get('destination');
return is_string($destination) && str_contains($destination, '/layout');
}| $block_config = $form_state->getValue(['target_block', 'config', 'block_config']) | ||
| ?? $configuration['target_block']['config'] | ||
| ?? []; |
There was a problem hiding this comment.
The fallback to saved configuration should only occur if the target block ID has not changed. If the user selects a different target block via AJAX, the saved configuration for the previous block is likely incompatible and should not be used as a fallback during validation.
$block_config = $form_state->getValue(['target_block', 'config', 'block_config']);
if ($block_config === NULL) {
$saved_id = $configuration['target_block']['id'] ?? NULL;
$block_config = ($target_block_plugin === $saved_id) ? ($configuration['target_block']['config'] ?? []) : [];
}
|
🎉 This PR is included in version 1.6.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Closes #30.
Summary
Three commits on top of
main:4276e71fix: integrate proxy block with Layout Builder — initial LB integration. Exposes target plugin's context definitions on the proxy, forwards section-storage contexts to the target at render time, and suppresses the duplicate innercontext_mappingelement when LB owns it at the form root. PlumbsSubformStatethrough validate/submit for the nested target sub-form.0e28b65fix: harden Layout Builder context handling — audit + fixes informed by reviewing the sibling Lullabotab_testsab_blockssubmodule:build()context forwarding now wraps eachsetContext()intry/catch (ContextException)with a logged notice — one type mismatch can no longer tear down the render.TargetBlockFormProcessor::submitTargetBlock()no longer clobbers a savedcontext_mappingwith[]when the inner mapping element was intentionally suppressed inside LB.TargetBlockContextManagernarrows the broadcatch (\Exception)tocatch (ContextException)with logger notices; non-context errors propagate instead of being silently swallowed. Emptyelse {}and dead branches removed.resolveRootEntityContext()helper probes['layout_builder.entity', 'entity', 'node']and is used as a render-time fallback so targets needing the conventionalentity/nodename still resolve under LB.blockManager->getDefinition()via agetTargetPluginDefinition()helper —getContextDefinitions()and the admin-mode label lookup now share a single call per build.logger.channel.proxy_blockonProxyBlockandTargetBlockContextManager.ProxyBlockContextDefinitionsTestcovering target-mirrored names, missing/stale target, parent fallback, and unknown-nameContextException.f98dcdcrefactor: slim ProxyBlock to delegate-only plugin — splits responsibilities. NewProxyBlockRendererservice absorbs thebuild()pipeline, cache-metadata collection, target-plugin-definition memoization, LB admin-mode preview, and context forwarding (incl. root-entity fallback).TargetBlockFormProcessorgainsgetAvailableBlockOptions()so the plugin no longer depends onBlockManagerInterface. TheProxyBlockplugin constructor drops from 11 args to 5: the 3 mandatory plugin args plusTargetBlockFormProcessorandProxyBlockRenderer. The renderer takes plain arguments (config, contexts, parent cache contributions, plugin id,CacheableDependencyInterface) rather than the (final) plugin instance, so it can be unit-tested in isolation. NewProxyBlockRendererTest;ProxyBlockTestrewritten to assert pure delegation.Test plan
vendor/bin/phpcs --standard=Drupal,DrupalPractice ... web/modules/contrib/proxy_block/{src,tests}— clean.vendor/bin/phpstan analyze --configuration=web/modules/contrib/proxy_block/phpstan.neon web/modules/contrib/proxy_block/src— no errors.vendor/bin/phpunit -c web/core/phpunit.xml.dist web/modules/contrib/proxy_block/tests/src/Unit— 82 tests, 214 assertions, all green.SIMPLETEST_DB):vendor/bin/phpunit -c web/core/phpunit.xml.dist web/modules/contrib/proxy_block/tests/src/Kernel— to be run in CI./admin/structure/block) and confirm the inner context-mapping element is rendered and saved values are preserved when the same placement is later opened in LB.