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 |
---|---|---|---|
#7 | interdiff-2960790-6-7.txt | 2.56 KB | ericgsmith |
#7 | tamper-trim-plugin-2960790-7.patch | 4.66 KB | ericgsmith |
| |||
#6 | interdiff_2-6.txt | 865 bytes | jamesdixon |
#6 | tamper-trim-plugin-2960790-6.patch | 4.6 KB | jamesdixon |
| |||
#3 | tamper-trim-plugin-2960790-2.patch | 4.57 KB | jamesdixon |
|
Comments
Comment #2
jamesdixon CreditAttribution: jamesdixon commentedSo I took a crack at the trim plugin and tests. The trouble I'm having is when I try to run the tests, it's not generating the test xml in /sites/default/files/simpletest and I'm getting some error in the watchdog. I'm new to writing tests in D8.
I'm sure I've made a mistake in my code somewhere, or need to clear the testing cache or something. Any suggestions on getting around this would be greatly appreciated.
Comment #3
jamesdixon CreditAttribution: jamesdixon commentedSorry I have my environment setup where php errors aren't being logged to watchdog properly. Thanks for the help with this one megachriz.
Please find the revised patch which passes all 4 tests!
Comment #4
jamesdixon CreditAttribution: jamesdixon commentedComment #5
ericgsmith CreditAttribution: ericgsmith as a volunteer commentedHi James,
Thanks for your contribution :) It's awesome to see the patches starting to roll in.
Just a couple minor issues for me then I think were good to go with this.
Doc comment short description must be on a single line, further text should be a separate paragraph.
This should be a string value. This is the config key, so should reflect what the configuration is storing. It looks like maybe this is being confused with the default config.
As above, something like 'side' would make more sense here.
Nitpick - theres an extra line here to remove.
Nitpick - else statements should be on a new line.
The whole expression could also be simplified using a shorthand if statement.
Comment #6
jamesdixon CreditAttribution: jamesdixon commentedThanks @ericgsmith!
I made all your suggestions besides using a shorthand if. Wasn't sure how to fit that all on one line within 80 chars, or how to format that properly if using shorthand with multiple lines.
Comment #7
ericgsmith CreditAttribution: ericgsmith as a volunteer commentedThanks again James - just updating the test against the new base class.
Comment #9
ericgsmith CreditAttribution: ericgsmith as a volunteer commented