Problem/Motivation

Context::getContextData() must always return a typed data object, but sometimes it returns NULL.

Proposed resolution

Always produce and return a typed data object.

Remaining tasks

Patch + tests.

User interface changes

none.

API changes

none.

Data model changes

none.

RC inclusion evaluation

No code in Drupal core relies on the current behavior that Context::getContextData() sometimes returns NULL. Page manager at https://www.drupal.org/project/page_manager never invokes Context::getContextData(), so that is also not affected. Rules at https://www.drupal.org/project/rules is affected by the current behavior and makes heavy use Context::getContextData(). No other contrib projects are known to use Context::getContextData() and the Context system in general, so no negative impact is expected by fixing this false behavior.

CommentFileSizeAuthor
#10 context-typed-data-2583135-5.patch3.33 KBTorenware
#8 context-typed-data-2583135-4.patch3.33 KBTorenware
#3 context-typed-data-2583135-3.patch3.26 KBTorenware
#2 context-typed-data-2583135-2.patch981 bytesklausi
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

klausi created an issue. See original summary.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Active » Needs review
Issue tags: +d8rules, +Contributed project blocker, +Needs tests
FileSize
context-typed-data-2583135-2.patch981 bytes

Patch.

Torenware’s picture

Torenware CreditAttribution: Torenware as a volunteer commented
FileSize
context-typed-data-2583135-3.patch3.26 KB

I've rerolled to add a test to @klausi's patch. The test runs red w/o klausi's fix, green with it.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

Isn't there code that is relying on this being NULL? I thought so..

Torenware’s picture

Torenware CreditAttribution: Torenware as a volunteer commented

@Berdir -- not to the best of our knowledge. Not sure what actual good that would do, though.

Part of the point of klausi's fix is that even if data comes back null, you still want to be able to query it. Rules definitely relies on that. And more to the point: the interface itself promises to return typed data. This was causing errors in Core's Context classes that brought down Rules.

Klausi can explain this in more detail -- I'm not immersed in the TypedData code the way Klausi and fago are -- but I'd doubt that there is anything in Contrib that relies on Core crashing it ;-) Which is how I became aware of #2508884: Make contexts immutable. TypedData objects can indeed wrap NULL, and w/o the patch in this issue, the code in Context:: getContextData() will misbehave if an object like that gets passed to it.

In any events, the tests have already passed the first of the two patches, indicating that if anything relies on #2508884: Make contexts immutable's behavior, it's not captured in the tests. This should not surprise you: the change in behavior has only been in place for 12 or so days now. It would be surprising if anybody has had time to rely on that.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs review » Needs work 
+++ b/core/tests/Drupal/Tests/Core/Plugin/Context/ContextTest.php
@@ -127,8 +138,9 @@ public function testSetContextValueCacheableDependency() {
+   * @param misc $default_value

Missing newline before, "misc" should be "mixed" and a parameter description is missing. Something like "The default value that should be used."

Otherwise looks good to me, thanks rtoren!

@Berdir: For the getContextValue() method some code might rely on the fact that the return value can also be NULL. But for getContextData() it is clear that we always want a typed data object that wraps any value (NULL, entities, strings etc.).

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

Yeah, I believe we had at some point no way of checking if there is a context value or not because getContextValue() still throws an exception if there is none AFAIK and getContextData() was used as a workaround. We probably have better ways of doing this now.

Torenware’s picture

Torenware CreditAttribution: Torenware as a volunteer commented
Status: Needs work » Needs review
FileSize
context-typed-data-2583135-4.patch3.33 KB

Fixed per @klausi and rerolled.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs review » Needs work
Issue tags: -Needs tests +rc eligible 
  1. +++ b/core/tests/Drupal/Tests/Core/Plugin/Context/ContextTest.php
@@ -127,8 +138,11 @@ public function testSetContextValueCacheableDependency() {
+   * ¶

    trailing white space 

  2. +++ b/core/tests/Drupal/Tests/Core/Plugin/Context/ContextTest.php
@@ -127,8 +138,11 @@ public function testSetContextValueCacheableDependency() {
+   *   default value to assign to the mock context definition.

    Should start capitalized like "The default value ..."

Sorry for nitpicking this to death, otherwise would be RTBC to me.

Since this is a contrib blocker this is absolutely eligible for RC2.

Torenware’s picture

Torenware CreditAttribution: Torenware as a volunteer commented
FileSize
context-typed-data-2583135-5.patch3.33 KB

@klausi -- I couldn't find the trailing whitespace you were referring to, but I've fixed the capitalization issue.

I did a scan with phpcs, but I didn't want to disturb any code that was already in the file before I touched it.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs work » Reviewed & tested by the community

I found the trailing whitespace with dreditor, it is gone now.

Looks good now, thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work
Issue tags: -rc eligible +Needs work

This is not rc eligible. If you want committers to consider it for inclusion in RC then the issue summary needs to include a statement of why we need to make this change and be tagged with rc target triage. It is possible that contrib or custom is already relying on the behaviour in HEAD.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs work +rc target triage

Sure.

tim.plunkett’s picture

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

We aren't depending on this functionality now, and nothing will break. I double-checked by running the CTools and Page Manager test suites locally with this patch applied to core, and everything passed.

RTBC +1

EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc at Acquia commented

So, PageManager & Rules are the two systems in contrib that are most likely to really put this system through its paces. It was built with them in mind, so if the implementation actually doesn't match the interface's expectations, that does seem problematic.

In practice, I'm unconvinced by the other arguments, but this shouldn't hurt anything, and if anyone needs this behavior in the future TypedDataInterface has methods on it that can be used in leu of this odd functionality.

In general ++ to the change.

Eclipse

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: -rc target triage +rc target

Discussed with the D8 committers and we agreed with making this an rc target.

alexpott’s picture

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

Committed 3a8cb0d and pushed to 8.0.x. Thanks!

  • alexpott committed 3a8cb0d on 8.0.x 
    Issue #2583135 by Torenware, klausi: Context::getContextData() sometimes...
Torenware’s picture

Torenware CreditAttribution: Torenware as a volunteer commented

You guys rock! Thanks.

Status: Fixed » Closed (fixed)

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