Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
We need to port the feeds tamper trim plugin into a tamper plugin for Drupal 8.
Comment | File | Size | Author |
---|---|---|---|
#53 | tamper-convertboolean-plugin-2962792-53.patch | 3.07 KB | jamesdixon |
| |||
#51 | tamper-convertboolean-plugin-2962792-51.patch | 627 bytes | jamesdixon |
#47 | other-text-works.png | 17.31 KB | jamesdixon |
#46 | interdiff-45-46.txt | 512 bytes | mitrpaka |
#46 | convert-boolean-2962792-46.patch | 9.83 KB | mitrpaka |
|
Comments
Comment #2
irinaz CreditAttribution: irinaz as a volunteer and at Fibonacci Web Studio commentedComment #3
irinaz CreditAttribution: irinaz as a volunteer and at Fibonacci Web Studio commentedComment #4
jamesdixon CreditAttribution: jamesdixon commentedIt 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:
In defaultConfiguration(), you should instead define defaults for all the constants you have defined above:
Great start!
Comment #5
irinaz CreditAttribution: irinaz as a volunteer and at Fibonacci Web Studio commented@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.
Comment #6
irinaz CreditAttribution: irinaz as a volunteer and at Fibonacci Web Studio commentedComment #7
MegaChriz CreditAttribution: MegaChriz as a volunteer commented@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:
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)cd tamper
.git checkout -b convert_boolean.2962792.8
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.)
Comment #8
irinaz CreditAttribution: irinaz as a volunteer and at Fibonacci Web Studio commented@MegaChriz, thanks a lot! I will update Tamper plugin and submit new patch.
Comment #9
irinaz CreditAttribution: irinaz as a volunteer and at Fibonacci Web Studio commentedI 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);
Comment #10
irinaz CreditAttribution: irinaz as a volunteer and at Fibonacci Web Studio commentedComment #11
irinaz CreditAttribution: irinaz as a volunteer and at Fibonacci Web Studio commentedComment #12
jamesdixon CreditAttribution: jamesdixon commentedThe 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:
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:
Basically you're using $data instead of field, and grabbing $settings from $this->getSetting();
Comment #13
jamesdixon CreditAttribution: jamesdixon commentedHere is the latest patch by @irinaz. Still running into some PHP errors but getting much closer!
Comment #14
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedMore or less copied from the #feeds channel in Slack:
$form['no_match']
does not matchconst 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.This line can be removed. There is no SETTING_OPERATION constant in this class.
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.
Comment #15
irinaz CreditAttribution: irinaz as a volunteer and at Fibonacci Web Studio commentedUpdated patch based on comments - thanks!!
Comment #16
irinaz CreditAttribution: irinaz as a volunteer and at Fibonacci Web Studio commentedUpdated 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.
Comment #17
irinaz CreditAttribution: irinaz as a volunteer and at Fibonacci Web Studio commentedstyle updates
Comment #18
irinaz CreditAttribution: irinaz as a volunteer and at Fibonacci Web Studio commentedtamper-convertboolean-plugin-2962792-18
Comment #19
jamesdixon CreditAttribution: jamesdixon at Dialed In Design commentedIt 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:
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.
Comment #20
irinaz CreditAttribution: irinaz as a volunteer and at Fibonacci Web Studio commented@jamesdixon - thanks! testing update patch
Comment #21
irinaz CreditAttribution: irinaz as a volunteer and at Fibonacci Web Studio commentedcorrect patch
Comment #22
jamesdixon CreditAttribution: jamesdixon at Dialed In Design commentedHi @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:
Comment #23
irinaz CreditAttribution: irinaz as a volunteer and at Fibonacci Web Studio commentedI am not sure how what needs to be included in schema, so here is first try with schema
Comment #24
jamesdixon CreditAttribution: jamesdixon at Dialed In Design commentedAh okay I thought you set something up for tamper.schema.yml. You do something like this example:
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?
Comment #25
irinaz CreditAttribution: irinaz as a volunteer and at Fibonacci Web Studio commentedHere is what I added
Comment #26
jamesdixon CreditAttribution: jamesdixon at Dialed In Design commentedCool, 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)
Comment #27
jamesdixon CreditAttribution: jamesdixon at Dialed In Design commentedComment #29
jamesdixon CreditAttribution: jamesdixon at Dialed In Design commentedAh 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.
Comment #30
jamesdixon CreditAttribution: jamesdixon at Dialed In Design commentedComment #31
ericgsmith CreditAttribution: ericgsmith as a volunteer commentedRemove commented out code.
Should this be a boolean rather than a string?
Same here
Unsure if there was any in the original d7 plugin, but a description would be helpful here.
We don't need the extra parenthesis around all the default values
Extra line added here can be removed
Should use $this->t('')
These should all use $this->t()
Would be good to reuse the constant here and also short hand array syntax.
Is this comment still valid?
If it is still todo this should be needs work instead of needs review
Do we need this - can we just keep them separate?
Shouldn't we be returning a boolean here?
Comment #32
jamesdixon CreditAttribution: jamesdixon at Dialed In Design commented@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.
Comment #33
vijay.mayilsamy CreditAttribution: vijay.mayilsamy as a volunteer and at Dialed In Design commentedHi @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
Comment #34
jamesdixon CreditAttribution: jamesdixon at Dialed In Design commentedYes sorry we jumped the gun on making these updates. Lets check to see if the new tests pass.
Comment #36
irinaz CreditAttribution: irinaz as a volunteer and at Fibonacci Web Studio commented@vijay.mayilsamy, thanks a lot for doing this, I was swamped lately and have not done anything. thanks!
Comment #37
jamesdixon CreditAttribution: jamesdixon as a volunteer and at Dialed In Design commentedAh 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.
Comment #38
jamesdixon CreditAttribution: jamesdixon as a volunteer and at Dialed In Design commented@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:
Comment #39
vijay.mayilsamy CreditAttribution: vijay.mayilsamy as a volunteer and at Dialed In Design commentedHi 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
Comment #40
jamesdixon CreditAttribution: jamesdixon as a volunteer and at Dialed In Design commentedHi 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()
Comment #41
vijay.mayilsamy CreditAttribution: vijay.mayilsamy as a volunteer and at Dialed In Design commentedHi @James,
Thanks for your advice. updated the config variables to match with ValidateFormConfiguration settings. Here is the updated patch for review.
Regards
Vijay
Comment #42
vijay.mayilsamy CreditAttribution: vijay.mayilsamy as a volunteer and at Dialed In Design commentedComment #43
ericgsmith CreditAttribution: ericgsmith as a volunteer commentedHi 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.
Class description needs updating.
This is a boolean, no need for casting here.
Should be the same format as the options key, so that we can do strict comparision. E.g 'false'
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
I don't think this is really validation logic - it looks like it should be part of the submit handler.
Comment #44
vijay.mayilsamy CreditAttribution: vijay.mayilsamy as a volunteer and at Dialed In Design commentedUpdated the description and constact for SETTING_OTHER_TEXT. Still needs lot of work to be done here.
@James, Need your help on this one
Comment #45
mitrpaka CreditAttribution: mitrpaka as a volunteer commentedUpdated patch with updated test from #43.
Comment #46
mitrpaka CreditAttribution: mitrpaka as a volunteer commentedFixed form #states input name.
Comment #47
jamesdixon CreditAttribution: jamesdixon as a volunteer and at Dialed In Design commentedThese 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:
Comment #48
demonde CreditAttribution: demonde commentedWorks for me. Can it become contributed?
Comment #50
jamesdixon CreditAttribution: jamesdixon as a volunteer and at Dialed In Design commentedThis has been commited to 8.x-1.x.
Thanks everyone!
Comment #51
jamesdixon CreditAttribution: jamesdixon as a volunteer and at Dialed In Design commentedSorry there was a one liner issue in the old test. Testing here.
Comment #53
jamesdixon CreditAttribution: jamesdixon as a volunteer and at Dialed In Design commentedTrying again with convert to boolean and date time fixes so tests will pass.
Comment #54
jamesdixon CreditAttribution: jamesdixon as a volunteer and at Dialed In Design commentedTests fixed now.