d7_node_title_label migration plugin incorrectly generating base_field_override for every node type, even those that don't have an overridden title label

+++ b/core/modules/node/src/Plugin/migrate/source/d7/NodeType.php
@@ -33,7 +33,17 @@ class NodeType extends DrupalSqlBase {
+    if (isset($this->configuration['only_customized_title_labels'])) {
+      $query->condition('t.title_label', 'Title', '<>');
+    }

I understand the intention here, but let's not limit the source plugin like this. Normally, a source plugin gets all the data and the process plugin does the filtering. Yes, there are exceptions and prepareRows() do a lot of work in some cases. Instead, lets remove this and modify the pipeline in d7_node_title_label as needed.

Rereading this again and it seems that the problem is that base_field_overrides are created for every content type when it only needs to be created when the title field is, in fact, overridden. (If I understood that correctly then the IS needs an updated.) And that can be tested if the field label is not 'Title'. A deriver is not needed to do that, as the attached patch shows.

The total rows returned by the source plugin will be equal to the number of content types. We want the source plugin to make available all the data to the pipeline, which is where decisions are made. I understand the desire to change the source plugin to get 'just the data we want' as I have done that on several occasions for some of the i18n source plugins and each time I get feedback that the source plugin needs to get all the data. Plus, in this case we are not changing a source plugin which would then require changing the test.

Changing to NR to run tests.

I don't think this needs to be postponed. And the test has been updated to test the change to the migration.

Minor point, but an update IS and title would be appropriate at this point. Let's go back to NW for those small fixes.

+1 on RTBC. IS and title make a lot more sense now given what is in the patch.

These comments might be out of step with The Way In Which Things Are Done™ in the migrate system, so only setting down to "Needs review" vs. "needs work"... nevertheless, these were my impressions:

-      'base_field_override' => 9,
+      'base_field_override' => 4,

What's the significance of this change? Could we maybe leave a comment behind, so this doesn't accidentally get reordered in the future, which breaks this behaviour again?

+    // Other content type should not have a base_field_override.
+    $no_override_node_type = [
+      'article',
+      'blog',
+      'book',
+      'page',
+      'test_content_type',
+    ];
+    foreach ($no_override_node_type as $type) {
+      $override = BaseFieldOverride::load("node.$type.title");
+      $this->assertFalse($override instanceof BaseFieldOverride);
+    }

Nitpick: "types"

This does not seem to be reflective though of the use case brought by Wim. Wim had one content type in the database which was doing a title override. The remainder were not. Could we expand the comments here, like:

// Forum title labels are overridden to "Subject"; verify that this change made it over okay.

...

// The remainder are just using the default of "Title", so we should expect configuration to be skipped on the new site.

...or something like that?

NW to update the comments.

Updating comments. Oddly, the patch didn't apply for me so there is a diff not interdiff. The patch failed on /core/modules/migrate_drupal_ui/tests/src/Functional/d7/Upgrade7Test.php.

I ideally wanted a bit more "why" there but quietone out-voted me, so we'll go with this, which is certainly better than the status quo. :)

Committed and pushed to 9.0.x; 8.9.x; 8.8.x. Thanks!