With the Field API improvements that went in the past weeks / months, looks like MetadataGeneratorInterface::generateField() could be refactored a bit:
- method name is weird, this is not generating a field :-)
- signature is generateField(EntityInterface $entity, FieldDefinitionInterface $field_definition, $langcode, $view_mode),
and part of the job of the method is to work on $items = $entity->getTranslation($langcode)->get($field_definition->getFieldName())
Typically, those kind of functions have switched to receiving $items directly, letting the caller resolve entity translation.
So this could be just generateField(FieldItemListInterface $items, $view_mode)
- editorSelector->getEditor($formatter_id, $field_definition, $items) should receive $items as the FieldItemListInterface object, instead of one that has been cast to an array by ->getValue()
Could then just be getEditor($formatter_id, FieldItemListInterface $items) - and then switching the order of arguments might be more consistent with what the rest of core does ($items is typically the primary business logic parameter)
Comment | File | Size | Author |
---|---|---|---|
#26 | interdiff.txt | 2.79 KB | Wim Leers |
#26 | edit_fielditemlistinterface_refactor-2144879-26.patch | 33.1 KB | Wim Leers |
Comments
Comment #1
Wim LeersThat's what I implemented in the attached patch in any case.
\Drupal\Core\Field\FieldItemListInterface
object, I can indeed retrieve the associated entity and field definition, so in theory this is indeed possible. But is it really better?AFAICT most functions that have switched to receiving
FieldItemListInterface $items
directly have done so because their primary task is to manipulate the values of that.But in this case, the majority of the metadata actually comes from the entity (access check, ARIA attribute value), field definition (label, ARIA attribute value).
$items
is used as an additional piece of context while selecting an in-place editor.EditorSelectorInterface
case, I agree that this change makes sense.Additionally, if we're changing
EditorSelectorInterface
, then it also makes sense to changeEditPluginInterface
. So I changed that too.Note that the attached patch works perfectly, but tests fail. I have no idea how to navigate through the maze of classes to get the
EditorSelectionTest::testText()
DUTB test working again. I'd appreciate some help with that :)(I tried, but failed.)
Comment #2
yched CreditAttribution: yched commented- 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);
Comment #4
Wim LeersFirst, a straight reroll so the patch applies again. Should result in the same test failures.
Comment #6
Wim LeersWorks for me. Done.
What about this, then: pass both
FieldDefinitionInterface $field_definition
andFieldItemListInterface $items
togenerateFieldMetadata()
?That's technically duplication, but the semantics are more clear.
This also has a semantical problem: it means it's possible to pass the untranslated node as well, since there isn't a separate interface I can typehint on.
Comment #7
yched CreditAttribution: yched commentedWell, 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 ?
Comment #10
Wim LeersOkay, then I concede. I personally find it very confusing. But I think that over time, it will be the clearest — I think my historical understanding might be causing that confusion :)
Also fixed all tests. Should come back green.
Comment #11
Wim LeersForgot to remove two debug statements.
Comment #14
yched CreditAttribution: yched commentedThanks Wim :-)
From the interdiff:
(and lots of similar renames)
$items is actually the usual var name for a FieldItemListInterface across core ;-)
Comment #15
Wim LeersIt is, except in one place. Plus, in many cases in Drupal core, the docs say "the field" or "the field object". And that's what makes sense in all of Edit's cases too, "the field values" or "the field items" doesn't make much sense (unless you have the necessary knowledge of the internals). So that's why I named them consistently like this.
Is there a good reason to not do it this way?
Comment #16
Wim Leers11: edit_fielditemlistinterface_refactor-2144879-11.patch queued for re-testing.
Comment #18
yched CreditAttribution: yched commentedNaming 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.
Comment #19
yched CreditAttribution: yched commentedMore 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 ;-)
Comment #20
Wim LeersFirst, let's get this patch green — finally :)
Comment #21
Wim LeersHehe :) I just want to make sure I continue to understand the module I'm supposed to maintain, hence my continued hesitance.
Thanks for the additional historical context you've provided in #18, that really helps!
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". Especially the$items
part.However, #19 has convinced me. If I see Field API also working solely with this parameter, then I have zero objections. I still think the naming is off/confusing (or maybe "misleading" is a better word), but it's better to then at least have consistently confusing naming, that's still less confusing :)
All this interdiff changes, is renaming
FieldItemListInterface $field
toFieldItemListInterface $items
.Comment #22
yched CreditAttribution: yched commentedSure - that's the constant maintainer battle. I totally sympathize with that...
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 :
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.
Similarly, $metadata['format'] = $items[0]->format would work.
+ similar in Drupal\editor\Plugin\InPlaceEditor\Editor
Comment #23
yched CreditAttribution: yched commentedWrong tab, meant to do this.
Comment #25
yched CreditAttribution: yched commented21: edit_fielditemlistinterface_refactor-2144879-21.patch queued for re-testing.
Comment #26
Wim LeersThanks for providing yet more context & insight — that not only is helpful for me for understanding the current situation, it also makes me optimistic about where we're heading :)
yched++
Reroll to address remarks in #22.
Comment #27
yched CreditAttribution: yched commentedStill green. Confirming RTBC.
Thanks a lot @Wim !
Comment #28
webchickOk, there's quite a bit here, but it all looks consistent with what we're doing elsewhere for similar stuff.
Committed and pushed to 8.x. Thanks!
Comment #29
Wim LeersComment #30
Wim LeersBackported to the Drupal 7 version of Edit: #2178575: Backport: Brush up MetadataGeneratorInterface::generate(Field|Entity)().