Make process steps plugins to ease maintenance and extension

Thank you for identifying this and putting the work in to get the fix moving! It's been a pain point for a while I've wanted to address, and there's actually some work refactoring the whole module to be more plugin-based in the 8.x-2.x branch, but that's stagnated for a while now.

I look forward to digging into this and getting you some feedback on it.

This work looks great! The last patch from #5 appears to be empty, but reviewing the work from the initial patch looks great!

Just reading through the patch I'm noting things as I find them:

1. +++ b/src/ContentLoader/ContentLoader.php
   @@ -157,6 +163,22 @@ class ContentLoader implements ContentLoaderInterface {
   +  /**
   +   * Get the EntityLoadHelper service.
   +   *
   +   * @return \Drupal\yaml_content\Plugin\YamlContentProcessManager
   +   *   The EntityLoadHelper service.
   +   */
   +  protected function getProcessManager() {
   +    // Lazy load the entity load helper service.
   +    if (!isset($this->processManager)) {
   +      $this->processManager = $this->container
   +        ->get('plugin.manager.yaml_content.process');
   +    }
   +
   +    return $this->processManager;
   +  }
   

   The docblock for this needs to be updated to reflect the correct return value.

2. +++ b/src/ContentLoader/ContentLoader.php
   @@ -497,8 +526,15 @@ class ContentLoader implements ContentLoaderInterface {
   +      $context = new ProcessingContext();
   +      $context->setField($field);
   +      $context->setContentLoader($this);
   

   This processing context is something we've been needing for a while and can definitely help with some problems in the event dispatching as well.

3. +++ b/src/Plugin/YamlContentProcessBase.php
   @@ -0,0 +1,35 @@
   +class YamlContentProcessBase extends PluginBase {
   

   This should likely be made abstract so it can implement the YamlContentProcessInterface as well.

4. +++ b/src/Plugin/YamlContentProcessInterface.php
   @@ -0,0 +1,25 @@
   +  /**
   +   * Processor function for processing and loading a file attachment.
   +   *
   +   * @param \Drupal\yaml_content\Plugin\ProcessingContext $context
   +   *   The processing context.
   +   * @param array $field_data
   +   *   The field data.
   +   *
   +   * @return array|int
   +   *   The entity id.
   +   *
   +   * @throws \Drupal\Core\TypedData\Exception\MissingDataException
   +   *   Error for missing data.
   +   *
   +   * @see \Drupal\yaml_content\Service\ProcessHelper::preprocessFieldData()
   +   */
   +  public function process(ProcessingContext $context, array &$field_data);
   

   This docblock needs to be generalized to be accurate here.

5. +++ b/src/Plugin/YamlContentProcessManager.php
   @@ -0,0 +1,91 @@
   +   * Constructs a AggregatorPluginManager object.
   

   Docblock needs updating.

6. +++ b/src/Plugin/YamlContentProcessManager.php
   @@ -0,0 +1,91 @@
   +   * Preprocessors are expected to be provided in the following format:
   

   This whole docblock needs to be updated at some point to reflect the current or new structure.

7. +++ b/src/Plugin/YamlContentProcessManager.php
   @@ -0,0 +1,91 @@
   +    // Check for a callback processor defined at the value level.
   +    if (isset($field_data['#process']['callback'])) {
   +      $plugin_id = $field_data['#process']['callback'];
   

   For clarity we'll probably want to rename this key, but we'll need to support it for backward compatibility as well. Maybe check for either "plugin" or "callback"?

8. +++ b/src/Plugin/YamlContentProcessManager.php
   @@ -0,0 +1,91 @@
   +      // If the plugin does not exist, throw an exception.
   +      else {
   +        throw new ConfigValueException('Unknown type specified: ' . $plugin_id);
   +      }
   

   We should probably customize this to make it easier to handle the exceptions.

9. +++ b/src/Plugin/yaml_content/process/File.php
   @@ -0,0 +1,64 @@
   +    $entity_type = $this->configuration[0];
   +    $filter_params = $this->configuration[1];
   

   I like this solution for how you've passed in the configuration from the content data as actual plugin configuration.

Nice work on this! Aside from some of the revisions I've noted above, I want to test this against some existing codebases and working content files to make sure we're not introducing any regressions. From there we'll want to create some documentation for how to create these plugins as well, but that won't need to hold up getting this merged in.

Thanks for all the hard work!

Thanks for the quick turnaround on the feedback and revisions!

I'll loop back and address the other comments, but tonight I ran through and applied the coding style updates from code sniffer.

2. Oh, we are so on the same page. That makes me very happy. I actually was playing with using it in events and it was great but it was getting really invasive and breaking BC so I set it aside but would love to see how it works out.

The backward compatibility concern is something I've still got to face in refactoring the events, but I think this could help a great deal with some of the issues I've faced in #2949902: Consolidate Event Dispatching Into Consistent Pattern.

6 & 7. So I'd kept all this as is so all the docs both docblock and handbook, didn't have to be touched. Plugins are more about supporting code rather then defining things like the structure of your YAML, etc so that's technically fine as it is. If you want I'll happily change the code though if you want.

Ya, sorry the docblock note was more of an observation as I read through the code again and saw that the notes there had become outdated again. I think before making this change it would be valuable to go ahead and refresh that to reflect the current status of it since we're touching that code again. In that same vein I think introducing a more intuitive keyword to replace "callback" would be a good step forward, and we can deprecate support for "callback" to remove it in the mythical 1.0 release.

Regarding the goal of plugins to add supporting code I think that's fair and we can define the generic structure for using these plugins in the plugin manager while just documenting specific arguments that might be expected in the separate plugin doc blocks.

8. Yeah, that's copy/pasted from the old implementation trying to limit how much I disturbed things. You are right that isn't the best exception though. If we just call createInstance() without checking its going to throw PluginNotFoundException which may be good enough for our needs. We could also override the createInstance method and have it throw a specific yaml content exception as well.

Since that's leftover from the current implementation let's go ahead and leave it for now. Soon I'd like to tackle exception handling more directly overall to provide some more helpful tools for writing and debugging the content import files anyway, and determining the best solution for this would be better addressed in that context.

The patch in #11 works really well. I did have a bit of confusion with the return value from the plugin, though. It's supposed to be an entity ID but doesn't look like it's actually used for anything. In my case, I didn't have an entity and just returned a string for the menu link UUID and that worked fine. I don't think it really needs to return anything at all except the docblock was expecting it. Maybe look at just getting rid of that if it's not being used?

I've pulled in the latest updates in the 8.x-1.x branch and rerolled the patch. The change is pretty minimal and unrelated to this issue.

The patch in #11 works really well. I did have a bit of confusion with the return value from the plugin, though. It's supposed to be an entity ID but doesn't look like it's actually used for anything.

@Michelle I don't think it's currently being used for anything, but some of the added reporting to better track individual entities being created will leverage it, so I'd like to keep it in there.

I'm marking this issue for inclusion in the alpha5 release (#2983642: Release Plan: 8.x-1.x-alpha5). It's been in use for a few months on one of my projects without any issue, and before merging it in I'll be testing builds with it on a couple of other projects I have access to.

I've confirmed this build successfully and as expected using 2 separate projects with complex data being imported. I've merged it into the dev branch for inclusion in the alpha5 release under #2983642: Release Plan: 8.x-1.x-alpha5.