Problem/Motivation

Ignore Character settings were not saving correctly and resulted in incorrect data (e.g. content types without the underscores).

When you save the Index settings, all of the fields are checked as included automatically.
Example Below.
ignore-chars-screenshot
Config is written as 

    all_fields: false

without the fields: line.

    fields:
      - body
      - field_banner_body
      - name
      - saa_field_files
      - title

Proposed resolution

Fix submitConfigurationForm for the IgnoreCharacters Plugin to save all field configs.

Original report by [jonnyeom]

CommentFileSizeAuthor
#8 3041144-8--ignore_characters_plugin_form_bug.patch4.8 KBdrunken monkey
#8 3041144-8--ignore_characters_plugin_form_bug--interdiff.txt2.28 KBdrunken monkey
#7 3041144-7--ignore_characters_plugin_form_bug.patch2.77 KBdrunken monkey
#7 3041144-7--ignore_characters_plugin_form_bug--tests_only.patch1.92 KBdrunken monkey
#7 3041144-7--ignore_characters_plugin_form_bug--interdiff.txt2.76 KBdrunken monkey
#5 interdiff-2_5.txt630 bytesjonnyeom
#5 search_api-fix_ignore_chars_config_save-3041144-5.patch857 bytesjonnyeom
#2 search_api-fix_ignore_chars_config_save-3041144-2.patch858 bytesjonnyeom
Selection_036.png15.59 KBjonnyeom

Comments

jonnyeom created an issue. See original summary.

jonnyeom’s picture

jonnyeom commented
Status: Active » Needs review
StatusFileSize
new search_api-fix_ignore_chars_config_save-3041144-2.patch858 bytes
laravz’s picture

laravz commented

Hi, thanks for your work! The patch applied to cleanly to the latest dev version of the module.

  1. +++ b/src/Plugin/search_api/processor/IgnoreCharacters.php
@@ -103,7 +103,10 @@ class IgnoreCharacters extends FieldsProcessorPluginBase {
+    $config = $form_state->getValues();

    This solves the issue. The configuration is now being saved when the checkboxes are (un)checked. 

  2. +++ b/src/Plugin/search_api/processor/IgnoreCharacters.php
@@ -103,7 +103,10 @@ class IgnoreCharacters extends FieldsProcessorPluginBase {
+    // Get our own version of 'ignorable_classes' from form values.
+    unset($config['strip']);

    This is not necessary in order to resolve the bug. I'm not really sure why you would want to unset 'strip'. Could you elaborate?

jonnyeom’s picture

jonnyeom commented

@LaravZ

My intention was to prevent duplicate data being saved in the configuration. Here is the entire snippet after the patch.

    $config = $form_state->getValues();
    $config['ignorable'] = $form_state->getValue('ignorable');
    // Get our own version of 'ignorable_classes' from form values.
    unset($config['strip']);
    $classes = $form_state->getValue(['strip', 'character_sets'], []);
    $config['ignorable_classes'] = array_values(array_filter($classes));
    $this->setConfiguration($config);

The data in the 'strip' form fields are being saved under a different name.
If I don't unset it, this is the config that would save for that field (Pasting my entire config as an example).

array (
  'all_fields' => 0,
  'fields' => 
  array (
    0 => 'body',
    1 => 'field_banner_body',
    2 => 'name',
    3 => 'saa_field_files',
    4 => 'title',
  ),
  'ignorable' => '[\'¿¡!?,.:;/]',
  'strip' => 
  array (
    'character_sets' => 
    array (
      'Pc' => 'Pc',
      'Pd' => 'Pd',
      'Pe' => 'Pe',
      'Pf' => 'Pf',
      'Pi' => 'Pi',
      'Po' => 'Po',
      'Ps' => 'Ps',
      'Cc' => 0,
      'Cf' => 0,
      'Co' => 0,
      'Mc' => 0,
      'Me' => 0,
      'Mn' => 0,
      'Sc' => 0,
      'Sk' => 0,
      'Sm' => 0,
      'So' => 0,
      'Zl' => 0,
      'Zp' => 0,
      'Zs' => 0,
    ),
  ),
  'ignorable_classes' => 
  array (
    0 => 'Pc',
    1 => 'Pd',
    2 => 'Pe',
    3 => 'Pf',
    4 => 'Pi',
    5 => 'Po',
    6 => 'Ps',
  ),
)

But in reality, keeping the duplicate data will not cause any errors anywhere.

Thoughts?

jonnyeom’s picture

jonnyeom commented
StatusFileSize
new search_api-fix_ignore_chars_config_save-3041144-5.patch857 bytes
new interdiff-2_5.txt630 bytes

Realized we don't need to $form_state->getValue('ignorable') Anymore

laravz’s picture

laravz commented
Status: Needs review » Reviewed & tested by the community

The new patch also applied successfully, and the addition makes the code and resulting array cleaner. The issue also remains fixed.

The data in the 'strip' form fields are being saved under a different name. If I don't unset it, this is the config that would save for that field ...

Correct, I've verified this as well. I think it would be cleaner in the long run to slightly rework this so that it won't have to be resaved under a new name (and the unset won't be necessary). I do not, however, consider this to be a part of this commit, therefore I'll set this to RTBC.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 3041144-7--ignore_characters_plugin_form_bug--interdiff.txt2.76 KB
new 3041144-7--ignore_characters_plugin_form_bug--tests_only.patch1.92 KB
new 3041144-7--ignore_characters_plugin_form_bug.patch2.77 KB

Thanks a lot for reporting this! Seems we messed up back when fixing the form submission code in #3007933: IgnoreCharacters processor - “Strip by character property” not saving.
Your patch looks very good already, too, thanks for that, also! (And, of course, thanks @ LaravZ for reviewing!)

Anyways, we should also make sure the tests cover this in the future. Revised patch attached, please review! (No functional differences in non-test code.)

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
StatusFileSize
new 3041144-8--ignore_characters_plugin_form_bug--interdiff.txt2.28 KB
new 3041144-8--ignore_characters_plugin_form_bug.patch4.8 KB

In fact, let’s add that to all the processor integration tests.

The last submitted patch, 7: 3041144-7--ignore_characters_plugin_form_bug--tests_only.patch, failed testing. View results

jonnyeom’s picture

jonnyeom commented
Status: Needs review » Reviewed & tested by the community

Latest patch is working on our sites.

Thanks for the Test Updates!
Jonathan

laravz’s picture

laravz commented

I have no further remarks either. Patch applied cleanly and the 'all_fields' option is now taken into account with the unit tests. So this seems good to go, thanks!

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Reviewed & tested by the community » Fixed

Great to hear, thanks a lot for testing/reviewing!
Committed.
Thanks again, everyone!

Status: Fixed » Closed (fixed)

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