When using multi-line value for a condition, the ContextFormTrait.php (the class which is responsable to set the context for a rule), doesn't have in count that line break could come in this format
\r\n
As a consequence, the rule is stored without the right condition values, causing not to be triggered at all, only on certain conditions. Please note the context_values in my sample rule export to notify users when creating contents of a given type:
uuid: daddf6e2-8424-4580-b7a5-85fe24c2566a
langcode: en
status: true
dependencies: { }
id: new_resource_email_notification
label: 'New resource email notification'
events:
-
event_name: 'rules_entity_insert:node'
description: ''
tags:
- notification
config_version: '3'
expression:
id: rules_rule
uuid: 5655a08e-ea7c-49c5-85de-1a4d3ff8cbf9
conditions:
id: rules_and
uuid: 5f278a28-1cf6-45de-afb9-771b87f64940
conditions:
-
id: rules_condition
uuid: 81325c49-5072-445e-bcf3-ea2631c52247
context_values:
types:
- "resources\r"
- "jobs\r"
- test
context_mapping:
node: '@node.node_route_context:node'
context_processors:
types:
rules_tokens: { }
provides_mapping: { }
condition_id: rules_node_is_of_type
negate: false
actions:
id: rules_action_set
uuid: 9824fada-b389-4872-bf73-f213441e16df
actions:
-
id: rules_action
uuid: 6b298058-db7c-40fe-9b77-34d7f7dd3806
context_values:
to:
- mauro@thebrickfactory.com
subject: 'A New Resource Has Been Added'
message: 'A new content has been added'
reply: ''
language: ''
context_mapping: { }
context_processors:
to:
rules_tokens: { }
subject:
rules_tokens: { }
message:
rules_tokens: { }
reply:
rules_tokens: { }
language:
rules_tokens: { }
provides_mapping: { }
action_id: rules_send_email
I would suggest adding a line in the ContextFormTrait.php, before processing the multi-line field (after checking if the textarea has multiple context definitions):
protected function getContextConfigFromFormValues(FormStateInterface $form_state, array $context_definitions) {
$context_config = ContextConfig::create();
foreach ($form_state->getValue('context') as $context_name => $value) {
if ($form_state->get("context_$context_name") == 'selector') {
$context_config->map($context_name, $value['setting']);
}
else {
// Each line of the textarea is one value for multiple contexts.
if ($context_definitions[$context_name]->isMultiple()) {
// Replacing \n\r line breaks to prevent context values to break
$value['setting'] = str_replace(array("\r\n", "\r"), "\n", $value['setting']);
$values = explode("\n", $value['setting']);
$context_config->setValue($context_name, $values);
}
else {
$context_config->setValue($context_name, $value['setting']);
}
// For now, always add in the token context processor - if it's present.
// @todo: Improve this in https://www.drupal.org/node/2804035.
if ($this->getDataProcessorManager()->getDefinition('rules_tokens')) {
$context_config->process($context_name, 'rules_tokens');
}
}
}
return $context_config;
}
Will provide the patch in a minute. I took this line format from other core implementations such as CKEditor, Twig, YML Parser...
Please let me know if you have any suggestions, thanks
Comment | File | Size | Author |
---|---|---|---|
#10 | 2855433-10-clean-up-textarea-input.patch | 4.19 KB | TR |
| |||
#8 | 2855433-textarea-line-terminators.patch | 853 bytes | TR |
| |||
#4 | rules_multiple_context_values-2855433-4.patch | 636 bytes | mitsuroseba |
#2 | rules_multiple_context_values-2855433.patch | 714 bytes | mauro_ptt |
Comments
Comment #2
mauro_ptt CreditAttribution: mauro_ptt as a volunteer and at Brick Factory commentedComment #4
mitsuroseba CreditAttribution: mitsuroseba at FFW for FFW commentedAlso faced this issue. Put little changes if you don't mind.
Comment #6
TR CreditAttribution: TR commentedBranch tests are passing now, so retested this patch and it's now working too ...
Comment #7
TR CreditAttribution: TR commented@mauro_ptt: You assigned this issue to yourself - are you still planning to work on this?
Comment #8
TR CreditAttribution: TR commentedOK, I looked into this and I can confirm the problem. When we look at the raw text that was entered into a textarea in a browser, it should ALWAYS be terminated by \r\n. This is not a system-dependent line terminator, it is mandated by the HTML specification. However the specification recommends that we be lenient and also accept just \n in case a non-compliant application generated the HTML. All browsers use the correct \r\n as far as I know.
So the existing code, which splits the lines on \n, is doing the wrong thing; this is why you always see that extra \r at the end of your strings.
I thought it would be worthwhile to profile some ways to split the string, to see which way was faster. I tested on a ~4Mb text file with \r\n line breaks, and each way was tested in a separate test run. Here are the ways I tried:
The relative times are what is important.
You will see that the current (WRONG!) way of splitting the lines is the quickest, but since it's wrong we can't use it.
The patch in #4 is surprisingly 25 times slower!
The 'fancy' pattern, which to me is the most readable, is 11 times slower.
But the winner I think has to be the patch from #2, which is only 2 times slower (it also takes twice the memory of any of the other solutions, but we're talking about parsing the input of a textarea here, so hundreds of bytes at most - thus memory usage isn't really an issue IMO).
Here's a re-roll. I'm going to use this on my own site for a while just to make sure it doesn't have any unintended consequences, but I really would like a review from someone else before I commit this ...
Comment #9
jasonmce CreditAttribution: jasonmce at Howard Hughes Medical Institute commentedThe patch in #8 works in my local. It required me to re-save all my "Node is of type" conditions which was trivial.
Comment #10
TR CreditAttribution: TR commentedHere's a revised version of the patch. It differs slightly from the patch in #8 because it additionally strips empty lines from the input and strips whitespace from the beginning and end of each line. In practice, user's input a lot of strange things, so if we want this to work we have to make an effort in the code to clean up the input as much as possible.
Focusing on speed of the method was a mistake. While we should make an effort not to use inefficient code, it's far more important to do it correctly. And in this case, the code runs only when we are parsing user input, so the speed of ANY of the methods above is dwarfed by the user's response time.
This patch has an accompanying test to ensure it does the intended job.
Comment #12
TR CreditAttribution: TR commentedCommitted.