Problem/Motivation

If a block defines contexts in the annotation, the block edit form will add a context mapping form element to be used to map available contexts to the ones the block can use. This is fine.

However, if the block defines a non-required context and the available contexts are not suitable for that block (none are valid for it), the block form context mapper will be an empty select list (no option to choose from).

Proposed resolution

Do not show the context mapper form element if there are no options to choose from.

Remaining tasks

  • Create patch to adjust ContextAwarePluginAssignmentTrait::addContextAssignmentElement() so that it handles this case.
  • Test

User interface changes

Context mapper will only show if there are (valid) contexts available to be mapped.

API changes

None

Data model changes

None

CommentFileSizeAuthor
#19 2809305-19.patch3.11 KBUpchuk
#19 interdiff-2809305-19.txt820 bytesUpchuk
#17 interdiff-2809305-17.txt849 bytesUpchuk
#17 2809305-17.patch2.92 KBUpchuk
#14 interdiff-2809305-14.txt3.04 KBUpchuk
#14 2809305-14.patch2.97 KBUpchuk
#12 2809305-12.patch2.96 KBUpchuk
#12 interdiff-2809305-12.txt2.74 KBUpchuk
#8 blockinterdiff.txt658 bytesPavan B S
#8 2809305-8.patch2.97 KBPavan B S
#7 2809305-7.patch2.97 KBjofitz
#2 2809305-2.patch2.99 KBUpchuk
#2 2809305-2-tests.patch2.2 KBUpchuk
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Upchuk created an issue. See original summary.

Upchuk’s picture

Upchuk CreditAttribution: Upchuk at Wunder commented
Status: Needs work » Needs review
FileSize
2809305-2-tests.patch2.2 KB
2809305-2.patch2.99 KB

And the patch.

The last submitted patch, 2: 2809305-2-tests.patch, failed testing.

Upchuk’s picture

Upchuk CreditAttribution: Upchuk at Wunder commented
Assigned: Upchuk » Unassigned

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Needs work

Not 100% sure, there are also issues that suggest to show that form element always, otherwise you don't even know that you could provide something.

  1. +++ b/core/modules/block/tests/modules/block_test/src/Plugin/Block/TestContextAwareNoValidContextOptionsBlock.php
@@ -0,0 +1,38 @@
+
+/**
+ * Provides a context-aware block that uses a non-required context which
+ * is not being passed to it.

    this should afaik be a single line. 

  2. +++ b/core/modules/block/tests/modules/block_test/src/Plugin/Block/TestContextAwareNoValidContextOptionsBlock.php
@@ -0,0 +1,38 @@
+   * {@inheritdoc}
+   */
+  protected function blockAccess(AccountInterface $account) {
+    return parent::blockAccess($account);
+  }

    not needed.

jofitz’s picture

jofitz CreditAttribution: jofitz at ComputerMinds commented
Status: Needs work » Needs review
FileSize
2809305-7.patch2.97 KB

Re-roll including both changes suggested by @Berdir in #6.

Pavan B S’s picture

Pavan B S CreditAttribution: Pavan B S at Valuebound commented
FileSize
2809305-8.patch2.97 KB
blockinterdiff.txt658 bytes

Made minor change of short array syntax, applying the patch, please review

Version: 8.4.x-dev » 8.5.x-dev

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

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

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

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.

Upchuk’s picture

Upchuk CreditAttribution: Upchuk as a volunteer commented
Issue tags: -Dublin2016
FileSize
interdiff-2809305-12.txt2.74 KB
2809305-12.patch2.96 KB

Rerolling, let's see.

Status: Needs review » Needs work

The last submitted patch, 12: 2809305-12.patch, failed testing. View results

Upchuk’s picture

Upchuk CreditAttribution: Upchuk as a volunteer commented
Status: Needs work » Needs review
FileSize
2809305-14.patch2.97 KB
interdiff-2809305-14.txt3.04 KB

Aha, it seems #3014949: Deprecate 'context' on Block and Condition plugin annotations in favor of 'context_definitions' changed the context definitions annotation :) Here is the updated patch.

Status: Needs review » Needs work

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

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented

The patch is passing a message as the second param of fieldNotExists(), but that's not supported. Just removing that should be enough.

Upchuk’s picture

Upchuk CreditAttribution: Upchuk as a volunteer commented
Status: Needs work » Needs review
FileSize
2809305-17.patch2.92 KB
interdiff-2809305-17.txt849 bytes

Ah yes, indeed. Missed that, thanks!

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented 
+++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginAssignmentTrait.php
@@ -48,7 +48,7 @@ protected function addContextAssignmentElement(ContextAwarePluginInterface $plug
-      if (count($options) > 1 || !$definition->isRequired()) {
+      if (count($options) > 1 || (count($options) == 1 && !$definition->isRequired())) {

Sorry I didn't ask for this sooner. But can you add a comment here explaining what this is checking for? Essentially that if options is 0 it should never show, and that it should only show for the 1 option if it is optional. (Which makes this logic weird, since it's the inverse).
The original issue that added the OR case should have included a comment but didn't and so it falls to this issue.

Upchuk’s picture

Upchuk CreditAttribution: Upchuk as a volunteer commented
FileSize
interdiff-2809305-19.txt820 bytes
2809305-19.patch3.11 KB

Alrighty, here we are.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Needs review » Reviewed & tested by the community

Thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 55f13aacb5 to 8.7.x and c490e4029c to 8.6.x. Thanks!

Backported to 8.6.x after running core/modules/block/tests/src/Functional/BlockUiTest.phplocally.

  • alexpott committed 55f13aa on 8.7.x 
    Issue #2809305 by Upchuk, Pavan B S, Jo Fitzgerald, tim.plunkett, Berdir...

  • alexpott committed c490e40 on 8.6.x 
    Issue #2809305 by Upchuk, Pavan B S, Jo Fitzgerald, tim.plunkett, Berdir...

Status: Fixed » Closed (fixed)

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

super_romeo’s picture

super_romeo CreditAttribution: super_romeo commented
Related issues: +#3251613: Block context select box should be visible even it is only one option to choose