Keyword Filter Plugin

You could mock FormStateInterface and in order to return a value for $form_state->getValue() you could let PHPUnit return a value map: https://phpunit.de/manual/6.5/en/test-doubles.html#test-doubles.stubs.ex...

Example (untested):

$map = [
  [KeywordFilter::SETTING_WORDS, 'booya'],
  [KeywordFilter::SETTING_WORD_BOUNDARIES, FALSE],
];
$form_state = $this->createMock(FormStateInterface::class);
$form_state->expects($this->any())
  ->method('getValue')
  ->will($this->returnValueMap($map));

An other option is to use Prophecy: https://github.com/phpspec/prophecy
I haven't used it much yet, but I believe it would work something like this:

$form_state = $this->prophesize(FormStateInterface::class);
$form_state->getValue(KeywordFilter::SETTING_WORDS)->willReturn('booya');
$form_state->getValue(KeywordFilter::SETTING_WORD_BOUNDARIES)->willReturn(FALSE);

As you see, it replaces ->method('methodName') with ->methodName() as if you were actually calling the method. It could be that the order in which the method is called with each given value matters, I'm not sure.

Finally, if a method tends to become very complex, it could be a good idea to put part of it into a new protected method. This way, you could write an unit test for just that piece of code in validateConfigurationForm() that doesn't necessarily do validating. Not sure if it would be a good idea to that in this case as I see in the more complex part of the method also a call to setErrorByName() on the form state object.

Submitting forms don't necessarily work in unit tests. If you call methods like validateConfigurationForm() in unit tests, you want to purely test the code in validateConfigurationForm(). The test method would then be called testValidateConfigurationForm(), testConfigurationForm() or alike. Feeds has some example unit tests for configuration forms. See the test classes in feeds/tests/src/Unit/Feeds/Fetcher/Form.

It looks like that the test testStriPosFilter() shouldn't be focussing on the form functions at all. Testing the Tamper plugins behavior when calling tamper() on it should focus on providing the Tamper configuration as it gets stored, and disregard the fact that these values get there via a form.

Tests that test validating or submitting the form, should test what configuration gets stored based on the input values. Testing what tamper() would then do is out of scope for that test.

I haven't take a thorough look but be caution of the way you are using setConfiguration.

This is from the ConfigurablePluginInterface - it is expected to be the whole configuration of the plugin, e.g. when the plugin is created. Its purpose is not for individual values, it is expected the whole configuration.

By setting an individual value, you are saying set this value and use the default values for everything else.

E.g instead of

$this->setConfiguration([self::SETTING_WORDS => $form_state->getValue(self::SETTING_WORDS)]);
$this->setConfiguration([self::SETTING_WORD_BOUNDARIES => $form_state->getValue(self::SETTING_WORD_BOUNDARIES)]);
$this->setConfiguration([self::SETTING_EXACT => $form_state->getValue(self::SETTING_EXACT)]);
$this->setConfiguration([self::SETTING_CASE_SENSITIVE => $form_state->getValue(self::SETTING_CASE_SENSITIVE)]);
$this->setConfiguration([self::SETTING_INVERT => $form_state->getValue(self::SETTING_INVERT)]);

It needs to be

$this->setConfiguration([
  self::SETTING_WORDS => $form_state->getValue(self::SETTING_WORDS),
  self::SETTING_WORD_BOUNDARIES => $form_state->getValue(self::SETTING_WORD_BOUNDARIES),
  self::SETTING_EXACT => $form_state->getValue(self::SETTING_EXACT),
  self::SETTING_CASE_SENSITIVE => $form_state->getValue(self::SETTING_CASE_SENSITIVE),
  self::SETTING_INVERT => $form_state->getValue(self::SETTING_INVERT),
]);

Thanks James - massive effort, looks like a fairly complex set of options to manage. I'm going to have a bit of a think about how the tamper method is being called, for now here is a few bits of minor feedback.

1. +++ b/src/Plugin/Tamper/KeywordFilter.php
   @@ -0,0 +1,210 @@
   + * @Tamper(
   

   Needs the handles multiple annotation property

2. +++ b/src/Plugin/Tamper/KeywordFilter.php
   @@ -0,0 +1,210 @@
   +      [self::SETTING_WORDS => $form_state->getValue(self::SETTING_WORDS)],
   

   This can only work with an array of configuraiton, here it is passing in 5 arrays.

1. +++ b/src/Plugin/Tamper/KeywordFilter.php
   @@ -0,0 +1,210 @@
   +    global $multibyte;
   

   This has been removed in Drupal 8. See https://www.drupal.org/node/1992584 for the change record. We will need to work with the new Unicode class.

2. +++ b/src/Plugin/Tamper/KeywordFilter.php
   @@ -0,0 +1,210 @@
   +            $form_state->setErrorByName(conf::SETTING_WORDS, $this->t('Search text must begin and end with a letter, number, or underscore to use the %option option.', ['%option' => t('Respect word boundaries')]));
   

   Typo - conf looks like it should be self

3. +++ b/src/Plugin/Tamper/KeywordFilter.php
   @@ -0,0 +1,210 @@
   +    $this->setConfiguration($config);
   

   I'm not a fan of setting any configuration in the validation method.

   Validation should be ensuring the all the form values are correct. If we need to massage or change any of the user input, I think its more common to put those values back into the form state object.

   We should only be configuring the plugin in the submit handler.

4. +++ b/src/Plugin/Tamper/KeywordFilter.php
   @@ -0,0 +1,210 @@
   +  protected function feedsTamperKeywordFilterMatch($match_func, $field, array $word_list) {
   

   Naming conventions for methods should reflect what this is doing, e.g. "match" - we don't need to prefix it with our module / plugin names.

Thanks for the feedback, Eric

@James, I have managed fix the above feedbacks except this one.

+++ b/src/Plugin/Tamper/KeywordFilter.php
@@ -0,0 +1,210 @@
+ $this->setConfiguration($config);
I'm not a fan of setting any configuration in the validation method.

Validation should be ensuring the all the form values are correct. If we need to massage or change any of the user input, I think its more common to put those values back into the form state object.

We should only be configuring the plugin in the submit handler.

Updated patch with changes in validation and submit handler to handle configuration settings in submit handler only.

Tagging for DrupalCamp Asheville

Tagging for the next to North American contrib days, Asheville and Colorado

DrupalCamp Asheville contrib days are July 13-14, 2019
DrupalCamp Colorado contrib day is Aug 4, 2019

I'd like to set RTBC but constants needs better names and docs

+++ b/src/Plugin/Tamper/KeywordFilter.php
@@ -0,0 +1,211 @@
+  const SETTING_WORDS = 'words';
+  const SETTING_WORD_BOUNDARIES = 'word_boundaries';
+  const SETTING_EXACT = 'exact';
+  const SETTING_CASE_SENSITIVE = 'case_sensitive';
+  const SETTING_INVERT = 'invert';
+  const SETTING_WORD_LIST = 'word_list';
+  const SETTING_REGEX = 'regex';
+  const SETTING_FUNCTION = 'function';
...
+    $config[self::SETTING_WORDS] = '';
+    $config[self::SETTING_WORD_BOUNDARIES] = FALSE;
+    $config[self::SETTING_EXACT] = FALSE;
+    $config[self::SETTING_CASE_SENSITIVE] = FALSE;
+    $config[self::SETTING_INVERT] = FALSE;
+    $config[self::SETTING_WORD_LIST] = [];
+    $config[self::SETTING_REGEX] = FALSE;
+    $config[self::SETTING_FUNCTION] = 'matchRegex';

It really hard to read "SETTINGS_" prefix and harder to type (use in code)
Also each one needs docs

I tried patches #22 and #30 in DP 8.7.7 (feeds 8.x-3.0-alpha5, feeds_tamper 8.x-2.0-beta1, feeds_ex 8.x-1.0-alpha2), and something still seems to be missing.

I get error "method not found" for "feedsTamperKeywordFilterMatch". Is there another patch file for KeywordFilter.php with that function?

Also, I was getting a form validation error until I added “use Drupal\Component\Utility\Unicode;” to the header of KeywordFilter.php.

OK, patch #31 works. You can disregard my #35 post.

Hi guys, this looks like the filter I'm looking for but is there a way to return the key for the matched keyword instead of the values?

array('Item1' => 'Yes', 'Item2' => 'No', 'Item3' => 'No');
// Keyword filter 'Yes', should return array('Item1');

Thanks for your help!

@jamesdixon made several changes here. Please have a look.

Standards for constants are
- outside of interface/class https://www.drupal.org/docs/develop/standards/coding-standards#s-constants
- inside should confirm https://www.drupal.org/node/2831620

Examples are all over core like https://git.drupalcode.org/project/drupal/-/blob/8.8.x/core/modules/node...

Hello @james,
Thanks for clarifications.
I've create one more changes package. Just a remark.

The three constants below has no any visual filter (in attachment https://www.drupal.org/files/issues/2020-05-28/admin-structure-feeds-man...). I also did not find them on tamper page. Please clarify where can I get to know the description. I add for regex something but not sure

  /**
   * WORD_LIST NTB.
   */

  const WORD_LIST = 'word_list';

  /**
   * If checked, use Regex mode for processing.
   */

  const REGEX = 'regex';

  /**
   * Particular regular expression for processing data.
   */

  const FUNCTION = 'function';

Re-roll (math accepted), fix CS around consts and address last 2 comments

Also converted test to use data provider so all sets for test in one place

PS: sadly interdiff bugger then patch

Hello @andypost

What do you mean under "PS: sadly interdiff bugger then patch" ?

@zabej It means that reading the patch more productive then interdiff which is repeatable pattern