Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

FWIW, the documentation for the plugin definition entries was formerly in the phpdoc for hook_field_formatter_info(), which got removed in #1785748: Field formatters as plugins. The latest state of the phpdoc can be seen in the commitdiff.

tim.plunkett’s picture

Status: Active » Needs review
FileSize
33.31 KB

First pass.

Status: Needs review » Needs work

The last submitted patch, formatter-annotation-1985344-2.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
4.18 KB
37.49 KB

Heh, forgot all of the test formatters.

yched’s picture

Looks good to me, aside from:

+++ b/core/modules/field/lib/Drupal/field/Annotation/FieldFormatter.phpundefined
@@ -0,0 +1,89 @@
+  /**
+   * @todo Something from edit module, should not be in here.
+   *
+   * @var array
+   */
+  public $edit;

So what do we do with this ? Move those to an alter implementation in edit.module ?

swentel’s picture

So what do we do with this ? Move those to an alter implementation in edit.module ?

That would make sense in a way. Otoh, the edit module will probably need to have some way to determine whether it will be using 'direct' or 'form' (are there more?), otherwise we'll end up with a lot of hardcoded stuff in that alter.

Wim Leers’s picture

Issue tags: +sprint, +Spark

It is unacceptable (at least AFAICT currently) to have edit.module implement an alter hook. It's an extension of field formatters' metadata. It's not metadata that edit.module can calculate in any way. It's something that field formatter developers can set for their formatters.

Besides, AFAICT the end result would be the same, no? Edit module would then still alter field formatters' metadata, it'd become part of the field formatter metadata.

More generally, isn't it a valid use case for a module to extend a given plugin type in a certain way, so that more metadata would be needed? Then what would be the recommended course of action in those cases?

tim.plunkett’s picture

I think you misunderstood my request.
This has nothing to do with moving the 'edit' key out of individual field formatter implementations.
It is about moving the documentation for the 'edit' key out of the new @FieldFormatter annotation class, and documenting it with the module who will use it.

See #1968970: Standardize module-provided entity info documentation and clean-up the @EntityType annotation for relevant discussion.

Something like this, but with actual documentation.

Wim Leers’s picture

Aha! :) Sorry, I completely misunderstood. I thought you were referring to hook_field_formatter_info_alter()

It feels bizarre that we can use hook_entity_info() to document additional plugin metadata keys though. I suppose it is because a field formatter is now a config entity?


Instead of providing ready-to-go docs for in the patch, I'll just explain what happens here, because I don't see a similar example.

This is what the edit annotation looks like:

 *   edit = {
 *     "editor" = "direct"
 *   }

So, there's a new key in the annotation called "edit" containing any metadata for the Edit module. Currently, there's only one subkey for that, called "editor", which indicates which in-place editor should be used. The default is the 'form' in-place editor (which in turn uses the configured widget), which works for every Field API field type because each field type must have a widget. It's a less-than-stellar experience though, because it less direct than it could be. The 'direct' in-place editor (which is poorly named) allows direct in-place editing of simple textual fields. Hence we configure that as the default for textual fields.

I.e., this is how Edit module determines which in-place editor to use:

    // Check if the formatter defines an appropriate in-place editor. For
    // example, text formatters displaying untrimmed text can choose to use the
    // 'direct' editor. If the formatter doesn't specify, fall back to the
    // 'form' editor, since that can work for any field. Formatter definitions
    // can use 'disabled' to explicitly opt out of in-place editing.
    $formatter_info = field_info_formatter_types($formatter_type);
    $editor_id = isset($formatter_info['edit']['editor']) ? $formatter_info['edit']['editor'] : 'form';

Sorry for not providing a copy/pasteable chunk of text. Hope this helps.

tim.plunkett’s picture

FieldFormatter is a plugin type, not a config entity.
When a module other than Field.module wants to add to the annotation, it needs to document its keys.
Just because Edit.module is in core doesn't mean it can attach itself directly to the FieldFormatter class itself.

Wim Leers’s picture

Right. Well, as you can see, I'm confused.
I'm not advocating for Edit module to be treated specially.
I just don't see how implementing edit_entity_info() allows us to extend the docs of \Drupal\field\Annotation\FieldFormatter.

tim.plunkett’s picture

Oh. Duhh. My apologies, I see why you were confused now!

The idea from the other issue was about @EntityType, so we chose to document new keys on the info/alter hook. This is @FieldFormatter, so hook_field_formatter_info_alter() would be the correct place.

I'm afraid your explanation isn't clear enough to me for me to write up my own docs, but I'd appreciate it if you could just write them out as docs.

dawehner’s picture

Issue tags: -Field API, -sprint, -Plugin system, -Spark
+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterPluginManager.phpundefined
@@ -35,12 +27,19 @@ class FormatterPluginManager extends PluginManagerBase {
+    return new $plugin_class($plugin_id, $plugin_definition, $configuration['instance'], $configuration['settings'], $configuration['label'], $configuration['view_mode']);

Can we add a todo to use ContainerFactory or similar pattern on the longrun?

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Issue tags: +Field API, +Plugin system
FileSize
37.95 KB

Is our plan to always use ContainerFactory?

Only two formatters use the container, and both for \Drupal::token():

  • \Drupal\link\Plugin\field\formatter\LinkSeparateFormatter
  • \Drupal\link\Plugin\field\formatter\LinkFormatter

Rerolling.

dawehner’s picture

I guess we will use the same pattern as we do with controllers (make it optional to implement it).

swentel’s picture

So the problem is that hook_field_formatter_info_alter() is actually an existing hook, so this can't work for the annotation right, or am I missing something completely ?

swentel’s picture

This should do it - I think.

Status: Needs review » Needs work
Issue tags: -Field API, -Plugin system, -Annotation

The last submitted patch, field-formatter-1985344-17.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review

#17: field-formatter-1985344-17.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Field API, +Plugin system, +Annotation

The last submitted patch, field-formatter-1985344-17.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
655 bytes
39.14 KB

Oh man, hit me.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
swentel’s picture

reroll

swentel’s picture

Stupid doc typo.

yched’s picture

@tim.plunkett:
Wasn't there also a movement to standardize "discovered" folders to mymodule/lib/.../Plugins/[TheNameOfTheAnnotationClass] ?
(this is not meant to push back from RTBC, just to know where we stand on that topic)

Status: Reviewed & tested by the community » Needs work
Issue tags: -Field API, -Plugin system, -Annotation

The last submitted patch, field-formatter-1985344-24.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
Issue tags: +Field API, +Plugin system, +Annotation

#24: field-formatter-1985344-24.patch queued for re-testing.

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Random failure

alexpott’s picture

Title: Add a dedicated @FieldFormatter annotation » Change notice: Add a dedicated @FieldFormatter annotation
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed 984940b and pushed to 8.x. Thanks!

I'm sure that the change notice for field formatters as plugins needs updating... https://drupal.org/node/1805846

swentel’s picture

Title: Change notice: Add a dedicated @FieldFormatter annotation » Add a dedicated @FieldFormatter annotation
Status: Active » Fixed

Yes indeed, added to the change notice. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

xjm’s picture

Issue tags: -Needs change record

Untagging. Please remove the tag when the change notification task is completed.