Create field collections source migrate plugin

Seems like a duplicate of #2897021: [META] Migrate support for importing field collections as paragraphs and more specifically #2911241: Migrate field collections types as paragraph types

Lots of small nits. This is looking really good.

1. similarity index 100%
   rename from migrations/d7_paragraph_type.yml
   
   rename from migrations/d7_paragraph_type.yml
   rename to migrations/d7_paragraphs_type.yml
   

   Renaming is fine as these are just place in time copies. No BC layer needed.

2. +++ b/src/Plugin/migrate/source/d7/FieldCollectionItem.php
   @@ -0,0 +1,88 @@
   +    // Select node in its last revision.
   

   Copy/pasta. Probably can be removed.

3. +++ b/src/Plugin/migrate/source/d7/FieldCollectionItem.php
   @@ -0,0 +1,88 @@
   +    if (isset($this->configuration['field_name'])) {
   +      $query->condition('f.field_name', $this->configuration['field_name']);
   

   It would be super helpful if we could add some doxygen comment to the class explaining what this config option is all about. It isn't just clear.

4. +++ b/src/Plugin/migrate/source/d7/FieldCollectionItem.php
   @@ -0,0 +1,88 @@
   +    $bundle = substr($bundle, 6);
   

   Let's add a comment on what we are doing here. Or better yet, create a class constant with comments what this magic number is all about.

5. +++ b/src/Plugin/migrate/source/d7/FieldCollectionItemRevision.php
   @@ -0,0 +1,31 @@
   +  public function getIds() {
   

   Is this just the revision id or is it revision id + item_id?

6. +++ b/src/Plugin/migrate/source/d7/FieldCollectionType.php
   @@ -33,22 +33,20 @@ class FieldCollectionType extends DrupalSqlBase {
   +    // Remove field_ prefix for new bundle.
   

   Great comment. Better if we make this a class constant.

7. +++ b/src/Plugin/migrate/source/d7/FieldCollectionType.php
   @@ -33,22 +33,20 @@ class FieldCollectionType extends DrupalSqlBase {
   +    if (isset($this->configuration['addDescription'])  && $this->configuration['addDescription']) {
   

   It would be nice to add some docs on what this config option is all about.

8. +++ b/src/Plugin/migrate/source/d7/ParagraphsItem.php
   @@ -0,0 +1,86 @@
   +  public function getIds() {
   +    $ids = [
   +      'item_id' => [
   
   +++ b/src/Plugin/migrate/source/d7/ParagraphsItemRevision.php
   @@ -0,0 +1,31 @@
   +  public function getIds() {
   +    $ids = [
   +      'revision_id' => [
   

   I think that the data model in D7, paragraphs used a composite ID as well. If I'm correct, then this should be a combo of both values.

9. +++ b/src/Plugin/migrate/source/d7/ParagraphsItemRevision.php
   similarity index 86%
   rename from src/Plugin/migrate/source/d7/ParagraphType.php
   
   rename from src/Plugin/migrate/source/d7/ParagraphType.php
   rename to src/Plugin/migrate/source/d7/ParagraphsType.php
   

   Can we keep a BC layer and the former class in the code base? It should extend this new class and add a trigger error, etc as listed in https://www.drupal.org/core/deprecation#how-class. Its been in the code base for a while now, someone might be using it. And its super easy to add the BC layer, so let's just do the right thing.

10. +++ b/src/Plugin/migrate/source/d7/ParagraphsType.php
    @@ -30,7 +30,7 @@ class ParagraphType extends DrupalSqlBase {
    +    if (isset($this->configuration['addDescription'])  && $this->configuration['addDescription']) {
    

    I think a !empty check would do mostly the same thing. No?

11. +++ b/tests/src/Kernel/migrate/FieldCollectionItemRevisionSourceTest.php
    @@ -0,0 +1,44 @@
    + * @group paragraphs
    
    +++ b/tests/src/Kernel/migrate/FieldCollectionItemSourceTest.php
    @@ -0,0 +1,57 @@
    + * @group paragraphs
    
    +++ b/tests/src/Kernel/migrate/FieldCollectionTypeSourceTest.php
    @@ -0,0 +1,42 @@
    + * @group paragraphs
    

    Probably this is just bikeshedding, but should we create a new field_collection test group for these things?

12. +++ b/tests/src/Traits/FieldCollectionSourceData.php
    @@ -0,0 +1,134 @@
    +      'paragraphs_item' => [
    ...
    +      'paragraphs_item_revision' => [
    

    Can we add a quick comment here why we have paragraphs data in a field collection data provider?

No need for an id on the BC class. Its only there if someone extended it. The id and the plugin system will find the non-BC class in its new name.

I just discovered ConfigurablePluginInterface yesterday. I suggest we use it for all our configs here in these various source plugins. And I also don't see any doxygen added to sources that have config options. Let's add those docs. Not everyone is going to use an automatic deriver, or even know how to find it. So adding the docs on the classes themselves will only help others and our future selves.

Otherwise, this is looking really close.

Very close now.

1. +++ b/src/Plugin/migrate/source/DrupalSqlBase.php
   @@ -0,0 +1,58 @@
   +    // Configuration keys should be snake_case, not CamelCase. Adding a BC
   +    // layer for addDescription which should be depricated.
   

   There's nothing saying config should be snake vs camel. Commerce 2.x uses camel. Its actually not too uncommon in contrib.

2. +++ b/src/Plugin/migrate/source/d7/FieldCollectionItem.php
   @@ -40,7 +55,7 @@ class FieldCollectionItem extends FieldableEntity {
   +    if (!empty($this->configuration['field_name'])) {
   
   +++ b/src/Plugin/migrate/source/d7/FieldCollectionType.php
   @@ -43,7 +47,7 @@ class FieldCollectionType extends DrupalSqlBase {
   -    if (!empty($this->configuration['addDescription'])) {
   
   +++ b/src/Plugin/migrate/source/d7/ParagraphsItem.php
   @@ -36,7 +51,7 @@ class ParagraphsItem extends FieldableEntity {
   +    if (!empty($this->configuration['bundle'])) {
   

   This can all be simplified to simple checks as I see you did in other places.

   Like: if ($this->configuration['foo'])

All looking good. Only very, very small nits. Which could be resolved at commit or left in the code. Commiter discretion.

1. +++ b/src/Plugin/migrate/source/d7/FieldCollectionItemRevision.php
   @@ -0,0 +1,38 @@
   + *   of that particular type.
   
   +++ b/src/Plugin/migrate/source/d7/FieldCollectionType.php
   @@ -68,8 +89,30 @@ class FieldCollectionType extends DrupalSqlBase {
   +    // layer for addDescription which should be depricated.
   
   +++ b/src/Plugin/migrate/source/d7/ParagraphsType.php
   @@ -0,0 +1,102 @@
   +    // layer for addDescription which should be depricated.
   

   Nit: spelling. Can be fixed on commit.

2. +++ b/src/Plugin/migrate/source/d7/FieldCollectionType.php
   @@ -68,8 +89,30 @@ class FieldCollectionType extends DrupalSqlBase {
   +    if (isset($configuration['addDescription']) && !isset($configuration['add_description'])) {
   
   +++ b/src/Plugin/migrate/source/d7/ParagraphsType.php
   @@ -0,0 +1,102 @@
   +    if (isset($configuration['addDescription']) && !isset($configuration['add_description'])) {
   

   We should trigger only if addDescription is set. No need to check if add_description too. But no big deal either if left in the code. Could be removed at time of commit.

There's now enough nits I'm going to unfortunately push back to NW.

+++ b/src/Plugin/migrate/source/d7/FieldCollectionItem.php
@@ -0,0 +1,114 @@
+    $default_configuration = [
...
+    ];
...
+    return NestedArray::mergeDeep(parent::defaultConfiguration(), $default_configuration);

This, and elsewhere, doesn't match the typical method for doing this. This was bugging me when I looked at the code earlier, but I couldn't put my finger on what it was. Below is much more typical.

return [
      'field_name' => '',
    ] + parent::defaultSettings();

Phenomenal work here. All feedback addressed.