Support for GatherContent Components

Yeh, this would be massively useful.

Any plans?

Started that, with the assumption that we flatten the components.

It depends on the patch and new client introduced here https://www.drupal.org/project/gathercontent/issues/3315895

related pull request for the client: https://github.com/gathercontent/gathercontent-php-client/pull/4

+++ b/gathercontent_ui/src/Form/MappingEditSteps/MappingSteps.php
@@ -648,4 +648,29 @@ abstract class MappingSteps {
+          $item_component->label = $item->label . ':' . $item_component->label;

space after :, I'd also suggest to do this as t('@component: @label') or so, that allows to "translate" it to rtl languages.

1. +++ b/gathercontent.module
   @@ -166,3 +166,29 @@ function gathercontent_on_entity_delete(EntityInterface $entity, $skipLanguage =
   +/**
   + * Get list of entity types.
   + *
   + * @param array $fields
   + *   Fields array.
   + * @return array
   + *   Array of fields.
   + */
   +function gathercontent_flattenGroup($fields) {
   

   this is a strange mix of a function (snake_case) and a method (camelCase).

   If we keep it as a function, it should be snake_case, but the goal for most code should be to be in a class, unless it's a hook or so.

   That's a bit tricky as we use it in multiple places. There are two options:
   * We add it as a public static function on e.g. MappingSteps.
   * We add it as a trait that we use in both places.

   a trait is slightly cleaner but also a bit more work/code.

   notes on coding standards/style:
   * empty line between params and return
   * use explict array type for parameter and the return type: (array $fields): array
   * for the the @param and @return, you can define it as list of element classes with ...\Element[]

   then, naming. You use 3 different concepts now for this (fields, item and component), that should all be the same. children is ok as it's generic.

   They call those classes Elements, so I would go with that, so that would be $elements as parameter, then elements as element foreach, then $children and $child_element. and the function/method name then also uses flattenGroup, that too should be either flattenFields or Elements.

   Update: I did just read below that suddenly they call them fields there as well. So it's also OK to go with fields, as long as it's consistent.

   the description should also explain a bit more what the different between the two is.

2. +++ b/gathercontent.module
   @@ -166,3 +166,29 @@ function gathercontent_on_entity_delete(EntityInterface $entity, $skipLanguage =
   +    if (is_a($item, "GatherContent\DataTypes\ElementComponent")) {
   

   instanceof.

3. +++ b/gathercontent_ui/src/Form/MappingEditSteps/MappingSteps.php
   @@ -231,37 +231,18 @@ abstract class MappingSteps {
   +          if (isset(${$groupType . '_fields'}[$lang_code]) && in_array($element, ${$groupType . '_fields'}[$lang_code])) {
   +            $formState->setErrorByName($groupId,
   +              $this->t('A GatherContent field can only be mapped to a single Drupal field. So each field can only be mapped to once.'));
   +            return;
   +          }
   

   It should be continue (proceed with next item in array), not return.

4. +++ b/gathercontent_ui/src/Form/MappingEditSteps/MappingSteps.php
   @@ -294,6 +275,18 @@ abstract class MappingSteps {
      }
    
   +  protected function getFieldFromId($id, $group_id = NULL) {
   

   missing docblock.

5. +++ b/gathercontent_ui/src/Form/MappingEditSteps/MappingSteps.php
   @@ -294,6 +275,18 @@ abstract class MappingSteps {
   +    if ($group_id && !strpos($id, '/')) {
   +      return $this->template['related']->structure->groups[$group_id]->fields[$id];
   +    }
   +    if ($group_id && strpos($id, '/')) {
   +      $ids = explode('/', $id);
   +      $component = $this->template['related']->structure->groups[$group_id]->fields[$ids[0]];
   +      $children = $component->getChildrenFields();
   

   a bit confused why $group_id here is optional, it won't do or return anything then?

   the call above does not check if the function returns something or not, either it must always return something (add a return type then to enforce that) or not, but then the caller needs to support that.

6. +++ b/gathercontent_upload/src/Export/MappingCreator.php
   @@ -440,7 +440,7 @@ class MappingCreator implements ContainerInjectionInterface {
          $fieldId = $field->id();
          if (!empty($parentFieldId)) {
   -        $fieldId = $parentFieldId . '||' . $fieldId;
   +        $fieldId = $parentFieldId . '/' . $fieldId;
   

   this is an existing replacement, are you sure that's correct?

7. +++ b/src/Plugin/migrate/source/GatherContentMigrateSource.php
   @@ -287,13 +287,7 @@ class GatherContentMigrateSource extends SourcePluginBase implements ContainerFa
   -
   -            $value[] = [
   -              'value' => $this->getFieldValue($subObject),
   -            ];
              }
   

   are we sure that there are no existing things that use this structure?

Thanks for your feedback :)

I addressed most of it:

1. I could not put this function in MappingSteps, because it is used in MappingEditForm. I moved it to a trait.

5: my reasoning was that I thought that it would be nice to not depend on group_id, but it was simpler to just rely on it, and I did not go to the next step of writing a version without group_id, since I did not need it. I removed the optionality.

6: removed that, thanks

7: this led to duplication after my changes in getFieldValue

1. +++ b/gathercontent_ui/src/Form/MappingEditSteps/MappingStepNew.php
   @@ -4,6 +4,7 @@ namespace Drupal\gathercontent_ui\Form\MappingEditSteps;
    use Drupal\Core\Form\FormStateInterface;
    use Drupal\Core\StringTranslation\StringTranslationTrait;
   +use Drupal\gathercontent\MappingTrait;
    
    /**
     * Class MappingStepNew.
   

   this is just the namespace use, you need to use it in the classes as well, see traits in base class for example.

   there are also more classes that call it. I'd suggest putting it in base classes if possible.

   (if you would make it public static you could have accessed it from other classes too, but trait is ok too)

2. +++ b/src/MappingTrait.php
   @@ -0,0 +1,36 @@
   +
   +namespace Drupal\gathercontent\Mapping;
   +
   +use GatherContent\DataTypes\ElementComponent;
   +
   +trait MappingTrait {
   +
   +  /**
   +   * Flattens a nested array of fields (e.g. with Components).
   +   *
   +   * @param \GatherContent\DataTypes\Element[] $elements
   +   *   Nested fields array.
   +   *
   +   * @return \GatherContent\DataTypes\Element[]
   +   *   Flat array of fields.
   +   */
   +  protected function flattenGroup($elements): array {
   +    $flat_elements = [];
   

   namespace and location does not match. also not sure if this should be in ui module, it's only used there.

Fixed that, and also removed the patch from the patch :D

Committed.