We need to port the feeds tamper trim plugin into a tamper plugin for Drupal 8.

CommentFileSizeAuthor
#53 tamper-convertboolean-plugin-2962792-53.patch3.07 KBjamesdixon
#51 tamper-convertboolean-plugin-2962792-51.patch627 bytesjamesdixon
#47 other-text-works.png17.31 KBjamesdixon
#46 interdiff-45-46.txt512 bytesmitrpaka
#46 convert-boolean-2962792-46.patch9.83 KBmitrpaka
#45 interdiff-44-45.txt3.84 KBmitrpaka
#45 convert-boolean-2962792-45.patch9.83 KBmitrpaka
#44 convert-boolean-up-date-test-2962792-44.patch10.09 KBvijay.mayilsamy
#43 convert-boolean-up-date-test-2962792-43.patch10.07 KBericgsmith
#43 interdiff-2962792.diff4.09 KBericgsmith
#41 convert_boolean-2962792-41.patch9.76 KBvijay.mayilsamy
#39 convert_boolean-2962792-39.patch9.6 KBvijay.mayilsamy
#33 convert_boolean-2962792-33.patch9.16 KBvijay.mayilsamy
#29 tamper-convertboolean-plugin-2962792-29.diff5.87 KBjamesdixon
#26 tamper-convertboolean-plugin-2962792-28.diff5.83 KBjamesdixon
#23 tamper-convertboolean-plugin-2962792-22.diff5.56 KBirinaz
#21 tamper-convertboolean-plugin-2962792-21.diff5.56 KBirinaz
#20 tamper-convertboolean-plugin-2962792-20.patch0 bytesirinaz
#18 tamper-convertboolean-plugin-2962792-18.diff5.51 KBirinaz
#17 tamper-convertboolean-plugin-2962792-17c.diff3.59 KBirinaz
#16 tamper-convertboolean-plugin-2962792-17.diff5.67 KBirinaz
#15 tamper-convertboolean-plugin-2962792-15.diff6.99 KBirinaz
#13 tamper-convertboolean-plugin-2962792-13.diff5.86 KBjamesdixon
#10 tamper-convertboolean-plugin-2962792-9.diff4.41 KBirinaz
#6 tamper-convertboolean-plugin-2962792-2.diff26.79 KBirinaz
#3 tamper-convertboolean-plugin-2962792.diff5.05 KBirinaz
#2 patch.diff846 bytesirinaz
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

irinaz created an issue. See original summary.

irinaz’s picture

Status: Needs review » Needs work
Related issues: -#2958013: [META] port remaining feeds tamper plugins
FileSize
846 bytes
irinaz’s picture

FileSize
5.05 KB
jamesdixon’s picture

It looks like you had a similar issue that I was having: my PHP error message wasn't being reported in the watchdog logs. When checking my apache error log at /var/log/apache2/error_log, I get this error:

[Fri Apr 20 19:52:02.835943 2018] [:error] [pid 26455] [client 127.0.0.1:49068] PHP Fatal error:  Undefined class constant 'SETTING_OPERATION' in /var/www/html/drupal/modules/tamper/src/Plugin/Tamper/ConvertBoolean.php on line 34, referer: http://www.thedrupal.com/admin/structure/feeds/manage/cast_number_to_int_test/tamper/add/title?destination=/admin/structure/feeds/manage/cast_number_to_int_test/tamper

In defaultConfiguration(), you should instead define defaults for all the constants you have defined above:

  public function defaultConfiguration() {
    $config = parent::defaultConfiguration();
    // line below causing error, remove it (no SETTING_OPERATION exists)
    //$config[self::SETTING_OPERATION] = 'ucfirst';
    $config[self::SETTING_TRUTH_VALUE = 'true';
    $config[self::SETTING_FALSE_VALUE = 'false';
    // ... and so on for the other config constants you've defined above
    return $config;
  }

Great start!

irinaz’s picture

@jamesdixon, I have implemented config changes, but now I see the following error
Class Drupal\tamper\Plugin\Tamper\ConvertBoolean contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (Drupal\tamper\TamperBase::tamperSingleValue)

I do not have tamperSingleValue anywhere in my module, and not sure what is right way to solve this issue.

irinaz’s picture

MegaChriz’s picture

@irinaz
tamperSingleValue() has been removed a few weeks ago. You'll need to checkout the latest Tamper code.
In the patch in #6 you accidentally added a diff for Feeds Tamper as well.

The way I work when creating a patch:

  1. Git clone of project. In this case git clone --branch 8.x-1.x https://git.drupal.org/project/tamper.git (see also https://www.drupal.org/node/1992672/git-instructions/8.x-1.x/nonmaintainer)
  2. On the command line, go to the directory of the project. In this case cd tamper.
  3. Create a new branch. I give it the name of the thing I'm working on. For example: git checkout -b convert_boolean.2962792.8
  4. Make my code changes.
  5. Add the changes and commit them to my local branch:
    git add .
    git commit -m "Worked on convert boolean plugin."
  6. Create a patch: git diff 8.x-1.x > ../_patches/tamper-convert-boolean-2962792-8.patch

(I have written a shell script to automatically match the patch name with the branch name.)

irinaz’s picture

@MegaChriz, thanks a lot! I will update Tamper plugin and submit new patch.

irinaz’s picture

I updated Tamper and now form is showing in tamper plugin :)) . I am stuck now with setting correctly operation in function tamper. I copy/pasted code from convert case plugin, but not sure what it is doing...

$operation = $this->getSetting(self::SETTING_TRUTH_VALUE);

public function tamper($data, TamperableItemInterface $item = NULL) {
    if (!is_string($data)) {
      throw new TamperException('Input should be a string.');
    }
    $operation = $this->getSetting(self::SETTING_TRUTH_VALUE);
    if (!is_callable(['\Drupal\Component\Utility\Unicode', $operation])) {
      throw new TamperException('Invalid operation ' . $operation);
    }

    return call_user_func(['\Drupal\Component\Utility\Unicode', $operation], $data);
  }
irinaz’s picture

irinaz’s picture

jamesdixon’s picture

The idea here is to copy what's in the feeds_tamper plugin, and convert it to work with the D8 class. Here is that feeds_tamper code:

function feeds_tamper_convert_boolean_callback($result, $item_key, $element_key, &$field, $settings, $source) {
  // Copy field value in case 'pass' is set.
  $match_field = $field;
  if (!$settings['match_case']) {
    $match_field = drupal_strtolower($match_field);
  }
  if ($match_field == $settings['true_value']) {
    $field = 1;
    return;
  }
  if ($match_field == $settings['false_value']) {
    $field = 0;
    return;
  }
  if ($settings['no_match'] == 'pass') {
    return;
  }
  $field = $settings['no_match_value'];
}

So in this case $field is actually $data in your function, so you change all $field to $data instead.

In D8 we have no $settings so you instead use your settings you defined above. For example, don't use $settings[false_value'].

Instead use:

$this->getSetting(self::SETTING_FALSE_VALUE);

Basically you're using $data instead of field, and grabbing $settings from $this->getSetting();

jamesdixon’s picture

Here is the latest patch by @irinaz. Still running into some PHP errors but getting much closer!

MegaChriz’s picture

More or less copied from the #feeds channel in Slack:

  1. +++ b/src/Plugin/Tamper/ConvertBoolean.php
    @@ -0,0 +1,164 @@
    +      $form['true_value'] = array(
    ...
    +      $form['false_value'] = array(
    ...
    +      $form['match_case'] = array(
    ...
    +      $form['no_match'] = array(
    ...
    +      $form['other_text'] = array(
    

    $form['no_match'] does not match const SETTING_NO_MATCH = 'no_match_value'. It is better to use constants as form keys here. So instead of $form['no_match'], use $form[self::SETTING_NO_MATCH]. That’s what the other Tamper plugins are doing as well.

  2. +++ b/src/Plugin/Tamper/ConvertBoolean.php
    @@ -0,0 +1,164 @@
    + //   $this->setConfiguration([self::SETTING_OPERATION => $form_state->getValue(self::SETTING_OPERATION)]);
    

    This line can be removed. There is no SETTING_OPERATION constant in this class.

  3. +++ b/src/Plugin/Tamper/ConvertBoolean.php
    @@ -0,0 +1,164 @@
    +                $this->getSetting(SETTING_NO_MATCH) = TRUE;
    ...
    +                $this->getSetting(SETTING_NO_MATCH) = FALSE;
    ...
    +                $this->getSetting(SETTING_NO_MATCH) = NULL;
    ...
    +                $this->getSetting(SETTING_NO_MATCH) = $this->getSetting(SETTING_OTHER_TEXT);
    

    I think here you forgot the 'self' keyword by referencing to the constants. So $this->getSetting(SETTING_NO_MATCH) should probably be $this->getSetting(self::SETTING_NO_MATCH).

There are some indentation issues as well. Would be cool to have a test for it. In some cases, a test can guide you where something is wrong. This is especially the case in unit tests.

irinaz’s picture

irinaz’s picture

Updated patch today - seems to be saving values from the form correctly, however, when I add TRUE/FALSE conversion to published field actual values do not seem to convert correctly.

irinaz’s picture

jamesdixon’s picture

It looks like the plugin and tests may actually be really good, but the latest patch didn't include the tests or the schema file you modified which is why the tests are failing.

How I roll my patches from within the modules/tamper folder is:

git add src
git add tests
git diff --staged > tamper-convertboolean-plugin-2962792-20.patch

If you have your test, plugin, and modified schema config yml file all ready to rock and do that it should include all files unless you've switched to another branch and converted or something like that.

irinaz’s picture

irinaz’s picture

jamesdixon’s picture

Hi @irinaz, I had a look at the latest one and realize I gave you bad advice. You also need to include config directory.

Here's updated instructions to include that tamper.schema.yml:

git add config
git add src
git add tests
git diff --staged > tamper-convertboolean-plugin-2962792-20.patch
irinaz’s picture

I am not sure how what needs to be included in schema, so here is first try with schema

jamesdixon’s picture

Ah okay I thought you set something up for tamper.schema.yml. You do something like this example:

tamper.find_replace:
  mapping:
    find:
      type: string
      label: 'Text to find'
    replace:
      type: string
      label: 'Text to replace'
    case_sensitive:
      type: boolean
      label: 'Case sensitive'
    word_boundaries:
      type: boolean
      label: 'Respect word boundaries'
    whole:
      type: boolean
      label: 'Match whole word/phrase'

But you change it to match convert boolean configuration variables at the top of the ConvertBoolean class. The top would be tamper.convert_boolean.

Your changes to tamper.schema.yml did not get added to the patch, when you do git status, does it show the file as modified?

irinaz’s picture

Here is what I added

tamper.convert_boolean:
  mapping:
    true:
      type: string
      label: 'True'
    false:
      type: string
      label: 'False'
    null:
      type: string
      label: 'Null'
    pass:
      type: string
      label: 'Do not modify'
    other:
      type: string
      label: 'Other'
jamesdixon’s picture

Cool, here's your patch rerolled. for git diff --staged to work you need to make sure all files that were modified had the git add command applied to them( ie: git add config/schema/tamper.schema.yml)

jamesdixon’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 26: tamper-convertboolean-plugin-2962792-28.diff, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

jamesdixon’s picture

Ah okay I see the keys for the schema mappings did not match the constants at the top of the class. I've updated these, and also updated the labels to match what was defined in the form.

Next we'll need to make a test inside of tamper/tests/src/Unit/Plugin/Tamper/ConvertBooleanTest.php

tamper/tests/src/Unit/Plugin/Tamper/ConvertCaseTest.php is a good example I use

Basically I change ConvertCase to ConvertBoolean, then update the tests to test what we find in the D7 feeds_tamper/tests/feeds_tamper_plugins.test. I can help you through it.

jamesdixon’s picture

Status: Needs work » Needs review
ericgsmith’s picture

Status: Needs review » Needs work
  1. +++ b/src/Plugin/Tamper/ConvertBoolean.php
    @@ -0,0 +1,166 @@
    +// use Drupal\tamper\Exception\TamperException;
    

    Remove commented out code.

  2. +++ b/src/Plugin/Tamper/ConvertBoolean.php
    @@ -0,0 +1,166 @@
    +    $config[self::SETTING_MATCH_CASE] = 'FALSE';
    

    Should this be a boolean rather than a string?

  3. +++ b/src/Plugin/Tamper/ConvertBoolean.php
    @@ -0,0 +1,166 @@
    +    $config[self::SETTING_NO_MATCH] = 'FALSE';
    

    Same here

  4. +++ b/src/Plugin/Tamper/ConvertBoolean.php
    @@ -0,0 +1,166 @@
    +      '#default_value' => ($this->getSetting(self::SETTING_TRUTH_VALUE)),
    

    Unsure if there was any in the original d7 plugin, but a description would be helpful here.

  5. +++ b/src/Plugin/Tamper/ConvertBoolean.php
    @@ -0,0 +1,166 @@
    +      '#default_value' => ($this->getSetting(self::SETTING_MATCH_CASE)),
    

    We don't need the extra parenthesis around all the default values

  6. +++ b/src/Plugin/Tamper/ConvertBoolean.php
    @@ -0,0 +1,166 @@
    +
    

    Extra line added here can be removed

  7. +++ b/src/Plugin/Tamper/ConvertBoolean.php
    @@ -0,0 +1,166 @@
    +      '#title' => t('If no match'),
    

    Should use $this->t('')

  8. +++ b/src/Plugin/Tamper/ConvertBoolean.php
    @@ -0,0 +1,166 @@
    +        'true' => ('True'),
    

    These should all use $this->t()

  9. +++ b/src/Plugin/Tamper/ConvertBoolean.php
    @@ -0,0 +1,166 @@
    +          'input[name="settings[no_match]"]' => array('value' => 'other'),
    

    Would be good to reuse the constant here and also short hand array syntax.

  10. +++ b/src/Plugin/Tamper/ConvertBoolean.php
    @@ -0,0 +1,166 @@
    +    /* todo:  check for conversion for matching case
    

    Is this comment still valid?

    If it is still todo this should be needs work instead of needs review

  11. +++ b/src/Plugin/Tamper/ConvertBoolean.php
    @@ -0,0 +1,166 @@
    +        $this->setConfiguration([self::SETTING_NO_MATCH => $this->getSetting(self::SETTING_OTHER_TEXT)]);
    

    Do we need this - can we just keep them separate?

  12. +++ b/src/Plugin/Tamper/ConvertBoolean.php
    @@ -0,0 +1,166 @@
    +      $data = 1;
    

    Shouldn't we be returning a boolean here?

jamesdixon’s picture

@irinaz: Let me know if you're still keen to wrap this one up, I can help out. :) If you'd rather focus on the feeds_migrate work the guys and I can take this one on no problem.

vijay.mayilsamy’s picture

Hi @irinaz,

I didn't know that you were working on this fixes. But made lot of the above requested changes last night and adding the patch of the same here. In case if you haven't started yet then you can continue from my patch attached.

Thanks
Vijay

jamesdixon’s picture

Status: Needs work » Needs review

Yes sorry we jumped the gun on making these updates. Lets check to see if the new tests pass.

Status: Needs review » Needs work

The last submitted patch, 33: convert_boolean-2962792-33.patch, failed testing. View results

irinaz’s picture

@vijay.mayilsamy, thanks a lot for doing this, I was swamped lately and have not done anything. thanks!

jamesdixon’s picture

Ah okay glad we did not duplicate work!

@Vijay: The tests did not pass on drupal.org for some reason. Let me know if you need help figuring this out.

jamesdixon’s picture

@vijay.mayilsamy This is failing for me in my local environment too not just drupal.org test bot.

I believe we can't use Unicode::strtolower in a test environment. Maybe try using just strtolower and see if that passes:

  public function tamper($data, TamperableItemInterface $item = NULL) {

    // Copy field value in case 'pass' is set.
    $match_field = $data;
    $data = $this->getSetting(self::SETTING_NO_MATCH);

    if (!$this->getSetting(self::SETTING_MATCH_CASE)) {
      $match_field = Unicode::strtolower($match_field);
    }   

vijay.mayilsamy’s picture

Hi James,

The test get's pass through if i add the switch case statement from "validateConfigurationForm" to tamper method.
So, the conditional settings coded in "validateConfigurationForm" is not getting applied for the tests.

I haven't removed the switch statement code from "validateConfigurationForm" method yet. If the right approach is to add the code in tamper method then we can remove it from validateConfigurationForm. Let me know your comments.

Thanks
Vijay

jamesdixon’s picture

Hi Vijay,

What I've been doing instead is actually changing the $config object in the Test to hold those values which are calculated in the validateConfigurationForm() method. validateConfigurationForm() does not get called during a test. Putting it into tamper() is not quite right I don't think. Please update those $config arrays to hold the value that's getting calculated in validateConfigurationForm()

vijay.mayilsamy’s picture

Hi @James,

Thanks for your advice. updated the config variables to match with ValidateFormConfiguration settings. Here is the updated patch for review.

Regards
Vijay

vijay.mayilsamy’s picture

Status: Needs work » Needs review
ericgsmith’s picture

Status: Needs review » Needs work
FileSize
4.09 KB
10.07 KB

Hi Vijay,

Thanks for picking this up. I've taken a look and we still have a bit to go on this plugin - for me it doesn't look like the tests we accurately showing the expected results of the plugin - as there is an error in the return logic that means we were only ever getting the no match value back unless it was 'pass'.

I've updated just the tests to show this issue, the tests will intentionally fail should pass once the tamper method if fixed.

Also a few comments below - I'll take a closer look once the tests are passing again.

I think there also seems to be a bit of confusion on the default settings and how the plugin form is configured. I would prefer the default settings reflect what is needed for the plugin to function, not for the form. I.e, the form is just one way of configuring a plugin. If we can correctly map the config to the form if the values differ, that would be better.

  1. +++ b/src/Plugin/Tamper/ConvertBoolean.php
    @@ -0,0 +1,166 @@
    + * Plugin implementation for converting case.
    

    Class description needs updating.

  2. +++ b/src/Plugin/Tamper/ConvertBoolean.php
    @@ -0,0 +1,166 @@
    +    $config[self::SETTING_MATCH_CASE] = (bool) FALSE;
    

    This is a boolean, no need for casting here.

  3. +++ b/src/Plugin/Tamper/ConvertBoolean.php
    @@ -0,0 +1,166 @@
    +    $config[self::SETTING_NO_MATCH] = 'FALSE';
    

    Should be the same format as the options key, so that we can do strict comparision. E.g 'false'

  4. +++ b/src/Plugin/Tamper/ConvertBoolean.php
    @@ -0,0 +1,166 @@
    +          'input[name="settings[no_match]"]' => ['value' => 'other'],
    

    Has this been tested with feeds ui? I'm not sure where the name settings comes from - I wonder if this will work if the plugin is used nested in other structures. We should also use the constant for no_match

  5. +++ b/src/Plugin/Tamper/ConvertBoolean.php
    @@ -0,0 +1,166 @@
    +  public function validateConfigurationForm(array &$form, FormStateInterface $form_state) {
    

    I don't think this is really validation logic - it looks like it should be part of the submit handler.

vijay.mayilsamy’s picture

Updated the description and constact for SETTING_OTHER_TEXT. Still needs lot of work to be done here.

@James, Need your help on this one

mitrpaka’s picture

Status: Needs work » Needs review
FileSize
9.83 KB
3.84 KB

Updated patch with updated test from #43.

mitrpaka’s picture

Fixed form #states input name.

jamesdixon’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
17.31 KB

These updates look good @mitrpaka! Code looks solid.

@ericgsmith: I can see the updates you requested in #43 were made.

Also that visibility selector works. When you click the "Other" option the "Other text" field appears:

demonde’s picture

Works for me. Can it become contributed?

  • jamesdixon committed 4d35e6d on 8.x-1.x authored by mitrpaka
    Issue #2962792 by irinaz, vijay.mayilsamy, jamesdixon, mitrpaka,...
jamesdixon’s picture

Category: Bug report » Task
Status: Reviewed & tested by the community » Fixed

This has been commited to 8.x-1.x.

Thanks everyone!

jamesdixon’s picture

Status: Fixed » Needs review
FileSize
627 bytes

Sorry there was a one liner issue in the old test. Testing here.

Status: Needs review » Needs work

The last submitted patch, 51: tamper-convertboolean-plugin-2962792-51.patch, failed testing. View results

jamesdixon’s picture

Status: Needs work » Needs review
FileSize
3.07 KB

Trying again with convert to boolean and date time fixes so tests will pass.

jamesdixon’s picture

Status: Needs review » Fixed

Tests fixed now.

Status: Fixed » Closed (fixed)

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