Problem/Motivation

The #2671964: ContextHandler cannot validate constraints introduced a new method called isSatisfiedBy(ContextInterface $context) which checks if given context satisfies context deifinition.

There is a following condition in this method:

    elseif ($definition instanceof static) {
      $values = $definition->getSampleValues();
    }

This condition fails if the ContextDefinition object is an instance of inherited class in which case it becomes:

    elseif ($definition instanceof SomeClassExtendingContextDefinition) {
      $values = $definition->getSampleValues();
    }

In a real life, the Rules module replaces context definition plugins with it's own, extending core.

Proposed resolution

The condition above should be checking 'instanceof self' instead of 'instanceof static'.

Remaining tasks

Review.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#3 core-is_satisfied_extended_context_test_and_fix-2958785-3.patch2.23 KBabramm
#2 core-is_satisfied_extended_context_test-2958785-2.patch1.63 KBabramm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

abramm created an issue. See original summary.

abramm’s picture

abramm
Primary language Ukrainian
Location Lutsk
CreditAttribution: abramm at DevBranch for Creative Propulsion Labs commented
FileSize
core-is_satisfied_extended_context_test-2958785-2.patch1.63 KB

The test in this patch is expected to fail.

abramm’s picture

abramm
Primary language Ukrainian
Location Lutsk
CreditAttribution: abramm at DevBranch for Creative Propulsion Labs commented
FileSize
core-is_satisfied_extended_context_test_and_fix-2958785-3.patch2.23 KB

Patch with test + fix.

abramm’s picture

abramm
Primary language Ukrainian
Location Lutsk
CreditAttribution: abramm at DevBranch for Creative Propulsion Labs commented
Assigned: abramm » Unassigned
Status: Needs work » Needs review
abramm’s picture

abramm
Primary language Ukrainian
Location Lutsk
CreditAttribution: abramm at DevBranch for Creative Propulsion Labs commented
Issue summary: View changes
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Version: 8.5.x-dev » 8.6.x-dev
Status: Needs review » Reviewed & tested by the community

Wow, nice find. I'm so used to thinking of it from the other direction when it comes to self vs static, this probably didn't even occur to me.

Test coverage is good, thanks for that.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Priority: Normal » Major
Issue tags: +Contributed project blocker
hd’s picture

hd CreditAttribution: hd commented

In my core 8.5.1 system in core/lib/Drupal/Core/Plugin/Context/ContextDefinition.php in function isSatisfiedBy()  I changed 

// elseif ($definition instanceof static) {
elseif ($definition instanceof self) {

and the two other changes as per patch https://www.drupal.org/files/issues/2018-04-05/core-is_satisfied_extende... and can confirm this fixes the problem. Now block visibility setting tab for Content types is showing when the Rules module is installed, and it also works as intended. Originally I reported the issue at https://www.drupal.org/project/rules/issues/2956908. Thanks everyone for getting to the bottom of this.

Madis’s picture

Madis CreditAttribution: Madis as a volunteer commented

Closed issues #2955370 and #2956908 as duplicates and as mentioned in the oldest one I can also confirm that this patch fixes block visibility settings with the Rules module installed (tested with latest core and Rules dev versions).

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed e817928144 to 8.6.x and d77ed3e3fc to 8.5.x. Thanks!

  • alexpott committed e817928 on 8.6.x 
    Issue #2958785 by abramm: ContextDefinition isSatisfiedBy() check fails...

  • alexpott committed d77ed3e on 8.5.x 
    Issue #2958785 by abramm: ContextDefinition isSatisfiedBy() check fails...

Status: Fixed » Closed (fixed)

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