Following on from Call to undefined method getSetting() in all tampers it would be useful to allow all tamper plugins to hold configuration.

Proposed solution is to move the functionality from ConfigurableTamperBase / ConfigurableTamperInterface to the base plugin. This will allow more consistent use of the plugins by third party modules, and allows modules like feeds tamper to extend the configuration / access it in a more consistent manor.

ConfigurableTamperInterface will be deprecated and can be safely removed once the checks in feeds tamper UI are removed.

CommentFileSizeAuthor
#5 tamper-make-all-plugins-configurable-2948543-5.patch7.9 KBericgsmith
#5 interdiff-294853-4-5.txt890 bytesericgsmith
#4 tamper-make-all-plugins-configurable-2948543-4.patch7.84 KBericgsmith
#3 interdiff-2948543-2-3.txt2.96 KBmegachriz
#3 tamper-make-all-plugins-configurable-2948543-3.patch8.18 KBmegachriz
#2 tamper-make-all-plugins-configurable-2948543-2.patch7.29 KBericgsmith

Comments

ericgsmith created an issue. See original summary.

ericgsmith’s picture

ericgsmith commented
Status: Active » Needs review
StatusFileSize
new tamper-make-all-plugins-configurable-2948543-2.patch7.29 KB
megachriz’s picture

megachriz
Primary language Dutch
commented
StatusFileSize
new tamper-make-all-plugins-configurable-2948543-3.patch8.18 KB
new interdiff-2948543-2-3.txt2.96 KB

This goes one step further by doing also the following:

  1. Removes $uuid and $weight from TamperBase and their accompanying methods. In my opinion these implementation details can be different per module that would make use of Tamper plugins. Feeds Tamper currently passes the uuid as part of the configuration array so it never lands on the $uuid property.
  2. Defines the method getSetting() in TamperInterface. I understood from a comment by either @bojanz or @mglaman somewhere in the Commerce issue queue that it is a good practice to have all public methods defined in an interface as well. In #2946698: Call to undefined method getSetting() in all tampers I'm mocking TamperInterface and I'm planning to mock the getSetting() method in a test.
  3. Removing the ConfigurableTamperInterface right away. This would break Feeds Tamper indeed, but I suppose we should fix that together with the other changes in #2946698: Call to undefined method getSetting() in all tampers .
ericgsmith’s picture

ericgsmith commented
StatusFileSize
new tamper-make-all-plugins-configurable-2948543-4.patch7.84 KB

Thanks, sounds good to me.

Rerolling against changes dev.

ericgsmith’s picture

ericgsmith commented
StatusFileSize
new interdiff-294853-4-5.txt890 bytes
new tamper-make-all-plugins-configurable-2948543-5.patch7.9 KB

Just a small change, if we are exposing get setting to the world we need to check that whatever value is request actually exists in the config array.

megachriz’s picture

megachriz
Primary language Dutch
commented

Good idea to return NULL in case the setting does not exist.

I was also thinking of adding a method called setSetting() as the counterpart for the getSetting() method. Could also be handled in a follow-up though. I think that Feeds Tamper won't need it. Feeds Tamper would just use setInstanceConfiguration().

ericgsmith’s picture

ericgsmith commented
Status: Needs review » Fixed

Committed

  • ericgsmith committed c555603 on 8.x-1.x 
    Issue #2948543 by ericgsmith, MegaChriz: Make all plugins configurable

Status: Fixed » Closed (fixed)

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