Support multiple node types in node source plugins

Can we add a test?

Added more tests to the source plugin. Tests getting 1 content type, 2 content types and all content types if the configuration key 'node_type' is not set.

Discussed this migrate maintainers calls. Code looks fine. We had some concerns about adding this feature to core (rather than migrate plus) since this doesn't directly help core's built-in migrations. But it does help contrib and definitely custom migrations. It is a really trivial change and will make filtering things from legacy sites just that much easier.

1. +++ b/core/modules/node/src/Plugin/migrate/source/d6/Node.php
   @@ -113,7 +113,7 @@ public function query() {
   -      $query->condition('n.type', $this->configuration['node_type']);
   +      $query->condition('n.type', $this->configuration['node_type'], is_array($this->configuration['node_type']) ? 'IN' : '=');
   
   +++ b/core/modules/node/src/Plugin/migrate/source/d7/Node.php
   @@ -94,7 +94,7 @@ public function query() {
   -      $query->condition('n.type', $this->configuration['node_type']);
   +      $query->condition('n.type', $this->configuration['node_type'], is_array($this->configuration['node_type']) ? 'IN' : '=');
   

   This can be written with less conditions - which is always a bit simpler.

   $query->condition('n.type', (array) $this->configuration['node_type'], 'IN');

   Going to commit this anyway this the utility is more important but someone can feel free to file a follow-up issue if they like. Ah.... just spotted something else this can be addressed here.

2. +++ b/core/modules/node/src/Plugin/migrate/source/d7/Node.php
   index d5a8db09da..9c3a5a4102 100644
   --- a/core/modules/node/tests/src/Kernel/Plugin/migrate/source/d6/NodeByNodeTypeTest.php
   
   --- a/core/modules/node/tests/src/Kernel/Plugin/migrate/source/d6/NodeByNodeTypeTest.php
   +++ b/core/modules/node/tests/src/Kernel/Plugin/migrate/source/d6/NodeByNodeTypeTest.php
   

   Only adding test coverage to d6 is probably not enough - what happens if we move d6 out of core?

I recently discovered this limitation and will be happy to move this issue forward.

Here is the merge request with 3 commits:
1) Contains test only from #5;
2) Contains a similar test for D7 plugin per suggestion in #8;
3) Contains the actual fix for all node source plugins including NodeEntityTranslation.php.

I didn't find a way to run tests for specific commit in GitLab, so I'm also attaching patches (tests only + tests with fix). The first one is expected to fail. The second one is equal to the merge request.

I also updated the issue summary according to my report in duplicated issue: #3184835: Allow multiple bundles in node source plugins configuration.

@Matroskeen, thanks. I may have noticed that MRs are tested. Have you seen, https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa...

I didn't review the tests yet but I did notice that NodeEntityTranslationTest needs to be updated to includes different configurations. Setting to NW and adding tag for this.

Added one more dataset to NodeEntityTranslationTest with multiple content type configuration.
Moving back to NR.

Just some minor tidying up to do.

@Matroskeen, thanks.

Thanks for the review. MR was updated.

@Matroskeen, thanks for the quick reply.

I applied the latest patch and read the NodeEntityTranslationTest. The extra addition provides proof that the configuration value, node_type, works as expected. Nice!

All the points from the review have been addressed and there are no coding standard issues.

So RTBC!

To stop messages about the patch in #15 failing I have made a patch from the changes in Merge request !77 and uploaded it here.

I think we should document this. Not sure how we document migrate source configuration options.

Unfortunately, the source plugin configurations not documented. I have added it here using the style of the migrate process plugins in the migrate module as a guide.

@quitone, thanks!

I applied your patch to open MR and added one more commit with more examples: https://git.drupalcode.org/project/drupal/-/merge_requests/77/diffs?comm...

Thanks for adding the docs - this looks nice. Given it was rtbc in #23 and only docs have been added I'm re-instating that rtbc.

Committed 4a30675 and pushed to 9.2.x. Thanks!

Pushed one more commit to fix the typo 🙄

@alexpott, thanks for committing the patch to 9.2.x. Can we also fix the typo mention in this note? (please apply the patch 2937989-9.2.x-typo.txt)

Can we also cherry-pick this commit to 9.1.x? Here is a link to the patch if it makes it easier: https://git.drupalcode.org/project/drupal/-/merge_requests/77.patch

@Matroskeen new features do not get backported to 9.1.x. And please open a new issue to fix the typo. Re-opening fixed issues for typos makes issue / commit history more difficult than necessary.

Note this was supported originally and only broke because of #2358079: Require a specific placeholder format in db_query() in order to trigger argument expansion, and require explicit 'IN' parameter for conditions.