geocoder_field preprocessors alter field data permanently

Created on 11 July 2023, over 1 year ago

Problem/Motivation

I'm trying to implement a preprocessor plugin that filters out values, entirely, from Address fields.

The problem is that the alterations I make to the values end up getting saved back. The field object is passed by reference all the way from hook_entity_presave() into preprocessors.

It is probably a happy accident that this has not been more of a problem with the geocoder_field module. Many source field types, such as address, do not have a "value" property on their items and so it's harmless for preprocessors of those fields to set it. But if a preprocessor happens to set a property that does get saved to the database - or if, like the one I am trying to implement, it alters the number of values altogether - those changes end up getting saved to the entity permanently.

Steps to reproduce

Install the Address β†’ , Geocoder, and Geocoder Field modules. On a content entity type, create an Address Field and a destination field type. (In my case I used the Geolocation field type from the module of the same name. I don't believe the type of destination field is relevant.) Configure the destination field to geocode from the Address field.

Implement a custom preprocessor such as the following that removes values in preprocessing.

namespace Drupal\my_module\Plugin\Geocoder\Preprocessor;

use Drupal\geocoder_field\PreprocessorBase;

/**
 * Removes values with a country code only from processing.
 *
 * @GeocoderPreprocessor(
 *   id = "conditional_address",
 *   name = "Conditional Address",
 *   field_types = {
 *     "address"
 *   },
 *   weight = -10
 * )
 */
class ConditionalAddress extends PreprocessorBase {

  /**
   * {@inheritdoc}
   */
  public function preprocess() {
    parent::preprocess();

    $values = [];
    foreach ($this->field->getValue() as $value) {
      if (!is_array($value)) { continue; }

      foreach ($value as $key => $property) {
        if (in_array($key, ['country_code', 'langcode'], TRUE)) {
          continue;
        }

        if ($property) {
          $values[] = $value;
          break;
        }
      }
    }
    $this->field->setValue($values, FALSE);

    return $this;
  }

}

I don't want to set empty values, because that leads to warnings being messaged and/or logged if that is turned on in field configuration. And I do want them on, for any other types of failures. But this preprocessor is not meant to introduce failures - it's meant to filter what values even get an attempt at geocoding.

Proposed resolution

In the preprocessor plugin manager, clone the field object before setting it on preprocessor plugins.

Remaining tasks

Code review, and hopefully merge

API changes

This will interfere with GeocoderPreprocessor plugin's abilities to change the values saved to the source fields they preprocess. However, I do not imagine this capability was intended in the first place.

I also do not think it would close the door completely on this kind of behavior if someone really wanted to implement it, because ->getEntity() still works on the cloned field object.

πŸ› Bug report
Status

Needs review

Version

4.0

Component

Code

Created by

πŸ‡ΊπŸ‡ΈUnited States bvoynick

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Production build 0.71.5 2024