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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mauro_ptt created an issue. See original summary.

mauro_ptt’s picture

Status: Needs review » Needs work

The last submitted patch, 2: rules_multiple_context_values-2855433.patch, failed testing.

mitsuroseba’s picture

Also faced this issue. Put little changes if you don't mind.

Status: Needs review » Needs work

The last submitted patch, 4: rules_multiple_context_values-2855433-4.patch, failed testing. View results

TR’s picture

Status: Needs work » Needs review

Branch tests are passing now, so retested this patch and it's now working too ...

TR’s picture

@mauro_ptt: You assigned this issue to yourself - are you still planning to work on this?

TR’s picture

OK, 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:

// Current code
$values = explode("\n", $contents); // 0.0091030597686768 sec

// 'Proper' line terminators - this isn't lenient however
$values = explode("\r\n", $contents); // 0.0093328952789307 sec

// 'Fancy' pattern - splits on \r, \n, or \r\n
$values = preg_split("/\R/", $contents); // 0.10002207756042 sec

// 'Explicit' pattern  - also splits on \r, \n, or \r\n
// This is what is used in patch #4
$values = preg_split("/\r\n|\r|\n/", $contents); // 0.23250293731689 sec

// Without preg_split - also splits on \r, \n, or \r\n
// This is what is used in patch #2
$temp = str_replace(["\r\n", "\r"], "\n", $contents); // 0.01739501953125 sec
$values = explode("\n", $temp);

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 ...

jasonmce’s picture

The patch in #8 works in my local. It required me to re-save all my "Node is of type" conditions which was trivial.

TR’s picture

Here'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.

  • TR committed 743a8d7 on 8.x-3.x
    Issue #2855433 by TR, mauro_ptt: ContextFormTrait.php not processing...
TR’s picture

Status: Needs review » Fixed

Committed.

Status: Fixed » Closed (fixed)

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