I'm working through the Kernel tests to see why they're broken after #2508884: Make contexts immutable landed.

There were 15 failing tests initially. 10 of those relate to the use of setContextValue() in ContextHandlerTrait. The next 4 seem to be a problem with null handling in RuleState::applyDataSelector(), since Context::getContextValue() returns NULL rather than an instance of TypedDataInterface, causing a crash when it's treated like an object. The last is also related the NULL data case, since Context::getContextValue() as now written treats a NULL'ed context that's supplied as not there for purposes of isRequired().

I have test fixes for the first two issues so far. They run GREEN, but I'm not sure if they're actually correct.

CommentFileSizeAuthor
#3 workarounds-pass-1.patch3.07 KBTorenware
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Torenware created an issue. See original summary.

Torenware’s picture

Issue summary: View changes
Related issues: +#2508884: Make contexts immutable
Torenware’s picture

FileSize
3.07 KB

I have plausible solutions for the first two items, as I've said before.

Getting a context to wrap a NULL value, however, appears to be hard to do. I've tried to create a sort of "typed null" object (an 'any', setting the value to NULL), but Context's implementation of getContextValue() will throw an except:

  /**
   * {@inheritdoc}
   */
  public function getContextValue() {
    if (!isset($this->contextData)) {
      $definition = $this->getContextDefinition();
      $default_value = $definition->getDefaultValue();

      if (isset($default_value)) {
        // Keep the default value here so that subsequent calls don't have to
        // look it up again.
        $this->setContextValue($default_value);
      }
      elseif ($definition->isRequired()) {
        $type = $definition->getDataType();
        throw new ContextException("The '$type' context is required and not present.");
      }
      return $default_value;
    }
    return $this->getTypedDataManager()->getCanonicalRepresentation($this->contextData);
  }

I don't see a good way around this. Worse: I'm seeing some weird behavior even when it doesn't throw.

I'm going to post patches for my approach to the other two issues, since I don't think it's to the point I can create a PR that's worth posting.

The patch shows changes to ContextHandlerTrait and RulesState that resolve 14 out of the 15 issues in the Kernel tests. Again, not sure that the behavior is what you want, but at least the tests like them.

emclaughlin’s picture

I'm running into a problem that may be related to this? I'm working on replacing all the evaluate calls with doEvaluate in conditions, and I'm having an issue with optional context arguments. Specifically: they're coming through as defined as NULL, which means the default doesn't get applied.

I haven't been able to track down why it's doing that, so I'm not 100% sure that it's related to this, but I figured I'd comment just in case.

Torenware’s picture

@emclaughlin: take a look at #2573823: Context in Rules plugins does not respect annotation order. Some of the problems I've seen related to doExecute() getting its arguments in the wrong order, which can happen with default arguments right now.

Is this relevant to your problem?

emclaughlin’s picture

@Torenware Hmmmm, I'm not sure. It looks like all of the other params are being provided in the expected order. It's just, when you don't specify a value for an optional param, the value ends up being NULL in the doEvaluate instead of what the default value is defined as.

emclaughlin’s picture

@Torenware Never mind! It's an issue with the execute()/evaluate() functions in RulesActionBase and RulesConditionBase respectively. I'm creating another issue. Thanks!

Torenware’s picture

Status: Active » Closed (fixed)