In writing unit tests for various parts of Component\Plugin for #2052109: [meta] Expand phpunit tests for \Drupal\Component\Plugin classes, I discovered that Drupal\Component\Plugin\Context\Context::getConstraints() tries to access an array property of an object.
/**
* Implements \Drupal\Component\Plugin\Context\ContextInterface::getConstraints().
*/
public function getConstraints() {
if (empty($this->contextDefinition['class'])) {
throw new ContextException("An error was encountered while trying to validate the context.");
}
return array(new Type($this->contextDefinition['class']));
}
$contextDefinition is defined as a \Drupal\Component\Plugin\Context\ContextDefinitionInterface.
After talking about it with @EclipseGC in IRC it seems that this is a bug.
It also seems that most of the Component aspect of Context is overridden by Core, which is why this bug hadn't been found yet.
Analyze a proper solution.
Analyze the requirements for Context::getConstraints().
Possibly modify ContextDefinitionInterface.
Modify Context::getConstraints() to conform with ContextDefinitionInterface.
Add PHPUnit test for the function Context::getConstraints()
| Disruption | Minimally disruptive because this code is mostly overridden by it's Drupal\Core namespace counterpart. |
|---|
| Comment | File | Size | Author |
|---|---|---|---|
| #21 | 2378297-21.patch | 5.53 KB | smustgrave |
| #21 | interdiff-8-21.txt | 0 bytes | smustgrave |
Comments
Comment #1
mile23Comment #2
daffie commentedUpdated IS with add PHPUnit test for the function Context::getConstraints().
Comment #3
mile23Comment #4
mile23This patch properly encapsulates
$contextDefinitionwithinContext, testsgetConstraints(), and updates a docblock or two.Comment #5
daffie commentedIt all looks good to me.
The patch fixed the problem as described in the issue summary.
All documentation is in order.
This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per beta-changes.
The patch gets a RTBC from me.
Comment #6
mile23Comment #7
alexpottWhy? Seems out of scope.
Why assert the count here and not the fact that we expect
according to the interface.
Should have a space between the @return and @throws
Let's just have two tests here with a help function to set up the the context to test.
Comment #8
mile23You know, that's my preference, but the last time this kind of thing came up, I was told to combine it. :-)
Because if you do a deep-dive, you learn that the *only* place this is documented is in the interface... All other implementations are documented as returning
array, without any hint of what the array is. However... If you look at the implementation we're dealing with here, it returns aTypeobject instead of aConstraint.Component/Context::getConstraint()is obviously sort of a reference implementation, so it might be important for DX to figure this out, but so far it's escaping me.So here's a patch with all the changes you suggest. It will fail because it loops through the array and check for the documented expectation of
Constraintobjects, which is obviously wrong. It would be great if someone with expertise on this system could address it.Comment #21
smustgrave commentedRerolled for D10 but as mentioned in #8 these will fail. But hopefully this puts new life in this issue.
Comment #22
quietone commentedThis issue was reviewed during a group triage meeting in #bugsmash. smustgrave and myself discussed the issue. I noted the issue looked valid and @smustgrave updated the patch.
The remaining tasks needs to be updated, tagging for an IS update.
Comment #23
smustgrave commentedMoving to NW for issue summary update.
Also patch #21 had a failure, which seems releated.