A context object for a specific entity type will not match a generic requirement for any entity

First attempt at a fix. It passes tests locally, but it sorta breaks the encapsulation of EntityContextDefinition. Sigh...this is a logical pickle.

Refactored the logic a bit at @tim.plunkett's suggestion so that we can support fuzzy matching without hard-coding the 'entity' string into ContextDefinition.

This is a huge win and clearly a big improvement over the current logic.

RTBC!

Eclipse

1. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextDefinition.php
   @@ -312,6 +312,29 @@ public function getDataDefinition() {
   +      $this_type === 'any' ||
   

   As a non-expert of the Context system: why is it okay for $this_type === 'any' but not $that_type === 'any'?

   I think it's because $this is a context definition and $that is a concrete context?

2. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextDefinition.php
   @@ -319,7 +342,7 @@ public function isSatisfiedBy(ContextInterface $context) {
        // If the data types do not match, this context is invalid unless the
        // expected data type is any, which means all data types are supported.
   

   This comment now belongs on the new helper method.

As a non-expert of the Context system: why is it okay for $this_type === 'any' but not $that_type === 'any'?

I'm not entirely sure, but I think your appraisal of it makes sense. In any event, we're just preserving the logic that was already there.

This comment now belongs on the new helper method.

+1 for this.

#16: cool, done.

Moving the comment pretty much answers my own question :)

+++ b/core/lib/Drupal/Core/Plugin/Context/ContextDefinition.php
@@ -312,6 +312,29 @@ public function getDataDefinition() {
+   * Checks if this definition's data type matches that of another context.

Nit: "another" implies $this is a context too, which it isn't. Clarified.

The patch look great; the test coverage also looks complete and the change makes sense. The only thing I pondered was whether we need to add another protected method here.

The only question I have is whether or not ContextDefinition often extended in contrib / custom? The reason I ask is because adding a new protected method is not strictly necessary but I do concede it makes the code easy to understand. Inlining the code will definitely lose the nice clarity of $this_type vs $that_type.

Can't speak for custom code, but Russian Contrib Grep says it has been extended, albeit not often: http://grep.xnddx.ru/search?text=extends+ContextDefinition&filename=

So this will certainly allow subclasses some greater flexibility when it comes to matching other contexts.

Chose not to backport for the time being due to the minuscule risk of disruption due to the new method. If people feel otherwise then I might be convinced otherwise :)

Committed 9601416 and pushed to 8.7.x. Thanks!

So whether to backport this is a release management decision (the risk of the disruption vs. the benefit to contrib).

After discussing this with @tim.plunkett, I don't think we should backport these. The benefit to the affected conrib project is marginal -- it also still needs to maintain a patch for the chain of issues for #2976148: Layout-based entity rendering should delegate to the correct section storage instead of hardcoding to either defaults or overrides, which are quite disruptive and definitely not backportable.

So let's keep these in 8.7.x only unless there's something I've overlooked. Thanks!