Expose Block Context mapping in the UI

1. +++ b/core/lib/Drupal/Core/Block/BlockBase.php
   @@ -160,6 +164,9 @@ protected function blockAccess(AccountInterface $account) {
   +    $temporary = $form_state->getTemporary();
   +    $contexts = isset($temporary['gathered_contexts']) ? $temporary['gathered_contexts'] : [];
   +    $form['context_mapping'] = $this->addContextAssignmentElement($this, $contexts);
   

   Wow this is ugly. Another +1 for #2371853: Add more helper methods around temporary FAPI storage

2. +++ b/core/modules/block/src/BlockForm.php
   @@ -310,7 +310,12 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +      $block->setContextMapping($context_mapping);
   

   Why do we need this here?

3. +++ b/core/modules/block/src/BlockRepository.php
   @@ -77,6 +77,11 @@ public function getVisibleBlocksPerRegion(array $contexts) {
   +      $block->setAvailableContexts($contexts);
   +      $block_plugin = $block->getPlugin();
   +      if ($block_plugin instanceof ContextAwarePluginInterface) {
   +        $this->contextHandler->applyContextMapping($block_plugin, $contexts);
   +      }
   

   Still don't need this hunk. It's done in BlockAccessControlHandler, which is triggered by the access call two lines away.

We have TestContextAwareBlock in block_test available for tests.

You're right on 3.2, that's my bad.

And you're also right, 3.3 is needed, but it should move into BlockAccessControlHandler, right above/below where condition context is applied.

This will speed things up for disabled blocks (the first check in BlockAccessControlHandler).

I have some fixes for ContextHandler, but they're out of scope for this issue and need to go into a blocker, which I'll open tomorrow.

1. +++ b/core/modules/block/src/BlockForm.php
   @@ -310,7 +310,12 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +      $context_mapping = $settings->hasValue('context_mapping') ? $settings->getValue('context_mapping') : [];
   

   You can just do $context_mapping = $settings->getValue('context_mapping', []);

2. +++ b/core/modules/block/src/BlockRepository.php
   @@ -76,6 +77,11 @@ public function getVisibleBlocksPerRegion(array $contexts) {
   +      $block->setContexts($contexts);
   ...
          if ($block->setContexts($contexts)->access('view')) {
   

   We have setContexts() twice now.

some minor nitpicks

1. +++ b/core/modules/aggregator/src/EventSubscriber/AggregatorFeedContext.php
   @@ -0,0 +1,86 @@
   + * Contains AggregatorFeedContext.php.
   

   Needs to be Contains \Full\Path\To\Class and no .php

2. +++ b/core/modules/aggregator/src/EventSubscriber/AggregatorFeedContext.php
   @@ -0,0 +1,86 @@
   +class AggregatorFeedContext extends BlockContextSubscriberBase {
   

   Needs a docblock

3. +++ b/core/modules/aggregator/src/EventSubscriber/AggregatorFeedContext.php
   @@ -0,0 +1,86 @@
   +   * Constructs a new NodeRouteContext.
   

   c/p fail?

1. +++ b/core/modules/aggregator/src/EventSubscriber/AggregatorFeedContext.php
   @@ -0,0 +1,86 @@
   +        list(, $fid) = explode(':', $contexts['feed']);
   

   We don't have any sanity checks around the contents of $contexts, that feels wrong.

Aggregator doesn't depend on block, this has to be optional :(

Also, do we really want to load all of these on runtime?

1) If you're going to check $feeds, you should actually add stuff to it... Leaving out for now.

2) I meant to put this in aggregator.module

Womp womp #2363351: Cached services can't be used in service providers/modifiers

Also #2371853: Add more helper methods around temporary FAPI storage went in, so we can rewrite the temporary FAPI calls.

1. +++ b/core/modules/aggregator/src/Plugin/Block/AggregatorFeedBlock.php
   @@ -152,7 +133,8 @@ public function blockSubmit($form, FormStateInterface $form_state) {
   -    if ($feed = $this->feedStorage->load($this->configuration['feed'])) {
   ...
   +    if ($feed = $this->getContextValue('feed')) {
   
   @@ -194,7 +176,7 @@ public function build() {
   +    $feed = $this->getContextValue('feed');
   

   Seems strange that we wrap this in an if() in one method but not the other.

2. +++ b/core/modules/block/src/BlockRepository.php
   @@ -76,8 +77,13 @@ public function getVisibleBlocksPerRegion(array $contexts) {
   +      $block->setContexts($contexts);
   ...
   -      if ($block->setContexts($contexts)->access('view')) {
   +      if ($block->access('view')) {
   

   This change is rather pointless, it doesn't matter when we call this as long as it's before access()

Since this touches the UI, needs to be checked over for Usability. We also need a beta phase evaluation template for this issue.

I tried to give the patch a whirl, but can't seem to see an Aggregator/Feed block no matter what I do. Steps:

1) Apply the latest patch
2) drush si
3) drush en aggregator
4) admin/config/services/aggregator
5) Add a feed to http://rss.cbc.ca/lineup/topstories.xml (Canada FTW)
6) admin/structure/block

No errors, just no block is visible.

Thanks for pushing this forward EclipseGC! For panels this is a critical component for us that really shouldn't live in contrib.

works well with #2287073-12: Allow views contextual filters to expose the context using argument validation plugins

Updated remaining tasks per EclipseGC at SCOTCH meeting.

Our previous test coverage was proving that we didn't support context-aware blocks.
In order to maintain our test coverage now that we do support them, I added a block that has an unsatisfied context (entity:foobar), and tweaked the original test.

Can we have a couple of screenshots?

Thnx! I had no clue what the patch was doing. This is awesome btw!

Thanks for #48. Awesome explanation!

IMHO it'd be better if at https://www.drupal.org/files/issues/06%20Block%20Config%20-%20post%20pat... we'd show that "Drupal Planet" was the feed that was used for this block. Otherwise, the user doesn't know this block is associated with that feed, which may lead the user to think that it'll show all feeds; i.e. that once an additional feed is created, it will also be listed in the block.

#52: Yes, using '#type' => 'item' or something like it.

Here's an initial review.

1. +++ b/core/lib/Drupal/Core/Annotation/ContextDefinition.php
   @@ -100,7 +100,7 @@ public function __construct(array $values) {
   -      if (isset($values[$key]) && $values[$key] instanceof TranslationWrapper) {
   +      if (isset($values[$key]) && $values[$key]->get() instanceof TranslationWrapper) {
   

   Interesting change. Does this point to a gap in HEAD's test coverage?

2. +++ b/core/modules/aggregator/config/schema/aggregator.schema.yml
   @@ -44,6 +44,10 @@ block.settings.aggregator_feed_block:
   +        feed:
   +          type: string
   +          label: 'Feed'
   

   This will actually contain a UUID, right? I know we don't have a specific config schema type for that, but a comment noting that here would be welcome.

3. +++ b/core/modules/aggregator/src/EventSubscriber/AggregatorFeedContext.php
   @@ -0,0 +1,88 @@
   +   * Constructs a new NodeRouteContext.
   

   C/p remnant.

4. +++ b/core/modules/aggregator/src/EventSubscriber/AggregatorFeedContext.php
   @@ -0,0 +1,88 @@
   +        $context = new Context(new ContextDefinition('entity:aggregator_feed'));
   +        $context->setContextValue($feed);
   +        $event->setContext('aggregator.feed:' . $feed->id(), $context);
   ...
   +      $context = new Context(new ContextDefinition('entity:aggregator_feed', $feed->label()));
   +      $context->setContextValue($feed);
   +      $event->setContext('aggregator.feed:' . $feed->id(), $context);
   

   I don't know contexts well, so please forgive any stupid mistakes.

   But if I understand this patch well, then classes like this one (AggregatorFeedContext) will become pretty widespread. Which means their DX is important.

   I think it's very confusing that these 3 lines are duplicated between these 2 required methods. With only a tiny subtle difference. It's not at all clear why that difference exists.

5. +++ b/core/modules/aggregator/src/EventSubscriber/AggregatorFeedContext.php
   @@ -0,0 +1,88 @@
   +        list(, $fid) = explode(':', $contexts['feed'], 2);
   

   And this feels like dark magic.

   Needs documentation to explain what it's doing.

6. +++ b/core/modules/aggregator/src/Plugin/Block/AggregatorFeedBlock.php
   @@ -23,19 +22,16 @@
   + *   context = {
   + *     "feed" = @ContextDefinition("entity:aggregator_feed", label = @Translation("Aggregator feed"), description = @Translation("Select the feed that should be displayed"))
   + *   }
   

   We always format these across multiple lines.

7. +++ b/core/modules/aggregator/src/Plugin/Block/AggregatorFeedBlock.php
   @@ -95,7 +87,6 @@ public function defaultConfiguration() {
   -      'feed' => NULL,
   

   Nice :)

8. +++ b/core/modules/aggregator/src/Plugin/Block/AggregatorFeedBlock.php
   @@ -152,40 +131,40 @@ public function blockSubmit($form, FormStateInterface $form_state) {
   -    if ($feed = $this->feedStorage->load($this->configuration['feed'])) {
   

   Even nicer!

9. +++ b/core/modules/aggregator/src/Plugin/Block/AggregatorFeedBlock.php
   @@ -194,7 +173,7 @@ public function build() {
   -    $feed = $this->feedStorage->load($this->configuration['feed']);
   +    $feed = $this->getContextValue('feed');
   

   Now needs IDE hint.

Thanks for asking for review :)

I am really happy to see this patch, and it looks like its 90% there in terms of the UI approach.

WimLeers is correct in stating that we should inform the user about the connected feed, even when there is only one. This sets the expectation for the next one that you can select it, and informs the user about the content of the block. Which is crucial for understanding the underlying concepts.

In terms of the labeling, I suggest to go with:

"Feed" - no description.

The rest should probably be a given, the point of a selector is to "select" something so you don't have to mention it and the point of a block is to "display" something. So if you connect the concept of which feed, you should be fine from a informing the user point of view of the result.

I think the aggregator block context should also include block_count - ie. everything from AggregatorFeedBlock::blockForm()? Also the schema for the context mapping should be defined by the block.settings.aggregator_feed_block schema.

1. +++ b/core/modules/aggregator/src/EventSubscriber/AggregatorFeedContext.php
   @@ -0,0 +1,88 @@
   +        $contexts = $block_plugin->getContextMapping();
   +        list(, $fid) = explode(':', $contexts['feed'], 2);
   +        $feed = $this->feedStorage->load($fid);
   +        $context = new Context(new ContextDefinition('entity:aggregator_feed'));
   +        $context->setContextValue($feed);
   +        $event->setContext('aggregator.feed:' . $feed->id(), $context);
   

   I must say it's really strange to me that the context provider class is responsible of injecting the context into the block plugin. There should be a separate component that's in charge of applying context mappings based on available context. The job of this class is providing context and that should be it.

   More generally, I do not think it makes much sense to expose the context of each entity globally as done for feed entities in this patch via "agreegator.feed:{ID}" - that way we'd end up with tons of contexts. Instead, that's actually a case where context is full-filled without a context mapping, but by directly provided value by the user (direct input value in Rules slang). For solving that properly adding something along the lines of registering "static context" or more sophisticated context configuration processing in order to support "direct input values" would be required. But imho that's far out of scope for this patch - so maybe we should come up with a better, easier to solve example here?
   One I could think of in core is "users", on the user profile page you have both the context of the viewed and of the current user available.

2. +++ b/core/modules/aggregator/src/Tests/DeleteFeedTest.php
   @@ -30,10 +30,10 @@ function testDeleteFeed() {
   -    $block->getPlugin()->setConfigurationValue('feed', $feed1->id());
   +    $block->getPlugin()->setContextMapping(['feed' => 'aggregator.feed:' . $feed1->id()]);
   

   This highlights perfectly that there shouldn't be a hard separation between context vs configuration parameter. Also see #1976758: Plugins miss metadata about configuration.

I think the aggregator block context should also include block_count - ie. everything from AggregatorFeedBlock::blockForm()? /blockquote>
Yep, exactly. That's how the system really ends up being powerful (and Rules works) - but that's a bit off-topic here.

Here is a re-roll of the patch. Not sure why, but the annotation doesn't appear to be working. Following webchick's steps in #30, I get to this on the block:

After talking a bit with EclipseGC, I've re-rolled again, where it seems to be working correctly with feed aggregator, it also fixes a problem introduced with #1763964: Use #type => link for theme_aggregator_block_item() -- because it defines the link incorrectly (uses href instead of url)

We added a markup value when only one context is available instead of hiding it from the user. Adding this also shows up in conditions, so we know which contexts are being added.

This patch takes a slightly different approach to #2429501: AggregatorFeedBlock does not output item links, which should get committed before this.

1. +++ b/core/lib/Drupal/Core/Annotation/ContextDefinition.php
   @@ -100,7 +100,7 @@ public function __construct(array $values) {
   -      if (isset($values[$key]) && $values[$key] instanceof TranslationWrapper) {
   +      if (isset($values[$key]) && $values[$key]->get() instanceof TranslationWrapper) {
   

   Points to missing test coverage?

   Already remarked in #56.1… but never addressed.

2. +++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginAssignmentTrait.php
   @@ -51,9 +51,17 @@ protected function addContextAssignmentElement(ContextAwarePluginInterface $plug
   +          '#title' => $this->t('@context value:', ['@context' => ucfirst($context_slot)]),
   

   Is the ucfirst appropriate? Isn't this enforcing the rules for English?

3. +++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginAssignmentTrait.php
   @@ -51,9 +51,17 @@ protected function addContextAssignmentElement(ContextAwarePluginInterface $plug
   +        // Remove the default value title because we have more than one value.
   

   "default value" vs "context slot" -> strange

4. +++ b/core/modules/aggregator/config/schema/aggregator.schema.yml
   @@ -44,6 +44,10 @@ block.settings.aggregator_feed_block:
   +    context_mapping:
   +      type: mapping
   +      label: 'Context assignments'
   +      mapping:
   ...
   +          type: string
   +          label: 'Feed'
   

   See #56.2 + #57.2. Also never addressed.

   In fact, I see that *nothing* in #56 was ever addressed. The points were replied to, but the patch was never updated. Marking NW for that.

   If we keep writing reviews without those reviews being actually addressed, we're going to be stuck in an endless loop here.

5. +++ b/core/modules/block/src/EventSubscriber/NodeRouteContext.php
   @@ -58,7 +61,7 @@ public function onBlockActiveContext(BlockContextEvent $event) {
   +    $context = new Context(new ContextDefinition('entity:node', $this->t('Node from route')));
   

   Where is this translated string exposed? "route" likely is a too technical term.

1. +++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginAssignmentTrait.php
   @@ -53,13 +53,14 @@ protected function addContextAssignmentElement(ContextAwarePluginInterface $plug
   +        // avaialable, we have multiple and require user interaction.
   

   s/avaialable/available/

2. +++ b/core/modules/aggregator/config/schema/aggregator.schema.yml
   @@ -48,6 +48,7 @@ block.settings.aggregator_feed_block:
   +        #This will be stored in the format of aggregator.feed:$uuid
   

   s/#This/# This/

   Let's put single quotes around the example, and add a trailing period.

3. +++ b/core/modules/block/src/EventSubscriber/NodeRouteContext.php
   @@ -61,7 +61,7 @@ public function onBlockActiveContext(BlockContextEvent $event) {
   +    $context = new Context(new ContextDefinition('entity:node', $this->t('Node from url')));
   

   s/url/URL/

I'll work on 56.1 now, but I want to make sure this still passes tests and everything.

Testbot is broken ATM: #2467621: Tests not being added to the testbot queue… :( Almost my entire workday already…

The #77 interdiff looks awesome! Generically doing this FTW! Except… I wonder if we actually want \Drupal\Core\Plugin\ContextAwarePluginBase to do that already? ContextAwarePluginBase could implement CacheableDependencyInterface and then do that.

And then it actually could and should also do the analogous thing for ::getCacheMaxAge() + ::getCacheContexts(). Because the context that the block depends on may only be valid for a limited amount of time (max-age) or may vary depending on some context, e.g. negotiated language (contexts).

Issue summary changes: Added steps to test and put a link to the comment which shows the UI changes.

In manually testing this (following the steps in the issue summary), the aggregator block wouldn't show up until I cleared the cache. With all the caching changes lately (including enabling the page cache by default: #606840: Enable internal page cache by default), maybe this is the cause of the tests failures on the latest patch?

Also, the change in #76 that removed ucfirst() (per Wim's review in #75.2) causes the field label in the aggregator block to be "feed value" (with a lower-case first letter) which doesn't match Drupals normal capitalization standards:

Looking at the patch, it looks like it's setting the label for the context to "Aggregator feed":

+++ b/core/modules/aggregator/src/Plugin/Block/AggregatorFeedBlock.php
@@ -23,19 +22,16 @@
+ *   context = {
+ *     "feed" = @ContextDefinition("entity:aggregator_feed", label = @Translation("Aggregator feed"), description = @Translation("Select the feed that should be displayed"))
+ *   }

... so, I'd expect that to be what appears on the form!

Per the field label stuff from my last comment, is there a reason we don't switch to:

--- a/core/lib/Drupal/Core/Plugin/ContextAwarePluginAssignmentTrait.php
+++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginAssignmentTrait.php
@@ -53,7 +53,7 @@ protected function addContextAssignmentElement(ContextAwarePluginInterface $plug
         ];
         $element['context_slot_title'] = [
           '#type' => 'item',
-          '#title' => $this->t('@context value:', ['@context' => $context_slot]),
+          '#title' => $this->t('@context value:', ['@context' => $definition->getLabel()]),
           '#markup' => $options[$context_id],
         ];
       }
@@ -65,7 +65,7 @@ protected function addContextAssignmentElement(ContextAwarePluginInterface $plug
 
         $assignments = $plugin->getContextMapping();
         $element[$context_slot] = [
-          '#title' => $this->t('Select a @context value:', ['@context' => $context_slot]),
+          '#title' => $this->t('Select a @context value:', ['@context' => $definition->getLabel()]),
           '#type' => 'select',
           '#options' => $options,
           '#required' => $definition->isRequired(),

Or even just use the $definition->getLabel() as the title straight up, like this:

--- a/core/lib/Drupal/Core/Plugin/ContextAwarePluginAssignmentTrait.php
+++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginAssignmentTrait.php
@@ -53,7 +53,7 @@ protected function addContextAssignmentElement(ContextAwarePluginInterface $plug
         ];
         $element['context_slot_title'] = [
           '#type' => 'item',
-          '#title' => $this->t('@context value:', ['@context' => $context_slot]),
+          '#title' => $definition->getLabel(),
           '#markup' => $options[$context_id],
         ];
       }
@@ -65,7 +65,7 @@ protected function addContextAssignmentElement(ContextAwarePluginInterface $plug
 
         $assignments = $plugin->getContextMapping();
         $element[$context_slot] = [
-          '#title' => $this->t('Select a @context value:', ['@context' => $context_slot]),
+          '#title' => $definition->getLabel(),
           '#type' => 'select',
           '#options' => $options,
           '#required' => $definition->isRequired(),

In the plugin annotation, the label is inside @Translation(...) where as $context_slot is the machine name, so this should be better from a multilingual perspective too...

I wonder if we can just go with the @context as label itself, so in this case "Feed". It might seem rather direct, but at core that is what its about and how we also show many other labels in our UI. For example "Title" in this screen isn't "Block label value".

The latest patch in #86 fixes the issue I had with the 'Aggregator feed' block not appearing until I cleared the cache which I mentioned in #84.

Here's a very cut down version of the steps I followed:

1. Adding the aggregator feed source
2. Then adding the block to the sidebar
3. Visiting frong page: no block visible
4. Then forcing cron to run and verifying that the items were imported
5. Visiting front page: no block visible
6. Do drush cr then the block would appear

But with the patch from #86, the block will appear filled with data correctly at step 5!

(Note: this problem only happened with these specific steps - adding the aggregator feed then running cron before adding the block didn't have this problem...)

Anyway, it's fixed now!

Awesome, it's looking good to me!

I had another random question/comment about the field title:

+++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginAssignmentTrait.php
@@ -51,16 +51,28 @@ protected function addContextAssignmentElement(ContextAwarePluginInterface $plug
+          '#title' => $this->t('@context', ['@context' => $title]),

Is running this through $this->t() really necessary? My understanding of @Translation() in annotations is that the string gets run through t() when the annotation is evaluated:

The @Translation() wrapper serves this purpose. It will run the text through t() when the annotation is evaluated.

... from https://www.drupal.org/developing/api/8/localization

If this is just about sanitizing $context_slot in the case that it's used, we could use String::checkPlain($context_slot) when setting the $title variable, right?

Anyway, I don't do as much Drupal 8 development as you guys, so maybe the $this->t() is necessary or part of a coding style thing. If so, please just let me know and I'll stop nit-picking this miniscule bit of code. :-)

Thanks! This is RTBC as far as I'm concerned. :-) But I'll leave it to someone more "D8 qualified" to do review and actually update the status.

Would you consider moving the test coverage + associated bugfix in #89/#56.1 into a separate issue? We could get that committed easily, and it's a bug independent from this issue; it was merely uncovered in this issue. You get progress, we get a simpler patch to review.

An almost comprehensive review:

1. +++ b/core/lib/Drupal/Core/Block/BlockBase.php
   @@ -204,6 +208,9 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
   +    $contexts = $form_state->getTemporaryValue('gathered_contexts') ?: [];
   

   Where does this magical string come from? Where is it documented?

2. +++ b/core/lib/Drupal/Core/Block/BlockBase.php
   @@ -309,21 +316,39 @@ public function setTransliteration(TransliterationInterface $transliteration) {
      public function getCacheContexts() {
   ...
      public function getCacheTags() {
   ...
      public function getCacheMaxAge() {
   

   Correct me if I'm wrong, but these changes could also be moved into an issue of their own?

   Also: missing test coverage.

3. +++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginAssignmentTrait.php
   @@ -51,16 +51,26 @@ protected function addContextAssignmentElement(ContextAwarePluginInterface $plug
   +          '#title' => $definition->getLabel() ?: $context_slot,
   

   Why can the label be NULL?

4. +++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginBase.php
   @@ -62,6 +62,10 @@ public function getContextMapping() {
   +    // The ContextAwarePluginAssignmentTrait adds a visual element to the
   +    // context mapping when only one context is available. We should unset this
   +    // before saving the context mapping.
   

   But this base class doesn't use that trait, so shouldn't we do this conditionally?

   It looks like class_uses() is the only option PHP gives us here.

5. +++ b/core/modules/aggregator/config/schema/aggregator.schema.yml
   @@ -44,6 +44,11 @@ block.settings.aggregator_feed_block:
   -    feed:
   -      type: string
   -      label: 'Feed'
   +    context_mapping:
   +      type: mapping
   +      label: 'Context assignments'
   +      mapping:
   +        # This will be stored in the format of 'aggregator.feed:$uuid'.
   +        feed:
   +          type: string
   +          label: 'Feed'
   

   Is aggregator the only place that uses/needs this?

   The IS doesn't mention aggregator at all. Really, the IS is quite vague, having read it, I barely understand what this issue is about, and I definitely don't understand the consequences. I think it'd be *very* helpful if you could list the concrete consequences and benefits :)

6. +++ b/core/modules/aggregator/src/AggregatorServiceProvider.php
   @@ -0,0 +1,33 @@
   + * Provides a block context event subscriber if the block module is enabled.
   

   block context? I don't think the context is block-specific at all?

7. +++ b/core/modules/aggregator/src/EventSubscriber/AggregatorFeedContext.php
   @@ -0,0 +1,91 @@
   +        // The context mapping is stored as aggregator.feed:uuid, so we're just
   +        // extracting the UUID so that we can load the specific feed object.
   

   s/aggregator.feed:uuid/'aggregator.feed:UUID'/

   I think everything after the comma can be omitted. Just everything before the comma is sufficient to understand the next line.

8. +++ b/core/modules/aggregator/src/EventSubscriber/AggregatorFeedContext.php
   @@ -0,0 +1,91 @@
   +        $context = new Context(new ContextDefinition('entity:aggregator_feed'));
   +        $context->setContextValue($feed);
   +        $event->setContext('aggregator.feed:' . $feed->uuid(), $context);
   ...
   +      $context = new Context(new ContextDefinition('entity:aggregator_feed', $feed->label()));
   +      $context->setContextValue($feed);
   +      $event->setContext('aggregator.feed:' . $feed->uuid(), $context);
   

   These look very bizarre. But not a problem introduced by this patch.

   But that DX looks … painful. So much repetition, or what looks like repetition, it's slightly different every time. I fear this will lead to a lot of frustration.

9. +++ b/core/modules/aggregator/src/Plugin/Block/AggregatorFeedBlock.php
   @@ -23,19 +21,16 @@
   + *     "feed" = @ContextDefinition("entity:aggregator_feed", label = @Translation("Aggregator feed"), description = @Translation("Select the feed that should be displayed"))
   

   See #56.6.

10. +++ b/core/modules/aggregator/src/Plugin/Block/AggregatorFeedBlock.php
    @@ -58,16 +53,13 @@ class AggregatorFeedBlock extends BlockBase implements ContainerFactoryPluginInt
    -   * @param \Drupal\aggregator\FeedStorageInterface $feed_storage
    -   *   The entity storage for feeds.
    

    Nice! :)

11. +++ b/core/modules/aggregator/src/Plugin/Block/AggregatorFeedBlock.php
    @@ -145,51 +123,43 @@ public function blockSubmit($form, FormStateInterface $form_state) {
    -  public function getCacheTags() {
    -    $cache_tags = parent::getCacheTags();
    -    $feed = $this->feedStorage->load($this->configuration['feed']);
    -    return Cache::mergeTags($cache_tags, $feed->getCacheTags());
    

    This looks like a regression though. If the items used in ::build() would use entity rendering instead of custom rendering though, then this wouldn't be a problem.

    Not the fault of this patch, but unfortunately this patch shows again how crappy this block's rendering is. I'm happy to help with that.

    Oops, no, 2. deals with that :)

Here's the interdiff from #65 to #95.
I think we need more tests for some of these changes.

#98:

1. I think this should be documented somewhere.
2. BlockBase didn't previously return anything for those, because that didn't make sense. So there's no test coverage for those methods in particular. The existing test coverage lives at \Drupal\block\Tests\BlockViewBuilderTest. But for this, you'll want an integration test that has several dummy contexts, and that verifies that their cacheability metadata is merged as expected by these methods.
3. So perhaps we could change that?
4. It just doesn't make sense to execute some logic always that only makes sense sometimes. Understandability matters.
5. Ok, still waiting for the IS update.
6. Ok.
7. As I said, not a problem introduced by this patch. But IMO we should have at least a major issue to improve this DX.

You asked me in IRC what test coverage is needed:

1. +++ b/core/lib/Drupal/Core/Block/BlockBase.php
   @@ -204,6 +208,9 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
   +    // Add context form elements.
   +    $contexts = $form_state->getTemporaryValue('gathered_contexts') ?: [];
   +    $form['context_mapping'] = $this->addContextAssignmentElement($this, $contexts);
   
   +++ b/core/modules/aggregator/src/EventSubscriber/AggregatorFeedContext.php
   @@ -0,0 +1,91 @@
   +        $event->setContext('aggregator.feed:' . $feed->uuid(), $context);
   
   +++ b/core/modules/aggregator/src/Plugin/Block/AggregatorFeedBlock.php
   @@ -145,51 +123,43 @@ public function blockSubmit($form, FormStateInterface $form_state) {
   +    $feed = $this->getContextValue('feed');
   

   This looks like magical code, needs test coverage exerting several different scenarios.

2. +++ b/core/lib/Drupal/Core/Block/BlockBase.php
   @@ -309,21 +316,39 @@ public function setTransliteration(TransliterationInterface $transliteration) {
      public function getCacheContexts() {
   ...
      public function getCacheTags() {
   ...
      public function getCacheMaxAge() {
   

   The necessary test coverage for this is already explained above.

3. +++ b/core/modules/aggregator/src/EventSubscriber/AggregatorFeedContext.php
   @@ -0,0 +1,91 @@
   +class AggregatorFeedContext extends BlockContextSubscriberBase {
   

   Needs a unit test probably.

4. +++ b/core/tests/Drupal/Tests/Core/Annotation/ContextDefinitionTest.php
   @@ -0,0 +1,70 @@
   +class ContextDefinitionTest extends UnitTestCase {
   

   Hrm, weird, shouldn't this already have test coverage in HEAD?

   If it should, then please move this into a separate issue, so we can get that in already.

Here's a reroll, proudly presented by the delayed Swiss LX 327 flight to Zurich :p

No major problems during the reroll, updating the block list builder and block repository to use the new block repository was already done as part of applying the patch, so that is not visible in the interdiff.

The interdiff just contains changes to update the new aggregator feed context to the new API. Which is IMHO *so much* easier. No more messing with block entities and parsing their configuration. We get a list of UUID's to load, can query them, load the feeds and return them. Trivial :)

Didn't run any tests but manual testing worked fine.

Oh, and cacheability metadata seems to be bubbling up just as it should. Tested manually that the aggregator feed tag is added to the page.

1. +++ b/core/modules/aggregator/src/EventSubscriber/AggregatorFeedContext.php
   @@ -51,41 +42,37 @@ class AggregatorFeedContext extends BlockContextSubscriberBase {
   -      if ($block_plugin instanceof ContextAwarePluginInterface) {
   -        $contexts = $block_plugin->getContextMapping();
   -        // The context mapping is stored as aggregator.feed:uuid, so we're just
   -        // extracting the UUID so that we can load the specific feed object.
   -        list(, $uuid) = explode(':', $contexts['feed'], 2);
   -        $feeds = $this->feedStorage->loadByProperties(['uuid' => $uuid]);
   ...
   +  public function getRuntimeContexts(array $unqualified_context_ids) {
   +    $ids = $this->feedStorage->getQuery()
   +      ->condition('uuid', $unqualified_context_ids, 'IN')
   +      ->execute();
   +    $contexts = [];
   

   This is an _awesome_ win!

   This shows exactly why the other issue was critical, because else this would have had to load all things or have to implement its own complicated API as seen above.

   +1

2. +++ b/core/modules/aggregator/src/EventSubscriber/AggregatorFeedContext.php
   @@ -51,41 +42,37 @@ class AggregatorFeedContext extends BlockContextSubscriberBase {
   +  public function getAvailableContexts() {
        $feeds = $this->feedStorage->loadMultiple();
   +    $contexts = [];
        foreach ($feeds as $feed) {
          $context = new Context(new ContextDefinition('entity:aggregator_feed', $feed->label()));
          $context->setContextValue($feed);
   -      $event->setContext('aggregator.feed:' . $feed->uuid(), $context);
   +      $contexts[$feed->uuid()] = $context;
        }
   +    return $contexts;
      }
   

   Do we really need to load all the feeds for that to work?

   Isn't the ContextDefinition with label and uuid enough?

   Or does entity storage not allow to just get a list of defined properties?

   In any case we should not be setting the runtime context here, then we can at least rely on GC to cleanup the feed object, but I think this could explode quite quickly and we have seen things like that explode in D7 (with bean), so this is also a more generic question.

2. You can't get the label of an entity without loading it.

And the context system still works so that all possible context is loaded and presented so that you can select one.

That's not different from the current code, although that could now very easily use an entity selection widget to select a feed with a nice and scalable autocomplete textfield.

So, yes, I'm interested to get this functionality into core but I'm not convinced at all that aggregator feed blocks is a good example with the way context configuration works now.

The way I solved that with my static context patch in page_manager is to have a configuration form to select a specific entity using such an autocomplete widget and then just expose that as available context.

I understand that it is useful as an example to demonstrate this. But given that this is a normal task (given the current state in of block context in D8, it could probably also be classified as a major bug) I don't see how we can get this committed given that this comes with data changes.

So my recommendation revert those changes and just get the UI and other needed fixes in. And write some tests using a test block plugin.

That sounds good, indeed major bug.

I still think we are able to omit the:

$context->setContextValue($feed)

here and only rely on the definition and label.

Also, this conflicted already again with #2509600: AggregatorFeedBlock should return render array, keeping the changes in the build method minimal and not removing the if.

Accidently defined the service in block.services.yml, that wasn't the idea.

We somehow completely lost track of this.

What's still left here? AFAICT this part from #106:

So my recommendation revert those changes and just get the UI and other needed fixes in. And write some tests using a test block plugin.

re #117 and #106:

At the SCOTCH meeting today we discussed doing exactly that: splitting the aggregator feed block changes out into it's own issue, and having this issue just be about API and UI changes to make it possible. EclipseGC is going to work on it!

For what it's worth, the tests already in this patch seem pretty good! So, I don't think anything additional needs to be added, just the aggregator changes removed, and the issue summary updated.

Sounds great! Let's get this done.

Removed the aggregator stuff. Also fixed a bad reroll.

Noticed extra code in the BlockListBuilder.

Not posting *another* patch right now, these could even be fixed on commit:

1. +++ b/core/tests/Drupal/Tests/Core/Annotation/ContextDefinitionTest.php
   @@ -0,0 +1,70 @@
   +   * Test the ContextDefinition Annotation.
   

   Tests

2. +++ b/core/tests/Drupal/Tests/Core/Annotation/ContextDefinitionTest.php
   @@ -0,0 +1,70 @@
   +    //throw new \Exception(print_r(, TRUE));
   ...
   +    //throw new \Exception(print_r(, TRUE));
   

   Remove these :)

1. +++ b/core/lib/Drupal/Core/Annotation/ContextDefinition.php
   @@ -113,7 +113,7 @@ public function __construct(array $values) {
          // @todo Remove this workaround in https://www.drupal.org/node/2362727.
   -      if (isset($values[$key]) && $values[$key] instanceof TranslationWrapper) {
   +      if (isset($values[$key]) && $values[$key] instanceof Translation && $values[$key]->get() instanceof TranslationWrapper) {
            $values[$key] = (string) $values[$key]->get();
   

   might have been discussed before, but why this change exactly?

2. +++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginAssignmentTrait.php
   @@ -51,16 +51,26 @@ protected function addContextAssignmentElement(ContextAwarePluginInterface $plug
   +        $element['context_slot_title'] = [
   +          '#type' => 'item',
   +          '#title' => $definition->getLabel() ?: $context_slot,
   +          '#markup' => $options[$context_id],
   +        ];
   

   I know we already use that term in this context, but context_slot seems quite weird to me. But I guess it's just consistent to continue use it.

3. +++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginBase.php
   @@ -64,6 +64,10 @@ public function getContextMapping() {
   +    // The ContextAwarePluginAssignmentTrait adds a visual element to the
   +    // context mapping when only one context is available. We should unset this
   +    // before saving the context mapping.
   

   Yet another thing that doesn't properly consider optional contexts btw: #2484645: Assigning context mapping: allow empty selection for optional contexts.

4. +++ b/core/tests/Drupal/Tests/Core/Annotation/ContextDefinitionTest.php
   @@ -0,0 +1,70 @@
   +    // If the label or description are not a Translation annotation instance,
   +    // it should be replaced by NULL.
   ...
   +    // If the label or description are a Translation annotation instance, the
   +    // translated string should be returned.
   

   are a => is a?

#123.1: I asked the same in #56.1, then in #75.1, and in #76.1 @EclipseGc suggests it's to fix a bug, but he hasn't fixed the test coverage yet.

I created an issue which just has the aggregator changes from #115: #2550199: Add a ContextProvider for Feeds

The beta evaluation in #92 is very outdated. This issue does not touch caching at all anymore, that was done in #2375695: Condition plugins should provide cache contexts AND cacheability metadata needs to be exposed.

1. +++ b/core/lib/Drupal/Core/Annotation/ContextDefinition.php
   @@ -113,7 +113,7 @@ public function __construct(array $values) {
   -      if (isset($values[$key]) && $values[$key] instanceof TranslationWrapper) {
   +      if (isset($values[$key]) && $values[$key] instanceof Translation && $values[$key]->get() instanceof TranslationWrapper) {
   
   +++ b/core/tests/Drupal/Tests/Core/Annotation/ContextDefinitionTest.php
   @@ -0,0 +1,70 @@
   +    $values = [
   +      'value' => 'entity:node',
   +      'label' => new Translation(['value' => 'test']),
   +      'description' => new Translation(['value' => 'test']),
   +    ];
   +    $context_definition = new ContextDefinition($values);
   +    $definition = $context_definition->get();
   +    //throw new \Exception(print_r(, TRUE));
   +    // If the label or description are a Translation annotation instance, the
   +    // translated string should be returned.
   +    $this->assertEquals('test', $definition->getLabel());
   +    $this->assertEquals('test', $definition->getDescription());
   

   I guess this is the test coverage for #123.1.

   But I wonder if it's a test-only thing, the use of Translation versus TranslationWrapper?

2. +++ b/core/lib/Drupal/Core/Block/BlockBase.php
   @@ -199,6 +202,9 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
   +    // Add context form elements.
   

   This needs a much clearer description, for example:

   // Add context mapping UI form elements.
   

3. +++ b/core/lib/Drupal/Core/Block/BlockBase.php
   @@ -199,6 +202,9 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
   +    $form['context_mapping'] = $this->addContextAssignmentElement($this, $contexts);
   

   Don't we need test coverage for this new part of the UI?

4. +++ b/core/lib/Drupal/Core/Block/BlockBase.php
   --- a/core/lib/Drupal/Core/Plugin/ContextAwarePluginAssignmentTrait.php
   +++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginAssignmentTrait.php
   

   No test coverage for these UI changes either.

5. +++ b/core/modules/block/src/BlockForm.php
   @@ -334,7 +334,12 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
        // Call the plugin submit handler.
   -    $entity->getPlugin()->submitConfigurationForm($form, $settings);
   +    $block = $entity->getPlugin();
   +    $block->submitConfigurationForm($form, $settings);
   +    if ($block instanceof ContextAwarePluginInterface && $block->getContextDefinitions()) {
   +      $context_mapping = $settings->getValue('context_mapping', []);
   +      $block->setContextMapping($context_mapping);
   +    }
   

   The comment now seems to apply to the setContextMapping() call too. This is misleading.

   Let's add another comment just before the if-statement?

6. +++ b/core/modules/block/src/BlockForm.php
   @@ -345,7 +350,7 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   -      if ($condition instanceof ContextAwarePluginInterface) {
   +      if ($condition instanceof ContextAwarePluginInterface && $condition->getContextDefinitions()) {
            $context_mapping = isset($values['context_mapping']) ? $values['context_mapping'] : [];
            $condition->setContextMapping($context_mapping);
          }
   

   Why do we need to check if $condition->getContextDefinitions() contains anything, if the body of the if statement doesn't use that?

   It'd make more sense if we were checking if $values['context_mapping'] was not empty.

   So, what am I missing?

7. +++ b/core/modules/block/src/BlockRepository.php
   @@ -10,6 +10,8 @@
   @@ -32,6 +34,20 @@ class BlockRepository implements BlockRepositoryInterface {
   

   I only see additions here, no removals. together with the fact that we haven't changed ContextHandler at all, this makes me wonder how this works at all in HEAD?

8. +++ b/core/modules/block/src/Controller/BlockLibraryController.php
   index 5e8b54c..adb3930 100644
   --- a/core/modules/block/src/Tests/BlockInterfaceTest.php
   
   --- a/core/modules/block/src/Tests/BlockInterfaceTest.php
   +++ b/core/modules/block/src/Tests/BlockInterfaceTest.php
   
   +++ b/core/modules/block/src/Tests/BlockInterfaceTest.php
   +++ b/core/modules/block/src/Tests/BlockInterfaceTest.php
   @@ -99,6 +99,7 @@ public function testBlockInterface() {
   
   @@ -99,6 +99,7 @@ public function testBlockInterface() {
              '#options' => $period,
            ),
          ),
   +      'context_mapping' => array(),
          'display_message' => array(
            '#type' => 'textfield',
            '#title' => t('Display message'),
   

   So this is the only UI test change. Which means we don't test it.

   Shouldn't we have at least a cursory test?

9. +++ b/core/modules/block/src/Tests/BlockUiTest.php
   @@ -166,20 +166,41 @@ public function testCandidateBlockList() {
      public function testContextAwareBlocks() {
   

   For example, this could be expanded to use the context mapping UI, couldn't it?

   (Or, preferably, an additional context-aware block that doesn't use the CurrentUserContext.)

10. +++ b/core/modules/node/src/ContextProvider/NodeRouteContext.php
    @@ -63,7 +66,7 @@ public function getRuntimeContexts(array $unqualified_context_ids) {
    -    $context = new Context(new ContextDefinition('entity:node'));
    +    $context = new Context(new ContextDefinition('entity:node', $this->t('Node from URL')));
    

    This suggests missing test coverage?

Edit: ignore, patch includes cruft from git gone havoc. #133 it is.

Edit: ignore.

Edit: ignore.

Edit: ignore.

Edit: Ignore #128 - #132. Git played games with me, sorry for the spam.

So, I was trying to get this to work with #2561935: Create a block for every field (which'll take the entity as context) and the core block layout UI (not with page manager), and it threw errors about no context being present, even though the block placement edit UI showed the mapping correctly. I started to debug, and after an hour arrived at attached interdiff. Not sure if this is correct, but without, the context is lost, because only the block ID and not the already-assigned context is transferred when BlockViewBuilder creates a #lazy_builder.

Didn't address anything else, was for the moment just trying it out and debugging to get the context assignement to work at all. Interdiff does not include rebase on HEAD (no intervention needed but some moved hunks).

Setting to NR to get a test run.

Not sure what Wim's opinion is, but we talked a lot about context dependent plugins in the lazy builder issue:

#2543340-50: Convert BlockViewBuilder to use #lazy_builder (but don't yet let context-aware blocks be placeholdered) and following has the reasoning we came up with.

It seems context aware blocks do not work, because some Block* manager injects the contexts prior to displaying, which means indeed that - while the context is available in theory (we are in the same request context) - the lazy builder does not have the necessary information on where to get it.

Edit: It is _this_ issue which does that and hence it needs to work around that, core is fine.

Therefore I think opting out blocks with the one line fix of #133 is the right way to go.

Could we push that to another major bug issue though as follow-up to #2543340: Convert BlockViewBuilder to use #lazy_builder (but don't yet let context-aware blocks be placeholdered) - as it could easily get lost in here?

Thanks for working on that. Context is unfortunately not much supported in core and such bugs like these are hard to find and fix - if they don't make tests fail.

#133 + #137: See #2543340-41: Convert BlockViewBuilder to use #lazy_builder (but don't yet let context-aware blocks be placeholdered) and #2543340-48: Convert BlockViewBuilder to use #lazy_builder (but don't yet let context-aware blocks be placeholdered). Please read those comments in full.

In a nutshell:

• we can make blocks use #lazy_builder, but then we have to read the block plugin's annotation, look at the contexts it may consume, and pass those in the #lazy_builder's arguments
• BUT we can never ever prevent blocks from reading context they should not be reading: block plugins SHOULD only read the contexts they declare they depend on in their annotation, BUT the unfortunate design of allowing blocks to access ANY context means that it is impossible to guarantee no block is doing something it should not do.

Ignore #137, an extra issue is not needed, the problem is introduced in this code.

+++ b/core/modules/block/src/BlockRepository.php
@@ -58,6 +77,13 @@ public function getVisibleBlocksPerRegion(array &$cacheable_metadata = []) {
+      $block_plugin = $block->getPlugin();
+      if ($block_plugin instanceof ContextAwarePluginInterface) {
+        $contexts = $this->contextRepository->getRuntimeContexts(array_values($block_plugin->getContextMapping()));
+        $this->contextHandler->applyContextMapping($block_plugin, $contexts);
+      }

This is the problem.

You are not allowed to inject the contexts here, but will need to do so in the callback called by #lazy_builder.

Then lazy_builder continues to work and the opt-out is not needed.

Yes, this makes totally sense. Works as expected.

#138: Do we have to pass the context annotation to #lazy_builder? We load the plugin anyway later on, and the config is present then anyways I think (or not?).

Other feedback still not addressed, and this needs tests. I likely won't be able to work on this in the next week.

1. +++ b/core/modules/block/src/BlockRepository.php
   @@ -78,12 +78,6 @@ public function getVisibleBlocksPerRegion(array &$cacheable_metadata = []) {
   -      $block_plugin = $block->getPlugin();
   -      if ($block_plugin instanceof ContextAwarePluginInterface) {
   -        $contexts = $this->contextRepository->getRuntimeContexts(array_values($block_plugin->getContextMapping()));
   -        $this->contextHandler->applyContextMapping($block_plugin, $contexts);
   -      }
   -
          $access = $block->access('view', NULL, TRUE);
   

   This change should fail tests. If not, we are missing coverage.

   Note the last line of the code I highlighted. We check access here, and we ABSOLUTELY need the contexts applied before calculating that.

2. +++ b/core/modules/block/src/BlockViewBuilder.php
   @@ -141,6 +140,12 @@ protected static function buildPreRenderableBlock($entity, ModuleHandlerInterfac
   +      $contexts = \Drupal::service('context.repository')->getRuntimeContexts($plugin->getContextMapping());
   +      \Drupal::service('context.handler')->applyContextMapping($plugin, $contexts);
   

   Both of these services should be injected, unless this hunk is reverted.

#141.1: Right, I just realized this too. I misread the code in BlockAccessControlHandler::checkAccess() that injects contexts - it only injects contexts into each of the block's conditions, but not into the block plugin itself. So this hunk has to be reverted, yes.

It also makes me wonder though if we shouldn't just move the context injection right into block loading, because this seems really easy to get wrong: Whenever you load a block (to display it somewhere, I think block usage in D8 contrib will extend to other places than just the region assignment) you have to inject context, otherwise access cannot be checked - so why not inject context directly when loading the block, or within the access handler? Are their situations where this is not desired?

#141.2: It's a static method, I don't think we can use DI there

Hm, I was reviewing the interdiff, not the patch.

+++ b/core/modules/block/src/BlockRepository.php
@@ -58,6 +77,7 @@ public function getVisibleBlocksPerRegion(array &$cacheable_metadata = []) {
       /** @var \Drupal\block\BlockInterface $block */
+
       $access = $block->access('view', NULL, TRUE);

This is the actual change made in the patch...

Because in HEAD, \Drupal\block\BlockAccessControlHandler::checkAccess() has the getRuntimeContexts() and applyContextMapping() calls.

# 143: Isn't that just an empty line that sneaked in?

Otherwise I don't really get what you're refering to - yes, in HEAD (and unchanged in patch) contexts are injected in BlockAccessControlHandler::checkAccess(), and this is why, I think, we don't have to inject contexts in BlockRepository::getVisibleBlocksPerRegion(). That's what my last interdiff did, and what you seemed to imply in #141 would be a nogo.

#145: Your patch is correct. The interdiff was what was confusing.

Also it is confusing that there are conditions and contexts.

BUT:

conditions == access - work great on HEAD currently - unchanged here
context == variation - does not work in HEAD, is changed here

Tim thought you moved the conditions around, but you only moved the context, so it is all good :).

If you want to inject the things, than you have two possibilities:

- Load the block_view_builder via getting the entity view builder for block and calling a method there from the static method

OR

- create a service for building blocks

OR

- expose block view builder as a service (no idea how that would work).

Overall:

I don't think its possible to DI that properly, so I would make that a follow-up.

@Tim: Thoughts?

Straight reroll first. s/TranslationWrapper/TranslatableString

Working on tests for this.

#127

1) This mimics the use of @Translation in plugin annotations.
2) Done
3) Yes, see 9
4) Yes, see 9
5) Done
6) Investigating next, not addressed here.
7) Reverted, that seems to be cruft from earlier patches.
8) Yes, see 9
9) See 3/4/8... :) Yes, I added test coverage for the UI here. It revealed a schema issue, glad to catch that now.
10) I don't personally think this needs coverage, it just enhances the UX as per this change:

-          '#title' => $this->t('Select a @context value:', ['@context' => $context_slot]),
+          '#title' => $definition->getLabel() ?: $this->t('Select a @context value:', ['@context' => $context_slot]),

Okay I looked into #6 more.

The getContextDefinitions() check is a copy/pasted performance micro-optimization. In this method we update the context mapping of both the block and of each condition, yet in the second one we check the pre-munged values. This fixes that.

Someone added a beta evaluation since last tagged.

@tim.plunkett: Thanks for picking this up!

I did some manual testing with this and this CTools patch #2561935: Create a block for every field (which'll take the entity as context) (which adds a bunch of context-aware blocks) and everything worked great! (Although, I did find some things to improve in the CTools patch ;-))

Unfortunately, the code changes here are a little beyond me for the most part. :-/ But the tests look correct to me. So, from the outside I think this is doing all the things it should be doing.

I only noticed one nit-picky thing:

+++ b/core/tests/Drupal/Tests/Core/Annotation/ContextDefinitionTest.php
@@ -0,0 +1,70 @@
+      //'label' => new Translation(['value' => 'test']),
+      'label' => 'test',

Not sure why the translated label is commented out? If it works, we should definitely use the Translation object. If not, then we should put a comment as to why it doesn't.

A few more thoughts after further testing:

1. Required contexts are handled a little different than I would have expected. It makes the mapping required (ie. have to select a value on the form) but it'll still try to render a block which has a required context with no value. I would have expected it to skip rendering the block altogether. But I'm not sure if that comes from this patch, or is an existing thing in core.
2. If only one possible mapping is available, it's automatically selecting it and not giving the user an option. This makes sense for required contexts, but not for optional contexts. If the context is optional, you should have the choice to not map it, right?

This may be from me misunderstanding how contexts work in D8, though. :-)

re #153.2: It looks like @Berdir brought up the same thing in #123.3 and there's an issue to fix the code as it exists in core - #2484645: Assigning context mapping: allow empty selection for optional contexts. It sounds like @Berdir was saying that this patch (as of #123 - it might have changed) was making things even worse? I'm not sure if it makes sense to merge that fix in here because now core will be using it, or if it should stay a seperate issue...

Conflicted with #2458763: Remove the ability to configure a block's cache max-age.

Addressed #152.

Also removing as many unnecessary changes as possible.

16 files changed, 294 insertions(+), 13 deletions(-)

I revisited the @Translation change again, and it *is* out of scope. So is the other change to submitting conditions.

Fixing the @todo as well.

14 files changed, 222 insertions(+), 10 deletions(-)

Took a stab at updating the beta evaluation.

I agree that the translation part should be removed for now, since the test written doesn't appear to be right from the beginning.

If there is missing test coverage for translation, it probably can be taken care of in a future issue.

We are not changing UI, yay! Looks good overall, I hope we can find a solid integration point in 8.1

We have one weird thing remaining. When you have a context, but only one, this patch is adding a weird UI element.
It doesn't need to, it doesn't convey any helpful information, and it is out of scope here (I found it while trying to reduce changes).

Somewhere along the way, the original test for #2277981: Provide a service for handling context-aware plugins was broken.
Since the Block UI changed, the xpath needs to change too.

Regarding the changes in #163: this was something requested by Wim in #51. I know it's fewer changes, and not something we did in D7 (in the Panels world) but I kinda liked it. Since "Current user" is always available, when you select a block that shows data from the user, it's instructive to see "Current user" on the settings page so you know where this data is coming from.

In any case, I'd be fine with either including on not including that feature - getting this committed is most important! - just to give it's history and say that I liked it. :-)

Assuming this passes tests, this should be ready to go! Marking as RTBC

See also #2484645: Assigning context mapping: allow empty selection for optional contexts, which is somewhat related, I think it would be a easier to just always make the select visible. I think it does have useful information as you might not actually know what kind of context the block uses and what is used. And as shown in the issue above, it is currently impossible to *not* select a context for an optional context if at least one is available that would match.

I think that will likely be the solution in that other issue. But the logic around the select is not changed in this patch.

It needs further usability review and discussion over there.

This looks great to me now. Fewer, more focused changes. A clear scope really helps. #165++ and #169++ regarding my remark in #51: we can do that in the other issue, it does indeed merit usability review.

Reviewed the screenshots in #48 with @tim.plunkett, @EclipseGc, @japerry, @mikepotter, et al at DrupalCon Barcelona.

The one thing I had questions around was the title/description of the selector, but this is set per-context definition, which means they can decide what makes the most sense for their use case. +1.

The code has been reviewed by many awesome people, this is a contrib project blocker, let's get it done.

Committed and pushed to 8.0.x. Thanks! :D

Huzzah! Thanks everyone! :-)

Unpostponed #2550199: Add a ContextProvider for Feeds.