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
EntityContextDefinition was introduced to allow easier DX when dealing with a specific entity or entity type.
However, it will not match a generic requirement for any entity.
Proposed resolution
Expand the data type checking to allow variants (aka derivatives) to satisfy a more generic context.
Remaining tasks
N/A
User interface changes
N/A
API changes
N/A
Data model changes
N/A
Comment | File | Size | Author |
---|---|---|---|
#17 | 3008431-context-17.patch | 8.01 KB | Wim Leers |
#17 | interdiff.txt | 1.51 KB | Wim Leers |
#9 | 3008431-context-9-interdiff.txt | 6.12 KB | tim.plunkett |
#9 | 3008431-context-9-PASS.patch | 8.03 KB | tim.plunkett |
#9 | 3008431-context-9-FAIL.patch | 5.7 KB | tim.plunkett |
Comments
Comment #2
tim.plunkettHere's a failing test
Comment #3
tim.plunkettComment #5
phenaproximaFirst attempt at a fix. It passes tests locally, but it sorta breaks the encapsulation of EntityContextDefinition. Sigh...this is a logical pickle.
Comment #6
phenaproximaRefactored the logic a bit at @tim.plunkett's suggestion so that we can support fuzzy matching without hard-coding the 'entity' string into ContextDefinition.
Comment #7
tim.plunkettThis is beautiful :chef kisses fingers:
Comment #8
tim.plunkettActually, unRTBCing for now as the test coverage is in the wrong place and the IS needs updating.
Comment #9
tim.plunkettThe last patch was actually a step too far, because if the requirement is for
entity:node
, then a user entity should not satisfy that. Only when the requirement is for the genericentity
should the entity type not matter.Fixed the logic, expanded the entity tests to include that reverse case, and wrote generic tests for a non-entity case.
Comment #11
tim.plunkettCan't reRTBC anymore, oh well :)
Comment #12
tim.plunkettThis is a blocker
Comment #13
andypostComment #14
EclipseGc CreditAttribution: EclipseGc at Acquia commentedThis is a huge win and clearly a big improvement over the current logic.
RTBC!
Eclipse
Comment #15
Wim LeersAs 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?This comment now belongs on the new helper method.
Comment #16
phenaproximaI'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.
+1 for this.
Comment #17
Wim Leers#16: cool, done.
Moving the comment pretty much answers my own question :)
Nit: "another" implies
$this
is a context too, which it isn't. Clarified.Comment #18
tim.plunkettThanks @Wim Leers for those clarifying changes. RTBC +1
Comment #19
alexpottThe 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
.Comment #20
phenaproximaCan'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.
Comment #21
tim.plunkettI much prefer the readability of the method. I could also see it being useful for subclasses, though they may be rare.
Comment #22
alexpottChose 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!
Comment #25
tim.plunkettUpon reflection, this would be very nice to have backported. It will enable Lightning to backport parts of 8.7 Layout Builder.
Comment #27
tim.plunkettRetested the patch from #17 on 8.6
Comment #28
xjmSo 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!
Comment #29
xjm