Files: 
CommentFileSizeAuthor
#24 field-formatter-1985344-24.patch39.17 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 57,204 pass(es).
[ View ]
#24 interdiff.txt633 bytesswentel
#23 field-formatter-1985344-23.patch39.17 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 55,461 pass(es).
[ View ]
#21 field-formatter-1985344-21.patch39.14 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 57,791 pass(es).
[ View ]
#21 interdiff.txt655 bytesswentel
#17 field-formatter-1985344-17.patch39.15 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 55,860 pass(es), 1 fail(s), and 4 exception(s).
[ View ]
#17 interdiff.txt2.27 KBswentel
#14 field-formatter-1985344-14.patch37.95 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,066 pass(es).
[ View ]
#12 field-formatter-1985344-12.patch37.97 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 55,904 pass(es).
[ View ]
#12 interdiff.txt576 bytestim.plunkett
#8 formatter-annotation-1985344-8.patch37.94 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 55,788 pass(es).
[ View ]
#8 interdiff.txt1.1 KBtim.plunkett
#4 formatter-annotation-1985344-4.patch37.49 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 55,608 pass(es).
[ View ]
#4 interdiff.txt4.18 KBtim.plunkett
#2 formatter-annotation-1985344-2.patch33.31 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 55,325 pass(es), 18 fail(s), and 21 exception(s).
[ View ]

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
StatusFileSize
new33.31 KB
FAILED: [[SimpleTest]]: [MySQL] 55,325 pass(es), 18 fail(s), and 21 exception(s).
[ View ]

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
StatusFileSize
new4.18 KB
new37.49 KB
PASSED: [[SimpleTest]]: [MySQL] 55,608 pass(es).
[ View ]

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

StatusFileSize
new1.1 KB
new37.94 KB
PASSED: [[SimpleTest]]: [MySQL] 55,788 pass(es).
[ View ]

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

StatusFileSize
new576 bytes
new37.97 KB
PASSED: [[SimpleTest]]: [MySQL] 55,904 pass(es).
[ View ]

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
StatusFileSize
new37.95 KB
PASSED: [[SimpleTest]]: [MySQL] 57,066 pass(es).
[ View ]

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

StatusFileSize
new2.27 KB
new39.15 KB
FAILED: [[SimpleTest]]: [MySQL] 55,860 pass(es), 1 fail(s), and 4 exception(s).
[ View ]

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
StatusFileSize
new655 bytes
new39.14 KB
PASSED: [[SimpleTest]]: [MySQL] 57,791 pass(es).
[ View ]

Oh man, hit me.

dawehner’s picture

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

StatusFileSize
new39.17 KB
PASSED: [[SimpleTest]]: [MySQL] 55,461 pass(es).
[ View ]

reroll

swentel’s picture

StatusFileSize
new633 bytes
new39.17 KB
PASSED: [[SimpleTest]]: [MySQL] 57,204 pass(es).
[ View ]

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.