Problem/Motivation

Layout Builder field blocks show up on Block UI when they shouldn't

This is because Block UI has a fake node(and user?) context.

Proposed resolution

@tim.plunkett suggested that Block UI would filter out Blocks that have require contexts.

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tedbow created an issue. See original summary.

tedbow’s picture

Issue summary: View changes

Removed the FieldBlock module mention from the summary. Tested it and it doesn't work this way.

tedbow’s picture

Status: Active » Needs review
FileSize
2.23 KB

Patch from @tim.plunkett's work

Status: Needs review » Needs work

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

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tim.plunkett’s picture

Bump

tim.plunkett’s picture

Added a test.
FAIL patch is the interdiff.

The last submitted patch, 7: 2968139-block_ui-7-FAIL.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 7: 2968139-block_ui-7-PASS.patch, failed testing. View results

tim.plunkett’s picture

Ahh, so we have a test that asserts that user fields can be placed in the Block UI when Layout Builder is installed.

This test was written to capture any bugs with the complex AJAX interactions when displaying field formatter settings within the block form.
All of the assertions could instead be done for a Layout Builder architecture instead of Block UI, but it was nice to have a second implementation to test against.

Any advice?

tim.plunkett’s picture

tedbow’s picture

+++ b/core/modules/block/block.module
@@ -284,3 +284,25 @@ function block_configurable_language_delete(ConfigurableLanguageInterface $langu
+    // Filter out any definition with required contexts.
+    elseif (!empty($definition['context'])) {
+      /** @var \Drupal\Core\Plugin\Context\ContextDefinitionInterface $context */
+      foreach ($definition['context'] as $context) {
+        if ($context->isRequired()) {
+          unset($definitions[$id]);
+          break;
+        }
+      }

This seems to disruptive. Couldn't a contrib module provide to UI for providing contexts if needed or have some other logic for providing contexts to the block UI?

It seems likes we should only be removing layout builder provided blocks here and the Block module should implement this logic if needed.

tedbow’s picture

Re #10

  1. It would be nice to have a second implementation but since the layout builder also will opens block in dialog I think we still getting complex test scenario for the ajax if we switch to using layout builder. Also the block UI doesn't open up the configure link in the a dialog so Layout Builder case is more complex I think.
  2. But if the patch was changed to my suggestion in #12 then quick way to test would be to create a test module that has empty block plugin class that extends \Drupal\layout_builder\Plugin\Block\ExtraFieldBlock with only the annotation id changed. That way they won't be removed and we could test the blocks plugins in the Block UI.
tedbow’s picture

Status: Needs work » Needs review
FileSize
5.88 KB
4.85 KB
  1. re #12 I change this to be in the layout_builder module and only unset where the provider is layout_builder
  2. Added the test I suggested in #13.2 but it extends Drupal\layout_builder\Plugin\Block\FieldBlock not ExtraFieldBlock. That was a mistake. The test fails because of this error..

    The website encountered an unexpected error. Please try again later.Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for block.block.datefield with the following errors: block.block.datefield:settings.formatter missing schema in Drupal\Core\Config\Development\ConfigSchemaChecker->onConfigSave() (line 95 of core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php).

    call_user_func(Array, Object, 'config.save', Object) (Line: 111)
    I am not sure why. I don't see layout_builder setting this in the schema so not sure why the test module would be fail and the regular blocks module won't. Probably missing something simple.

Status: Needs review » Needs work

The last submitted patch, 14: 2968139-14.patch, failed testing. View results

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
5.4 KB
570 bytes

The schema expects field_block, we need it to also count when it is field_block_test.

tim.plunkett’s picture

Discussed with @phenaproxima, and decided the trickiness around the subclass of FieldBlock deserved more explanation.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Perfect. Thanks, @tim.plunkett. I understand the patch now and feel fine RTBCing it.

catch’s picture

Version: 8.7.x-dev » 8.6.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 87646bb85a to 8.7.x and bd32e2e374 to 8.6.x. Thanks!

  • catch committed 87646bb on 8.7.x
    Issue #2968139 by tim.plunkett, tedbow: Layout Builder field blocks show...

  • catch committed bd32e2e on 8.6.x
    Issue #2968139 by tim.plunkett, tedbow: Layout Builder field blocks show...
tim.plunkett’s picture

Status: Fixed » Closed (fixed)

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