Set field value does not set trimmed token

This is caused by what was implemented in #3261414: Add or remove a value or values from a multi-value field → where ECA aims to only update field values, and consequently save entities, when they have actually changed. The code section looks like this:

        if (is_string($current_value)) {
          // Extra processing is needed for strings, in order to prevent false
          // comparison when dealing with values that are the same but
          // encoded differently.
          $current_value = nl2br(trim($current_value));
        }

        foreach ($values as $i => $value) {
          $new_value = !is_array($value) ? $value : (array_key_exists($property_name, $value) ? $value[$property_name] : reset($value));
          if (is_string($new_value)) {
            $new_value = nl2br(trim($new_value));
          }
          if (((is_object($new_value) && $current_value === $new_value) || ($current_value == $new_value)) && !isset($existing[$i]) && !in_array($k, $existing, TRUE)) {
            $existing[$i] = $k;
          }
          if (($i === $k) && is_array($value) && is_array($current_item) && (reset($method_settings) === 'set')) {
            $values[$i] += $current_item;
          }
        }

For string values, both the current and the new value get trimmed before comparison. That means in this use case that ECA always believes that the title hasn't changed if it only gets trimmed. When removing the trimming before comparison, then both methods from @Grimreaper (trimming title directly or through the tamper trim plugin) do work as expected. But that would cause other changes, that are probably unintended.

@mxh, do you have any idea how we can resolve this?

One possible way I currently see is to add a new value for the "Method" option called "Set and enforce clear previous value" set:force_clear where force_clear empties the arrays before set is being applied:

// ...
switch ($method_setting) {

          case 'force_clear':
            $keep = $existing = $current_values = [];
            break;

          case 'clear':
// ...

I've now implemented the suggested solution from @mxh which needs a review. But we also need to write a test for this.

I have added a test for force_clear. Looks good. I also added a return type in SetFieldValue.php

Please review and set issue to fixed if everything is ok.

Looked at the test and I think it doesn't actually test the problem why this fix got introduced: it's about trimming a field value.

Example: if the node title is my title with one or many spaces at the beginning and/or end of the string, and the use wants to replace that title with the trimmed version my title, then the set:clear function does NOT work, but the set:force_clear does.

So, we should have tests that confirm, for that case, that one mode works and the other doesn't.

Apart from that, there is a code style issue and the change of return type in modules/content/src/Plugin/Action/SetFieldValue.php is probably off-topic, and we may want to go through such smells (that are not reported by PHPCS or PHPStan) in a separate issue, if at all. I'm not sure how certain PHP versions behave if interfaces and subclasses have different notations of the same method.

Thanks for that hint. I will update the test. You are right, we should tackle this in another issue, but it is hard for me to ignore those smells.

I wrote a new test function testNodeTitleForceClear. As for as I understand your example above: The test creates a node with the title my title and the action sets the title to my title.

Whether the clear nor the force_clear works. See the failed test. Please take a look if the test is appropriate. If not, please adapt.

After debugging conditionally, the test is correct.:

In FiledUpdateActionBase the currentValue and the newValue get trimmed anyway. Line 280 and line 286. So line 289 gets executed and the this causes the function to jump out of the loop in line 298, because the arrays have the same size.

The method setting switch case gets never executed.

I've checked the method in FieldUpdateActionBase and realized that the Create a map of indices that refer to the already existing counterpart. part should only be executed if the method is NOT force_clear. That makes it work correctly and also fixed the test written by @danielspeicher in #11

Then, I've also added another test in the same method, that ensures that the method clear does NOT change the field value.

While being in that part of the code, I remove the nested ternary structures as they are difficult to read and should ever be used in PHP.

Cool. Think that will work now perfecly.

Great, thank you.

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