Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
#2281635: \Drupal\Core\Plugin\Context\Context should use a ContextDefinition class instead of arrays introduced ContextDefinition, and updated all but on plugin to use it.
Unfortunately, that was the only plugin that was run through ContextHandler, so we didn't catch that ContextHandler is completely broken.
Proposed resolution
Fix ContextHandler to expect ContextDefinition, and update the test block plugin.
Remaining tasks
Fix remaining unit test failures
User interface changes
N/A
API changes
N/A
Comment | File | Size | Author |
---|---|---|---|
#11 | interdiff.txt | 743 bytes | tim.plunkett |
#11 | context-2292889-11.patch | 21.32 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettComment #3
EclipseGc CreditAttribution: EclipseGc commentedThis needs to reference the Component version since it's a Component file doing the referencing. Otherwise this patch looks pretty spot on.
Comment #5
tim.plunkettFixed the docs bit. But I cannot figure out why these unit tests are failing :(
Comment #7
BerdirAwww... 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)?
Comment #8
BerdirThis 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?
Comment #9
tim.plunkettUghhhhhhh thanks berdir.
I think #8 is a whole other can of worms, and not nearly as critical as this, I've opened #2293205: Consider removing usage of DataDefinition in ContextHandler to handle that.
Comment #10
tim.plunkettThis isn't ready.
Comment #11
tim.plunkettOkay, now it is (this is code @Berdir suggested, I just typed it out).
Comment #12
EclipseGc CreditAttribution: EclipseGc commentedManually tested this and am happy with it as well. RTBC stands in my opinion.
Eclipse
Comment #13
webchickCommitted and pushed to 8.x. Thanks for the fast turnaround on this!
Comment #15
oriol_e9gIs this a correct format for a code comment?
Comment #16
EclipseGc CreditAttribution: EclipseGc commentedYes, 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