Brush up MetadataGeneratorInterface::generate(Field|Entity)(): use FieldItemListInterface + better naming

- Method name:
AFAIK we try to have method names that carry a standalone meaning (verb + complement) without the help of the class name. I.e, even if the object is a "Metadata generator", it helps IMO to have the method state that it generates metadata :-)

--> I'd suggest generateFieldMetadata() ? getFieldMetadata() ?

- arguments:
I see your point about $entity being the primary source of logic, and actual field values being only secondary. I'd argue that this prioritization is only a detail true of the current implementation in MetadataGenerator::generateField(). At the interface level, the semantic of the method is "the metadata can depend on the $items field values", even if in some/most cases it will only depend on the entity & field definition.
But I'll let you see what you think is best :-)

At any rate, the $langcode should IMO still disappear from the signature, and resolved upstream by the calling code taking care of the $entity->getTranslation($langcode)

--> generateFieldMetadata($entity, $field_definition, $view_mode);

What about this, then: pass both FieldDefinitionInterface $field_definition and FieldItemListInterface $items to generateFieldMetadata()?
That's technically duplication, but the semantics are more clear

Well, I'd tend to understand that as "if I get passed $items and $definition as separate params, it means they can differ - i.e the calling code can decide to pass a $definition that's different from $items->getFieldDefinition() - or maybe the $definition is in fact supposed to be another, entirely unrelated definition ?"
So, not sure about clearer semantics ;-)
I think at this point the rest of core is pretty standard about the fact that a FieldItemListInterface $items means access to the underlying definition.

Regarding $langode: well, the moment the caller passes generateFieldMetadata() a FieldItemListInterface param, it means it already has effectively narrowed to a language. So no need to pass a useless $langcode separately ?

Thanks Wim :-)

From the interdiff:

+++ b/core/modules/edit/lib/Drupal/edit/EditPluginInterface.php
@@ -18,13 +18,13 @@
-   * @param \Drupal\Core\Field\FieldItemListInterface $items
+   * @param \Drupal\Core\Field\FieldItemListInterface $field

(and lots of similar renames)
$items is actually the usual var name for a FieldItemListInterface across core ;-)

Naming has gone through a lot of confusion in this cycle :-/.
EntityNG initially started naming all its "field value objects" $field / Field / "the field",
while the Field API that came from D7 used $field for "the definition structures", and $items for values.

After a couple months of confusion and debates, it's been agreed in Prague to settle on :
- FieldDefinition / $field_definition for the definition structures
- FieldItem / FieldItemList / $item / $items for the values

It's very likely that there remains places in core that haven't been updated accordingly, but we've been cleaning up existing code progressively, and trying to make sure new code follows the above.

More thoughts:

IMO it makes a lot of sense that the object passed to generateFieldMetadata() is the same than the one passed to WidgetInterface::form() or FormatterInterface::view() - an $item - because that's exactly the level at which these metadata come into play : included in the formatter display, used to generate a widget in a form. You generate metadata for the exact same thing the formatter / widget generate HTML. So parameters being the same is actually rather telling IMO, those really are sister APIs.

Not willing to start a war on that though - your system, your call. Just refining my arguments ;-)

I just want to make sure I continue to understand the module I'm supposed to maintain, hence my continued hesitance.

Sure - that's the constant maintainer battle. I totally sympathize with that...

It's just that whenever I see FieldListItemInterface $items, that signals to me "these are merely the values of the field, not any metadata-ish/definition-ish of the field. ... I still think the naming is off/confusing (or maybe "misleading" is a better word)

TBH I'm not fully convinced that we have nailed the absolute best naming pattern either. But that was a thorny discussion, concessions were made, an agreement was found, which was a win in itself :-). And yeah, consistency helps clarity too.

Other than that, being able to retrieve metadata about a runtime object from the runtime object itself has become more & more common in many places in D8: from an entity you can get the definitions of its fields, and soon its "entity type definition" too; from a plugin object you can get its plugin definition... Similarly, from a "field value object" you can get its definition, its langcode, its parent entity...

This greatly helped saving API verbosity & consistency IMO, because you don't need to think of adding separate params to pass the various metadata (entity type, field definition, langcode...) along (and make sure they're in sync) in case implementations need them; or force them to fetch the metadata themselves, which means dependencies on the metadata registries and prevents one-off variants...

Anyway - glad that you agree :-)

Minor remarks, but no blockers, patch is committable as is :

1. +++ b/core/modules/edit/tests/modules/lib/Drupal/edit_test/Plugin/InPlaceEditor/WysiwygEditor.php
   @@ -32,7 +34,7 @@ function isCompatible(FieldDefinitionInterface $field_definition, array $items)
   -      $format_id = $items[0]['format'];
   +      $format_id = $items[0]->get('format')->getValue();
          if (isset($format_id) && $format_id === 'full_html') {
   

   Core code tends to standardize on the shorthands : $items[0]->format.
   Also, I think the isset check isn't necessary anymore.
   if ($items[0]->format === 'full_html') should do the job.

2. +++ b/core/modules/edit/tests/modules/lib/Drupal/edit_test/Plugin/InPlaceEditor/WysiwygEditor.php
   @@ -43,8 +45,8 @@ function isCompatible(FieldDefinitionInterface $field_definition, array $items)
        return $metadata;
   ...
   +    $format_id = $items[0]->get('format')->getValue();
        $metadata['format'] = $format_id;
   

   Similarly, $metadata['format'] = $items[0]->format would work.

   + similar in Drupal\editor\Plugin\InPlaceEditor\Editor

Wrong tab, meant to do this.

21: edit_fielditemlistinterface_refactor-2144879-21.patch queued for re-testing.

Still green. Confirming RTBC.

Thanks a lot @Wim !