Fieldable entity source plugins should report custom fields

Neither the D6 or D7 source plugin is reporting custom fields. Both should, when node_type is specified (it makes no sense without a specific node type to check for instances).

This shouldn't be too hard to implement. I've used this approach on a custom source for fieldable entities in D7, and assume it would work on nodes as well. It should work if we added something like this to the fields() function:

// Get Field API fields.
if (!empty($this->configuration['node_type'])) {
  $legacy_fields = $this->getFields('node', $this->configuration['node_type']);
}
else {
  $legacy_fields = $this->getFields('node');
}
foreach ($legacy_fields as $field_name => $instance) {
  $data = !empty($instance['data']) ? $instance['data'] : '';
  $data = unserialize($data);
  if (!empty($data['description'])) {
    $fields[$field_name] = $this->t('@field: @description', [
      '@field' => $data['label'],
      '@description' => @$data['description'],
    ]);
  }
  else {
    $fields[$field_name] = $this->t('@field', [
      '@field' => $data['label'],
    ]);
  }
}

I've attached a more compact version of the above suggestion. Note that this only applies to migrations using d7_node as source.
This also returns fields when no bundle is specified, in which case the user needs to discern whether the fields are actually available for the row being migrated (more flexibility for multi-bundle migrations, yay).

This can be generalized, I think, to handle any fieldable entity that either has no bundles, or has a source bundle key in the source configuration. FieldableEntity could add

protected function getFieldLabels($entity_type, $bundle = NULL)

(better name suggestions welcome).

Thus, the node plugins could call

$fields += $this->getFieldLabels('node', isset($this->configuration['node_type']) ? $this->configuration['node_type'] : NULL);

And the user plugins could call

$fields += $this->getFieldLabels('user');

Once #2746671: CCK field data not available for D7 taxonomy term migrations lands, d7_taxonomy_term could do

$fields += $this->getFieldLabels('taxonomy_term', isset($this->configuration['vocabulary']) ? $this->configuration['vocabulary'] : NULL);

We'd also want to implement getFieldLabels() in the d6_node source plugin, although maybe that should be a follow-up issue.

Good point, however how do we explain the differences between fields(), getFields() and the new getFieldLabels()? Seems to me that the fields should already be provided in FieldableEntity::fields(). That, however, would require a unified key to determine the bundle (e.g. bundle instead of node_type/vocabulary etc.), or some other way to figure out the correct key. (It's probably somewhere in the source data, but a getBundleKey() or the likes would be more durable.)

The bundle key is not discoverable - besides the fact that there's no database artifact documenting entities the way we have field_config/field_config_instance for fields, note that 'node_type' is not the bundle key in the D7 table, but an arbitrary configuration option key name in d7_node.yml. Yes, it would be nice to settle on 'bundle' as the key (with a BC layer continuing to accept node_type for nodes), but let's make that a separate issue. Maybe not too late for #2746671: CCK field data not available for D7 taxonomy term migrations to switch from 'vocabulary' to 'bundle' to make that simpler...

We could have a getBundleKey() with a default of NULL for sources without bundles (or which have not implemented a source option for querying on the bundle), yes.

Yes, the default fields() in FieldableEntity could retrieve the fields, then the concrete source plugin could add them in after listing the non-field properties, e.g.

return $fields + parent::fields();

I didn't do a re-roll as it was more easy to just take the hunk in the previous patch and respond to the feedback. Still needs tests.

Needs work because needs tests.

Rerolled this and changed getFieldLabels to always return an array plus translated the label because the values in fields() are translated.

I'd like to use MigrateSqlSourceTestBase for the test but that specializes in testing query().

Quick review:

1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d7/FieldableEntity.php
   @@ -37,6 +37,29 @@ protected function getFields($entity_type, $bundle = NULL) {
   +   * Returns the field labels for attached fields.
   

   It's been ages since I worked on D7... is 'attached fields' the correct terminology for what we call config fields on D8?

   At any rate, an extra line of docblock to explain what is meant by 'attached field' would help, as I imagine lots of developers will be in my position too of not remembering what this terminology means!

2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d7/FieldableEntity.php
   @@ -37,6 +37,29 @@ protected function getFields($entity_type, $bundle = NULL) {
   +   *   (optional) The bundle.
   

   This should say what it's going to do if $bundle is omitted.

3. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d7/FieldableEntity.php
   @@ -37,6 +37,29 @@ protected function getFields($entity_type, $bundle = NULL) {
   +  protected function getFieldLabels($entity_type, $bundle = NULL) {
   

   I think this needs a clearer name. getAttachedFieldLabels(), maybe?

   (Though see comment on the first line of the docblock.)

> I'd like to use MigrateSqlSourceTestBase for the test but that specializes in testing query().

Hmm yes, the problem is that MigrateSqlSourceTestBase has the testSource() method which expects a data provider in non-abstract child classes, and which we're not interested it at all. In an ideal world, I'd suggest we move testSource() to a new child class. But LOTS of test classes across core inherit from MigrateSqlSourceTestBase, so that would hugely complicate this patch.

I'd say make the test for this issue inherit from MigrateSourceTestBase, and just copy the getDatabase() method over to it.

We can then file a follow-up to refactor the test classes.

1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d7/FieldableEntity.php
   @@ -37,6 +37,29 @@ protected function getFields($entity_type, $bundle = NULL) {
   +  protected function getFieldLabels($entity_type, $bundle = NULL) {
   

   should use type-hints

2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d7/FieldableEntity.php
   @@ -37,6 +37,29 @@ protected function getFields($entity_type, $bundle = NULL) {
   +        $fields[$field_name] = $this->t($data['label']);
   

   it's weird t($data[label]) maybe label is already translated

This is a bug smash triage issue today.

I rerolled that patch and made changes for #17.1, #17.2 and #23.1. I am not running tests because it is not necessary yet, let's do that after there is new test.

#17.3. Not convinced the name change is the right thing to do, only because it is inconsistent with the existing method getFields() which gets the data for the fields attached to the entity.

But most importantly this needs a test.

Leaving as a bug because this is functionality that was in the D7 version of migrate.