The FeedTypeTamperMeta code makes calls to $tamper->getSetting(KEY) in the code, e.g. removeTamper(), updateTamper(), and getTampersGroupedBySource(). However, the Tamper module's plug-ins (and interface) do not have this method define (or required).
This is a critical bug because you can't remove or update Tampers once added.
| Comment | File | Size | Author |
|---|---|---|---|
| #14 | interdiff-2946698-12-14.txt | 3.4 KB | megachriz |
| #14 | feeds_tamper-tamper-meta-2946698-14.patch | 15.02 KB | megachriz |
| #12 | interdiff-2946698-07-12.txt | 945 bytes | megachriz |
| #12 | feeds_tamper-tamper-meta-2946698-12.patch | 13.87 KB | megachriz |
| #7 | feeds_tamper-tamper-meta-2946698-7.patch | 12.96 KB | megachriz |
Comments
Comment #2
megachrizYou are right. Only the class \Drupal\tamper\ConfigurableTamperBase has this method defined.
Comment #3
megachrizThis expands the tests for the FeedTypeTamperMeta class and also changes it's API a bit to be more like how \Drupal\filter\Entity\FilterFormat handles plugins.
I do think that the TamperBase class should have a
getSetting()method. Feeds Tamper need to be able to store it's configuration on a Tamper plugin instance. While the configuration can get stored there, it cannot be retrieved by Tamper plugins directly extending TamperBase.Comment #5
ericgsmith commentedI've opened a related ticket Make all plugins configurable to make all plugins configurable by default.
I don't see a downside to this approach and will make things a bit more consistent. The feeds UI currently checks for an instance of ConfigurableTamperInterface which would need to be removed before we can remove the deprecated code in tamper if we are happy with the related ticket.
Comment #6
megachrizAnticipating on the patch in #2948543-3: Make all plugins configurable. Removes references to ConfigurableTamperInterface.
We need UI tests for adding and editing tamper instances. All Feeds Tamper's form code is not covered by any tests.
Comment #7
megachrizReroll (needed since #2948850: Add an availability to remove tampers from sources is committed).
Comment #9
ericgsmith commentedTest runs fine locally with the latest patch in https://www.drupal.org/project/tamper/issues/2948543#comment-12522953
@MegaChriz are UI tests blocking this issue or are you happy for me to merge the patch in the tamper module?
Comment #11
megachrizUI tests are not blocking this issue. They would be a nice to have to ensure that this patch doesn't break anything, but should it break something then that can also be fixed once the tests are implemented.
I'm happy to have the patch in the Tamper module merged.
Comment #12
megachrizReroll (needed after the changes in #2937728: Add an UI) + small change.
Comment #14
megachrizShortened
\Drupal\Tests\feeds_tamper\Unit::testSetTamperConfig()as what it tested first only works during a Kernel test. Also a few coding standard fixes.Comment #16
megachrizCommitted #14.