ContextHandler does not expect ContextDefinitionInterface

+++ b/core/lib/Drupal/Component/Plugin/Context/ContextInterface.php
@@ -42,7 +42,7 @@ public function setContextDefinition(ContextDefinitionInterface $context_definit
+   * @return \Drupal\Core\Plugin\Context\ContextDefinitionInterface

This needs to reference the Component version since it's a Component file doing the referencing. Otherwise this patch looks pretty spot on.

Awww... I did point this out in the other issue but then forgot about it myself and didn't find time to review it again.

Unit test fun:

You were setting up a single context definition for the available context in all cases, that one had an internal $this->typedDataManager, so the first test did get it from the container but then it was set, and the second test got the old mock object, which had been reset by phpunit and didn't have any matchers anymore.

Not sure why the expect() on $context itself works...?

There are a bunch of ways to fix this:

a) Quickfix: drop $this->typedDataManager = \Drupal::typedDataManager() in ContextDefinition, return it directly.

b) Set up the context and definition inside the test method, right now we only vary between "having the context" and not having it, so that's what the current patch changes it to. This is IMHO cleaner, because it doesn't re-use mock objects between test runs.. that's scary and as written above, not sure why the $context->expects() itself is not failing too. But, the current patch is less flexible, as we can't test something like multiple available contexts or different types right now, would have to be done slightly different. maybe pass an array of context definitions and the provider method needs to ensure to not re-use them across tests?

c) the context definition patch introduced TypedDataTrait to support unit testing, so instead of relying on the container, we should actually inject the typed data manager explicitly into those objects I think. That would just work if we'd do that on the first line of the test method, but I'm unsure about keeping the approach with re-using a mock object either way, so maybe this should be done in combination with b)?

+++ b/core/lib/Drupal/Core/Plugin/Context/ContextHandler.php
@@ -51,19 +50,20 @@ public function filterPluginDefinitionsByContexts(array $contexts, array $defini
-        $definition = $this->typedDataManager->getDefinition($plugin_context['type']);
-        $definition['type'] = $plugin_context['type'];
+        $definition = $this->typedDataManager->getDefinition($plugin_context->getDataType());
+        $definition['type'] = $plugin_context->getDataType();
 

This seems a bit weird and some kind of fallback if the type doesn't exist? That's not really valid, is it? as internally, we now throw an exception.

Also, this should now be able to use $plugin_context->getDataDefinition(), no?

Would save you the whole constraint stuff below and possibly more?

Do we even still need to convert to data definitions to the comparisons or can we compare context definitions directly?

Should we name this $plugin_context_definition now?

Manually tested this and am happy with it as well. RTBC stands in my opinion.

Eclipse

Committed and pushed to 8.x. Thanks for the fast turnaround on this!

Is this a correct format for a code comment?

       $requirements = array();
+      /** @var $plugin_context \Drupal\Core\Plugin\Context\ContextDefinitionInterface */
       foreach ($plugin_definition['context'] as $context_id => $plugin_context) {

Yes, it tells IDEs that $plugin_context in the foreach statement is a ContextDefinitionInterface so that you can get type hinting against that class's methods.

Eclipse