Problem/Motivation

ContextHandler needlessly recreates context objects when applying contexts to a plugin.
This is wasteful but also can cause side-effects:
Given an EntityContext object, it will recreate a plain Context object.

Proposed resolution

Use setContext instead of setContextValue in ContextHandler::applyContextMapping

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#16 3016458-context-16-interdiff.txt843 bytestim.plunkett
#16 3016458-context-16.patch7.87 KBtim.plunkett
#12 3016458-context-12-PASS.patch7.86 KBtim.plunkett
#12 3016458-context-12-FAIL.patch1.67 KBtim.plunkett
#3 3016458-context-3-PASS.patch6.18 KBtim.plunkett
#3 3016458-context-3-FAIL.patch5.38 KBtim.plunkett
#2 3016458-context-2-PASS.patch103.32 KBtim.plunkett
#2 3016458-context-2-FAIL.patch823 bytestim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Active » Needs review
Related issues: +#2976148: Layout-based entity rendering should delegate to the correct section storage instead of hardcoding to either defaults or overrides
FileSize
3016458-context-2-FAIL.patch823 bytes
3016458-context-2-PASS.patch103.32 KB

Blocks #2976148: Layout-based entity rendering should delegate to the correct section storage instead of hardcoding to either defaults or overrides

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
FileSize
3016458-context-3-FAIL.patch5.38 KB
3016458-context-3-PASS.patch6.18 KB

Whoops, included that other issue.

The last submitted patch, 2: 3016458-context-2-FAIL.patch, failed testing. View results

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented

This code was in the original version of ContextHandler #2277981: Provide a service for handling context-aware plugins, committed 2014-06-10.
Because at that time, setContext() hadn't been written. That wasn't until #2281635: \Drupal\Core\Plugin\Context\Context should use a ContextDefinition class instead of arrays, committed 2014-06-25.

The last submitted patch, 3: 3016458-context-3-FAIL.patch, failed testing. View results

EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc at Acquia commented
Status: Needs review » Reviewed & tested by the community

This change seems very sensible to me.

Eclipse

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented

Also I should note that if we were to have fixed this issue before fixing #3008431: A context object for a specific entity type will not match a generic requirement for any entity, we would have been exposing that bug in core.
But now since that issue landed, we're safe here.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Thanks @tim.plunkett for documenting the history of the bug and the relationship to #3008431: A context object for a specific entity type will not match a generic requirement for any entity. That gives me confidence that this is a good change and that any side effects from this fix are mitigated in 8.7.x.

One thing -- the final fail patch says "Build Successful". I'm going to queue another run to see if we can get a proper fail

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Related issues: +#3008431: A context object for a specific entity type will not match a generic requirement for any entity

Via @tim.plunkett: CI is obscuring the fail, but it can be seen on Dispatcher:
https://dispatcher.drupalci.org/job/drupal_patches/78334/testReport/juni...

fail: [Other] Line 0 of sites/default/files/simpletest/phpunit-444.xml:
PHPunit Test failed to complete; Error: PHPUnit 6.5.13 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\Core\Plugin\ContextHandlerTest
.......................F....F.                                    30 / 30 (100%)

Time: 345 ms, Memory: 8.00MB

There were 2 failures:

1) Drupal\Tests\Core\Plugin\ContextHandlerTest::testApplyContextMapping
Expectation failed for method name is equal to "setContext" when invoked 1 time(s).
Method was expected to be called 1 times, actually called 0 times.

2) Drupal\Tests\Core\Plugin\ContextHandlerTest::testApplyContextMappingConfigurableAssigned
Expectation failed for method name is equal to "setContext" when invoked 1 time(s).
Method was expected to be called 1 times, actually called 0 times.

FAILURES!
Tests: 30, Assertions: 59, Failures: 2.

So we don't actually need a retest.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Let's add a test verifying that we get the EntityContext object when appropriate as described in the IS (and add that test to the fail-patch).

Thanks!

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3016458-context-12-FAIL.patch1.67 KB
3016458-context-12-PASS.patch7.86 KB

Better to be explicit in our testing.
FAIL patch is the interdiff.

The last submitted patch, 12: 3016458-context-12-FAIL.patch, failed testing. View results

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Reviewed & tested by the community

Looks great.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: 3016458-context-12-PASS.patch, failed testing. View results

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Needs work » Reviewed & tested by the community
FileSize
3016458-context-16.patch7.87 KB
3016458-context-16-interdiff.txt843 bytes

Rerolled for #3014949: Deprecate 'context' on Block and Condition plugin annotations in favor of 'context_definitions'

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed b33232b7d2 to 8.7.x and 2b93e80229 to 8.6.x. Thanks!

  • alexpott committed b33232b on 8.7.x 
    Issue #3016458 by tim.plunkett, xjm: ContextHandler should use...

  • alexpott committed 2b93e80 on 8.6.x 
    Issue #3016458 by tim.plunkett, xjm: ContextHandler should use...

  • xjm committed 6a51921 on 8.6.x 
    Revert "Issue #3016458 by tim.plunkett, xjm: ContextHandler should use...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Fixed » Needs work

This seems likely to have introduced:
https://www.drupal.org/pift-ci-job/1134931

It's possible this is this is just due to inattentive backporting given the relationship to #3008431: A context object for a specific entity type will not match a generic requirement for any entity documented above. We'll know in an hour or so. Meanwhile, I've reverted it from 8.6.x only.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

Yep my mistake. Thanks for the revert. As long as 8.7.x passes I suggest we close this unless the authors want to port a fix to 8.6.x

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs work » Fixed

Yep so the builds are fine. Setting to fixed as per #22. If we want this in 8.6.x then we'll need to port the patch.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Version: 8.7.x-dev » 8.6.x-dev
Status: Closed (fixed) » Postponed
Issue tags: +Contributed project blocker

I've asked for #3008431: A context object for a specific entity type will not match a generic requirement for any entity to be backported, and would like the same for this once that's in.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Version: 8.6.x-dev » 8.7.x-dev
Status: Postponed » Fixed

As per #3008431: A context object for a specific entity type will not match a generic requirement for any entity, I think this should remain in 8.6.x for now, since there isn't actually direct benefit to the affected project from backporting them alone. The affected project should depend on the next minor.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.