Plugins that depend on a tamperable item need to know the available properties

Forgive my beginner's attempt at understanding here. :)

Right so if some plugin wants to use tamper to modify sources of an item/row, right now the developer of the plugin would need to write their own custom code or hard code the sources into a UI (if they need a UI). This is because we are not processing items during configuration.

If that's the case I agree setting something like this up would be useful.

$this->feedsFeedType->someMethodToGetSources();

Could be used to grab the sources outside of the actual processing step from external plugins.

These external plugins could use set and get temporary value to store the sources for use outside of processing.

Is this what you mean?

Ah okay totally makes sense.

Do you think

$this->feedsFeedType->someMethodToGetSources();

Should return the sources in a format a form '#options' would understand, so they could simply drop the result into a 'radios' or 'select' form? Is selecting sources from these form types the main use case, or do you see any other ways people would need to access the sources from a plugin?

I see these possible options to make passing sources more obvious:

1. Maybe throw an exception if 'available_source_properties' is not set on $form_state? Maybe throw that exception even on TamperBase so that the implementer finds out much quicker that something is missing and not until he/she just happens to test with the "copy" plugin?
2. When constructing a Tamper plugin, require a list of sources to be passed, maybe as part of $configuration (or $plugin_definition?). Con: tamper plugins that are not extending TamperBase would receive the list of sources as well and may not do anything with it. Another con: the list of sources shouldn't be saved when storing the Tamper plugin instance.
3. Make Tamper plugins context aware as you proposed in #2937717-2: Add support for manipulating a complete 'feeds item'/'row'., but that made things more complicated.
4. I suppose adding an extra parameter to buildConfigurationForm() to pass in additional context wouldn't work as you then break the interface?

An other idea, similar to idea 2 from #9:

1. Add an extra parameter to the constructor of TamperBase.
2. Override the default factory (\Drupal\Core\Plugin\Factory\ContainerFactory) with a Tamper factory class.
3. In TamperFactory, override the createInstance() method. Inside $configuration, expect 'available_source_properties' to be set (else throw an exception).
4. Pass 'available_source_properties' separately to the Tamper plugin and unset it from $configuration before instantiating the plugin.

I did something alike in a project where the base plugin class did not extend \Drupal\Component\Plugin\PluginBase at all and had a complete different set of constructor parameters.
https://github.com/MegaChriz/afasprofit/blob/3.x/src/Core/Entity/EntityF...

It may not be the best approach, but the advantage is that Tamper plugins then would have access to the available sources list outside of buildConfigurationForm() as well.

A type of plugin that uses an extra parameter in the constructor is "CommerceCheckoutPane".

• CheckoutPaneBase asks in its constructor and also in its ::create() method for a CheckoutFlowInterface. For ::create() the parameter is defined optional in order to comply to the interface \Drupal\Core\Plugin\ContainerFactoryPluginInterface.
• The plugin manager for checkout panes also defines an extra parameter for createInstance(), asking for CheckoutFlowInterface. Again optional, in order to be compatible with \Drupal\Component\Plugin\PluginManagerBase and \Drupal\Component\Plugin\Factory\FactoryInterface..
• \Drupal\commerce_checkout\Plugin\Commerce\CheckoutFlow::getPanes() instantiates plugins of type "CommerceCheckoutPane".

The plugin type "FieldFormatter" uses a slightly different approach.

• FormatterBase::__contruct() requires a instance of FieldDefinitionInterface.
• FormatterPluginManager::createInstance() expects a FieldDefinitionInterface as part of the configuration, defined with the key 'field_definition'.
• The instance is created in FormatterPluginManager::getInstance(), however the options array also expects to receive a FieldDefinitionInterface.
• EntityViewDisplay::getRenderer() finally passes this FieldDefinitionInterface to the plugin manager.

My point is that I think it is a approach that is proven to work.

So I think we should come up with a SourceDefinitionInterface and a SourceDefinition object that consumers can use to pass to the Tamper plugin.

This is how a SourceDefinition implementation with Tamper would look like.

I plan on reviewing this in the week coming up.

Seems like the approach in #14 would work.

Would it be possible to make the source definition optional for plugins which aren't using the source definitions in any way?

Or is the idea we always need the source definition so other modules working with the tamper plugins have an idea of the sources for form building etc?

The idea is that other modules always need to pass a source definition, because they won't know beforehand whether or not a Tamper plugin requires that information.

Attached three patches:

1. One that builds on top of the patch in #14.
2. One that is a port of the Rewrite plugin, so you can see how the SourceDefinition could actively be used.
3. And one that shows what changes in Feeds Tamper would be required.

Last week @andileco asked during the Feeds meetup what work is left for this issue.

Basically, the patch in #17 looks good to go. The reason that I didn't commit the patch yet is:

• I'm not 100% sure if it's the best approach, but it was the best I currently could come up with. I think it could possibly cause some WTF's by developers trying to integrate Tamper in their modules. Because it adds some extra complexity in order to successfully integrate Tamper. Modules are required to instantiate and pass a SourceDefinition.
• When I post a non-trivial patch for a module that I (co-)maintain, I usually leave it there for a while so I can look at it later with fresh eyes. I didn't come back to this patch yet because I had set other priorities.

I tried hard to think about a better approach, but my mind was going in circles there. Maybe we should accept the less perfect solution, so we can move forward. And perhaps I'm wrong about fearing that my solution makes integrating Tamper too hard.

I added the rewrite patch that's part of #17 to the appropriate issue: https://www.drupal.org/project/tamper/issues/2976180

I've taken a look at my code in #17 and investigated parts I didn't like:

• The many occurrence of unset($configuration['source_definition']);
• That Feeds Tamper needed to do the following:
  

  // Add in source definition for each tamper.
  foreach ($tampers as &$tamper) {
    $tamper['source_definition'] = $this->getSourceDefinition();
  }

• That it was passing the source definition as an extra parameter to a plugin's create() method:
  

  return $plugin_class::create(\Drupal::getContainer(), $configuration, $plugin_id, $plugin_definition, $source_definition);
  

  The CountryToCode plugin then had to define that extra parameter as an optional one in order to still comply with the interface "ContainerFactoryPluginInterface":

  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition, SourceDefinitionInterface $source_definition = NULL) {
  

Field formatter plugins in Drupal Core approach

So I've taken another close look to how Drupal Core has organized code for field formatter plugins. FormatterBase requires an instance of FieldDefinitionInterface, so its situation is in that regard similar to the approach I had taken.

I see that TimestampFormatter for example just expects that the field definition is passed as part of $configuration:

/**
 * {@inheritdoc}
 */
public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
  return new static(
    $plugin_id,
    $plugin_definition,
    $configuration['field_definition'],
    $configuration['settings'],
    $configuration['label'],
    $configuration['view_mode'],
    $configuration['third_party_settings'],
    $container->get('date.formatter'),
    $container->get('entity.manager')->getStorage('date_format')
  );
}

So in TamperManager, passing the source definition as part of $configuration sounds like a better approach to me. And it eliminates one occurrence of unset($configuration['source_definition']); as well.

TamperPluginCollection

I already left a @todo in that class to investigate if an instance of it should require a source definition. I tried how that worked out and I think this made the code in Feeds Tamper a bit cleaner:

$this->tamperCollection = new TamperPluginCollection($this->tamperManager, $this->getSourceDefinition(), $tampers);

And guess what, it eliminated another occurrence of unset($configuration['source_definition']); ;).

So that concludes my motivation for the changes I had made in this new patch. It might not solve the (potential) issue that it will get harder to integrate Tamper in other systems, but I think that the implementation is much cleaner. Now I hope it will pass tests. At least all Feeds Tamper tests pass locally.

Ah, I forgot to update SprintfTest and I missed updating TamperConfigSchemaTest as well.

New patch. Also fixes some coding standard issues.

Hehe, referenced the wrong thing in TamperConfigSchemaTest.

Hide some files, so the most useful patches show up on the top of this issue.

Committed #26, with some slight changes in the code comments.

Now onto the rewrite plugin in #2976180: Rewrite Plugin! And adapting Feeds Tamper of course.

And opened follow-up for Feeds Tamper: #3061554: Pass new SourceDefinition to Tamper plugins.