Skip to content

Commit f089923

Browse files
klausiklausi
committed
Issue #2655604: Added integrity checks for data type matching
1 parent c01bd1a commit f089923

File tree

4 files changed

+174
-17
lines changed

4 files changed

+174
-17
lines changed

‎src/Engine/IntegrityCheckTrait.php

Lines changed: 75 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,13 @@
77

88
namespace Drupal\rules\Engine;
99

10+
use Drupal\Core\Plugin\Context\ContextDefinitionInterface;
1011
use Drupal\Core\Plugin\ContextAwarePluginInterface as CoreContextAwarePluginInterface;
11-
use Drupal\rules\Context\ContextDefinitionInterface;
12+
use Drupal\Core\TypedData\ComplexDataInterface;
13+
use Drupal\Core\TypedData\DataDefinitionInterface;
14+
use Drupal\Core\TypedData\ListInterface;
15+
use Drupal\Core\TypedData\PrimitiveInterface;
16+
use Drupal\rules\Context\ContextDefinitionInterface as RulesContextDefinitionInterface;
1217
use Drupal\rules\Context\ContextProviderInterface;
1318
use Drupal\rules\Exception\RulesIntegrityException;
1419

@@ -33,52 +38,62 @@ protected function doCheckIntegrity(CoreContextAwarePluginInterface $plugin, Exe
3338
$violation_list = new IntegrityViolationList();
3439
$context_definitions = $plugin->getContextDefinitions();
3540

36-
foreach ($context_definitions as $name => $definition) {
41+
foreach ($context_definitions as $name => $context_definition) {
3742
// Check if a data selector is configured that maps to the state.
3843
if (isset($this->configuration['context_mapping'][$name])) {
3944
try {
4045
$data_definition = $metadata_state->fetchDefinitionByPropertyPath($this->configuration['context_mapping'][$name]);
46+
47+
$this->checkDataTypeCompatible($context_definition, $data_definition, $name, $violation_list);
4148
}
4249
catch (RulesIntegrityException $e) {
4350
$violation = new IntegrityViolation();
4451
$violation->setMessage($this->t('Data selector %selector for context %context_name is invalid. @message', [
4552
'%selector' => $this->configuration['context_mapping'][$name],
46-
'%context_name' => $definition->getLabel(),
53+
'%context_name' => $context_definition->getLabel(),
4754
'@message' => $e->getMessage(),
4855
]));
4956
$violation->setContextName($name);
5057
$violation_list->add($violation);
5158
}
5259

53-
if ($definition instanceof ContextDefinitionInterface
54-
&& $definition->getAssignmentRestriction() === ContextDefinitionInterface::ASSIGNMENT_RESTRICTION_INPUT
60+
if ($context_definition instanceof RulesContextDefinitionInterface
61+
&& $context_definition->getAssignmentRestriction() === RulesContextDefinitionInterface::ASSIGNMENT_RESTRICTION_INPUT
5562
) {
5663
$violation = new IntegrityViolation();
5764
$violation->setMessage($this->t('The context %context_name may not be configured using a selector.', [
58-
'%context_name' => $definition->getLabel(),
65+
'%context_name' => $context_definition->getLabel(),
5966
]));
6067
$violation->setContextName($name);
6168
$violation_list->add($violation);
6269
}
6370
}
6471
elseif (isset($this->configuration['context_values'][$name])) {
65-
if ($definition instanceof ContextDefinitionInterface
66-
&& $definition->getAssignmentRestriction() === ContextDefinitionInterface::ASSIGNMENT_RESTRICTION_SELECTOR
72+
if ($context_definition instanceof RulesContextDefinitionInterface
73+
&& $context_definition->getAssignmentRestriction() === RulesContextDefinitionInterface::ASSIGNMENT_RESTRICTION_SELECTOR
6774
) {
6875
$violation = new IntegrityViolation();
6976
$violation->setMessage($this->t('The context %context_name may only be configured using a selector.', [
70-
'%context_name' => $definition->getLabel(),
77+
'%context_name' => $context_definition->getLabel(),
7178
]));
7279
$violation->setContextName($name);
7380
$violation_list->add($violation);
7481
}
7582
}
83+
elseif ($context_definition->isRequired()) {
84+
$violation = new IntegrityViolation();
85+
$violation->setMessage($this->t('The required context %context_name is missing.', [
86+
'%context_name' => $context_definition->getLabel(),
87+
]));
88+
$violation->setContextName($name);
89+
$violation_list->add($violation);
90+
}
7691
}
7792

7893
if ($plugin instanceof ContextProviderInterface) {
7994
$provided_context_definitions = $plugin->getProvidedContextDefinitions();
8095

81-
foreach ($provided_context_definitions as $name => $definition) {
96+
foreach ($provided_context_definitions as $name => $context_definition) {
8297
if (isset($this->configuration['provides_mapping'][$name])) {
8398
if (!preg_match('/^[0-9a-zA-Z_]*$/', $this->configuration['provides_mapping'][$name])) {
8499
$violation = new IntegrityViolation();
@@ -92,15 +107,63 @@ protected function doCheckIntegrity(CoreContextAwarePluginInterface $plugin, Exe
92107
// Populate the state with the new variable that is provided by this
93108
// plugin. That is necessary so that the integrity check in subsequent
94109
// actions knows about the variable and does not throw violations.
95-
$metadata_state->setDataDefinition($this->configuration['provides_mapping'][$name], $definition->getDataDefinition());
110+
$metadata_state->setDataDefinition(
111+
$this->configuration['provides_mapping'][$name],
112+
$context_definition->getDataDefinition()
113+
);
96114
}
97115
else {
98-
$metadata_state->setDataDefinition($name, $definition->getDataDefinition());
116+
$metadata_state->setDataDefinition($name, $context_definition->getDataDefinition());
99117
}
100118
}
101119
}
102120

103121
return $violation_list;
104122
}
105123

124+
/**
125+
* Checks that the data type of a mapped variable matches the expectation.
126+
*
127+
* @param \Drupal\Core\Plugin\Context\ContextDefinitionInterface $context_definition
128+
* The context definition of the context on the plugin.
129+
* @param \Drupal\Core\TypedData\DataDefinitionInterface $provided
130+
* The data definition of the mapped variable to the context.
131+
* @param string $context_name
132+
* The name of the context on the plugin.
133+
* @param \Drupal\rules\Engine\IntegrityViolationList $violation_list
134+
* The list of violations where new ones will be added.
135+
*/
136+
protected function checkDataTypeCompatible(ContextDefinitionInterface $context_definition, DataDefinitionInterface $provided, $context_name, IntegrityViolationList $violation_list) {
137+
$expected_class = $context_definition->getDataDefinition()->getClass();
138+
$provided_class = $provided->getClass();
139+
$expected_type_problem = NULL;
140+
141+
if (is_subclass_of($expected_class, PrimitiveInterface::class)
142+
&& !is_subclass_of($provided_class, PrimitiveInterface::class)
143+
) {
144+
$expected_type_problem = $this->t('primitive');
145+
}
146+
elseif (is_subclass_of($expected_class, ListInterface::class)
147+
&& !is_subclass_of($provided_class, ListInterface::class)
148+
) {
149+
$expected_type_problem = $this->t('list');
150+
}
151+
elseif (is_subclass_of($expected_class, ComplexDataInterface::class)
152+
&& !is_subclass_of($provided_class, ComplexDataInterface::class)
153+
) {
154+
$expected_type_problem = $this->t('complex');
155+
}
156+
157+
if ($expected_type_problem) {
158+
$violation = new IntegrityViolation();
159+
$violation->setMessage($this->t('Expected a @expected_type data type for context %context_name but got a @provided_type data type instead.', [
160+
'@expected_type' => $expected_type_problem,
161+
'%context_name' => $context_definition->getLabel(),
162+
'@provided_type' => $provided->getDataType(),
163+
]));
164+
$violation->setContextName($context_name);
165+
$violation_list->add($violation);
166+
}
167+
}
168+
106169
}

‎src/Form/Expression/ActionForm.php

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,8 @@ public function form(array $form, FormStateInterface $form_state) {
7373
'#type' => 'submit',
7474
'#value' => $this->t('Continue'),
7575
'#name' => 'continue',
76+
// Only validate the selected action in the first step.
77+
'#limit_validation_errors' => [['action']],
7678
'#submit' => [static::class . '::submitFirstStep'],
7779
];
7880

‎src/Form/Expression/ConditionForm.php

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,8 @@ public function form(array $form, FormStateInterface $form_state) {
7474
'#type' => 'submit',
7575
'#value' => $this->t('Continue'),
7676
'#name' => 'continue',
77+
// Only validate the selected condition in the first step.
78+
'#limit_validation_errors' => [['condition']],
7779
'#submit' => [static::class . '::submitFirstStep'],
7880
];
7981

‎tests/src/Integration/Engine/IntegrityCheckTest.php

Lines changed: 95 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ public function testIntegrityCheck() {
3131
$violation_list = RulesComponent::create($rule)
3232
->addContextDefinition('entity', ContextDefinition::create('entity'))
3333
->checkIntegrity();
34-
$this->assertEquals(iterator_count($violation_list), 0);
34+
$this->assertEquals(0, iterator_count($violation_list));
3535
}
3636

3737
/**
@@ -45,7 +45,7 @@ public function testUnknownVariable() {
4545

4646
$violation_list = RulesComponent::create($rule)
4747
->checkIntegrity();
48-
$this->assertEquals(iterator_count($violation_list), 1);
48+
$this->assertEquals(1, iterator_count($violation_list));
4949
$violation = $violation_list[0];
5050
$this->assertEquals(
5151
'Data selector <em class="placeholder">unknown_variable</em> for context <em class="placeholder">Entity</em> is invalid. Unable to get variable unknown_variable, it is not defined.',
@@ -145,7 +145,7 @@ public function testInvalidProvidedName() {
145145

146146
$violation_list = RulesComponent::create($rule)
147147
->checkIntegrity();
148-
$this->assertEquals(iterator_count($violation_list), 1);
148+
$this->assertEquals(1, iterator_count($violation_list));
149149
$this->assertEquals(
150150
'Provided variable name <em class="placeholder">invalid_näme</em> contains not allowed characters.',
151151
(string) $violation_list[0]->getMessage()
@@ -168,7 +168,7 @@ public function testInputRestriction() {
168168
$violation_list = RulesComponent::create($rule)
169169
->addContextDefinition('variable_1', ContextDefinition::create('string'))
170170
->checkIntegrity();
171-
$this->assertEquals(iterator_count($violation_list), 1);
171+
$this->assertEquals(1, iterator_count($violation_list));
172172
$this->assertEquals(
173173
'The context <em class="placeholder">Entity type</em> may not be configured using a selector.',
174174
(string) $violation_list[0]->getMessage()
@@ -190,11 +190,101 @@ public function testSelectorRestriction() {
190190

191191
$violation_list = RulesComponent::create($rule)
192192
->checkIntegrity();
193-
$this->assertEquals(iterator_count($violation_list), 1);
193+
$this->assertEquals(1, iterator_count($violation_list));
194194
$this->assertEquals(
195195
'The context <em class="placeholder">Data</em> may only be configured using a selector.',
196196
(string) $violation_list[0]->getMessage()
197197
);
198198
}
199199

200+
/**
201+
* Tests that a primitive context is assigned something that matches.
202+
*/
203+
public function testPrimitiveTypeViolation() {
204+
$rule = $this->rulesExpressionManager->createRule();
205+
206+
// The condition expects a string but we pass a list, which will trigger the
207+
// violation.
208+
$rule->addCondition('rules_test_string_condition', ContextConfig::create()
209+
->map('text', 'list_variable')
210+
);
211+
212+
$violation_list = RulesComponent::create($rule)
213+
->addContextDefinition('list_variable', ContextDefinition::create('list'))
214+
->checkIntegrity();
215+
$this->assertEquals(1, iterator_count($violation_list));
216+
$this->assertEquals(
217+
'Expected a primitive data type for context <em class="placeholder">Text to compare</em> but got a list data type instead.',
218+
(string) $violation_list[0]->getMessage()
219+
);
220+
}
221+
222+
/**
223+
* Tests that a list context is assigned something that matches.
224+
*/
225+
public function testListTypeViolation() {
226+
$rule = $this->rulesExpressionManager->createRule();
227+
228+
// The condition expects a list for the type context but we pass a node
229+
// which will trigger the violation.
230+
$rule->addCondition('rules_node_is_of_type', ContextConfig::create()
231+
->map('node', 'node')
232+
->map('types', 'node')
233+
);
234+
235+
$violation_list = RulesComponent::create($rule)
236+
->addContextDefinition('node', ContextDefinition::create('entity:node'))
237+
->checkIntegrity();
238+
$this->assertEquals(1, iterator_count($violation_list));
239+
$this->assertEquals(
240+
'Expected a list data type for context <em class="placeholder">Content types</em> but got a entity:node data type instead.',
241+
(string) $violation_list[0]->getMessage()
242+
);
243+
}
244+
245+
/**
246+
* Tests that a complex data context is assigned something that matches.
247+
*/
248+
public function testComplexTypeViolation() {
249+
$rule = $this->rulesExpressionManager->createRule();
250+
251+
// The condition expects a node context but gets a list instead which cause
252+
// the violation.
253+
$rule->addCondition('rules_node_is_of_type', ContextConfig::create()
254+
->map('node', 'list_variable')
255+
->map('types', 'list_variable')
256+
);
257+
258+
$violation_list = RulesComponent::create($rule)
259+
->addContextDefinition('list_variable', ContextDefinition::create('list'))
260+
->checkIntegrity();
261+
$this->assertEquals(1, iterator_count($violation_list));
262+
$this->assertEquals(
263+
'Expected a complex data type for context <em class="placeholder">Node</em> but got a list data type instead.',
264+
(string) $violation_list[0]->getMessage()
265+
);
266+
}
267+
268+
/**
269+
* Tests that an absent required context triggers a violation.
270+
*/
271+
public function testMissingRequiredContext() {
272+
$rule = $this->rulesExpressionManager->createRule();
273+
274+
// The condition is completely unconfigured, missing 2 required contexts.
275+
$rule->addCondition('rules_node_is_of_type');
276+
277+
$violation_list = RulesComponent::create($rule)
278+
->checkIntegrity();
279+
$this->assertEquals(2, iterator_count($violation_list));
280+
$this->assertEquals(
281+
'The required context <em class="placeholder">Node</em> is missing.',
282+
(string) $violation_list[0]->getMessage()
283+
);
284+
$this->assertEquals(
285+
'The required context <em class="placeholder">Content types</em> is missing.',
286+
(string) $violation_list[1]->getMessage()
287+
);
288+
}
289+
200290
}

0 commit comments

Comments
 (0)