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.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
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedYou are right. Only the class \Drupal\tamper\ConfigurableTamperBase has this method defined.
Comment #3
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedThis 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 CreditAttribution: ericgsmith as a volunteer 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
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedAnticipating 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
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedReroll (needed since #2948850: Add an availability to remove tampers from sources is committed).
Comment #9
ericgsmith CreditAttribution: ericgsmith as a volunteer 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
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedUI 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
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedReroll (needed after the changes in #2937728: Add an UI) + small change.
Comment #14
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedShortened
\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
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedCommitted #14.