Now that widgets are in good shape, time to start on formatters !

Core issue used for testbot runs (until we have a patch ready to push for review in the wild):
#1785690: Testbot helper issue for 'formatters as plugins'

I extracted the code I had in field-plugins-yched, and adapted it to match the current design of the widgets code.
It lives in the field-plugins-formatters-1785748 branch.

The FieldInstance::getFormatter() method is a little more involved than getWidget(), since an $instance holds several formetters, and because of nice things like field_view_field($arbitrary_display_settings), and $display['type'] = 'hidden'.
Also, the prepareView() method being 'multiple' (acts on a array of $entities) adds some fun logic.

Only text formatters are migrated for now. A legacy layer, quite similar to the one we put for widgets, catches the other ones.
To match the current state of widgets, we should also migrate number formatters and test formatters.

Subtasks
#1785752: Move test formatters to the new API
#1785750: Move number formatters to the new API
#1787236: Convert email module formatters to Plugin system
#1787238: Convert options module formatters to Plugin system
#1787242: Convert file module formatters to Plugin system
#1787246: Convert image module formatters to Plugin system
#1787248: Convert taxonomy module formatters to Plugin system

Files: 
CommentFileSizeAuthor
#77 Drupal-followup-1785748-70.patch2.8 KBaritra.ghosh
PASSED: [[SimpleTest]]: [MySQL] 49,343 pass(es).
[ View ]
#67 1785748-67.patch125.94 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 42,164 pass(es).
[ View ]
#53 1785748-53.patch126.34 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 42,285 pass(es).
[ View ]
#53 interdiff.txt694 bytesswentel
#49 field-plugins-formatters-1785748-49.patch126.32 KBStalski
PASSED: [[SimpleTest]]: [MySQL] 42,288 pass(es).
[ View ]
#42 field-plugins-formatters-1785748-42.patch126.24 KByched
PASSED: [[SimpleTest]]: [MySQL] 42,108 pass(es).
[ View ]
#42 interdiff.txt2.41 KByched
#41 field-plugins-formatters-1785748-41.patch125.07 KByched
PASSED: [[SimpleTest]]: [MySQL] 42,098 pass(es).
[ View ]
#41 interdiff.txt7.61 KByched
#33 field-plugins-formatters-1785748-23.patch122.89 KByched
PASSED: [[SimpleTest]]: [MySQL] 42,085 pass(es).
[ View ]
#20 field-plugins-formatters-1785748-20.patch121.9 KByched
PASSED: [[SimpleTest]]: [MySQL] 41,974 pass(es).
[ View ]
#15 field-plugins-formatters-1785748-15.patch122.63 KByched
PASSED: [[SimpleTest]]: [MySQL] 41,976 pass(es).
[ View ]
#13 field-plugins-formatters-1785748-13.patch98 KByched
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field-plugins-formatters-1785748-13.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#10 field-plugins-formatters-1785748-10.patch0 bytesyched
PASSED: [[SimpleTest]]: [MySQL] 41,994 pass(es).
[ View ]
#9 field-plugins-formatters-1785748-9.patch215.55 KByched
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field-plugins-formatters-1785748-9.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#8 field-plugins-formatters-1785748-7.patch92.1 KByched
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field-plugins-formatters-1785748-7_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#7 field-plugins-formatters-1785748-7.patch213.03 KByched
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field-plugins-formatters-1785748-7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#5 field-plugins-formatters-1785748-5.patch92.04 KByched
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field-plugins-formatters-1785748-5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#4 field-plugins-formatters-1785748-4.patch92.04 KByched
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field-plugins-formatters-1785748-4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#2 field-plugins-formatters-1785748-2.patch77.32 KByched
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field-plugins-formatters-1785748-2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#1 field-plugins-widgets-formatters-1785748-1.patch75.57 KByched
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field-plugins-widgets-formatters-1785748-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

StatusFileSize
new75.57 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field-plugins-widgets-formatters-1785748-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Attached is a patch against field-plugins-widgets-1785256.

Note: i haven't been able to get testbots run on this yet, the bots look like they are on strike tonight.

Project:D8 Field API» Drupal core
Issue summary:View changes

added real meat

Issue summary:View changes

added testbot issue

Issue summary:View changes

added formatting

Project:Drupal core» D8 Field API
StatusFileSize
new77.32 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field-plugins-formatters-1785748-2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Pushed :
- e8783c6: stop pre-filling display settings for all view modes in _field_info_prepare_instance()

Status:Active» Needs work

Two minor things in there

+++ b/core/modules/field/modules/text/lib/Drupal/text/Plugin/field/formatter/TextTrimmedFormatter.php
@@ -0,0 +1,82 @@
+ * Note: This class also contains the implementations usedby the

typo: used by

+++ b/core/modules/field/modules/text/lib/Drupal/text/Plugin/field/formatter/TextTrimmedFormatter.php
@@ -0,0 +1,82 @@
+   * Implements FormatterInterface::viewElements().

Possible inconsistence in the comments, in other formatters the whole class name is informed, e.g. Implements Drupal\field\Plugin\Type\Formatter\FormatterInterface::viewElements()

StatusFileSize
new92.04 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field-plugins-formatters-1785748-4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Pushed :
- #1785752: Move test formatters to the new API by pcambra
- fixes for the remarks in #3 - thanks !

Updated patch against field-plugins-widgets-1785256

Status:Needs work» Needs review
StatusFileSize
new92.04 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field-plugins-formatters-1785748-5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Pushed:
- merged latest field-plugins-widgets-1785256 - includes:
latest 8.x
do not include upstream fixes for #1778942: Discovery::getDefinition() / getDefinitions() : inconsistent return values for "no result", but work around by overriding PluginManagerBase::getDefinition() in WidgetPluginManager.
- c0f5d7a: Added same workaround in FormatterPluginManager

This looks great, here are some code style nitpicks that aren't urgent, but could be kept in mind going forward:

+++ b/core/modules/field/field.attach.incundefined
@@ -197,6 +197,145 @@ function field_invoke_method($method, Closure $target_closure, EntityInterface $
+ * @param $method
+ *   The name of the method to invoke.
+ * @param $target
+ *   A closure that receives an $instance object and returns the object on

Might as well add the type hints here and for newly added functions/methods

+++ b/core/modules/field/field.attach.incundefined
@@ -197,6 +197,145 @@ function field_invoke_method($method, Closure $target_closure, EntityInterface $
+ *   An array of entities, keyed by entity id.

ID, not id. (Here and other places)

+++ b/core/modules/field/field.attach.incundefined
@@ -197,6 +197,145 @@ function field_invoke_method($method, Closure $target_closure, EntityInterface $
+ * @param $a
+ *   A parameter for the invoked method.
+ * @param $b
+ *   A parameter for the invoked method.
+ * @param $options

These should mention their default values.

+++ b/core/modules/field/field.attach.incundefined
@@ -197,6 +197,145 @@ function field_invoke_method($method, Closure $target_closure, EntityInterface $
+ *  - 'field_name': The name of the field whose operation should be invoked. By
+ *    default, the operation is invoked on all the fields in the entity's
+ *    bundle. NOTE: This option is not compatible with the 'deleted' option; the
+ *    'field_id' option should be used instead.
+ *  - 'field_id': The id of the field whose operation should be invoked. By

The hyphens should be indented one more space, and there shouldn't be quotes around the key names

+++ b/core/modules/field/field.attach.incundefined
@@ -197,6 +197,145 @@ function field_invoke_method($method, Closure $target_closure, EntityInterface $
+function field_invoke_method_multiple($method, Closure $target, $entities, &$a = NULL, &$b = NULL, $options = array()) {

As I understood it, this either had to be use'd at the top of the file, or have an inline leading \ per #1614186: Coding standards for using "native" PHP classes like stdClass in namespaced code

+++ b/core/modules/field/modules/text/lib/Drupal/text/Plugin/field/formatter/TextDefaultFormatter.phpundefined
@@ -0,0 +1,44 @@
+    return $elements;
+  }
+}
diff --git a/core/modules/field/modules/text/lib/Drupal/text/Plugin/field/formatter/TextPlainFormatter.php b/core/modules/field/modules/text/lib/Drupal/text/Plugin/field/formatter/TextPlainFormatter.php
diff --git a/core/modules/field/modules/text/lib/Drupal/text/Plugin/field/formatter/TextPlainFormatter.php b/core/modules/field/modules/text/lib/Drupal/text/Plugin/field/formatter/TextPlainFormatter.php
new file mode 100644

Should have a blank line after the method (Here and other places)

StatusFileSize
new213.03 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field-plugins-formatters-1785748-7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Thanks Tim.

Most of those remarks actually apply to #1785256: Widgets as Plugins as well, so :
- fixed those upstream where applicable and rerolled #1785256: Widgets as Plugins
- pushed fixes for the formatter-specific bits :
71d8db3
76b2df4

StatusFileSize
new92.1 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field-plugins-formatters-1785748-7_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Oops, patch was rolled against 8.x rather than field-plugins-widgets-1785256.
Rerolled.

StatusFileSize
new215.55 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field-plugins-formatters-1785748-9.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

- Fixed a couple test fails
- Lots of phpdoc fixes, pointed by@Lars Toomre

Project:D8 Field API» Drupal core
Issue summary:View changes

Added subtasks

Project:Drupal core» D8 Field API
StatusFileSize
new0 bytes
PASSED: [[SimpleTest]]: [MySQL] 41,994 pass(es).
[ View ]

Fixes last test notices, we should be green now.

If this patch is re-rolled, here are some additional documentation notes. Otherwise, the enhanced docblocks are much better than before!!

+++ b/core/modules/field/field.info.incundefined
@@ -496,52 +366,42 @@ function field_info_field_types($field_type = NULL) {
+ * @return array
+ *   Either a single formatter type description, as provided by class annotations,
+ *   or an array of all existing formatter types, keyed by formatter type name.
  */

This comment needs to reformated to wrap at or before column 80.

+++ b/core/modules/field/lib/Drupal/field/FieldInstance.phpundefined
@@ -0,0 +1,226 @@
+
+  public $definition;

Doesn't this also require a docblock?

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterInterface.phpundefined
@@ -0,0 +1,104 @@
+   * @param Drupal\Core\Entity\EntityInterface $entity
+   *   A single object of type $entity_type.

Not sure why $entity_type is referenced here... Is this correct?

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterPluginManager.phpundefined
@@ -0,0 +1,88 @@
+
+  protected $defaults = array(
+    'settings' => array(),
+  );
+
+  protected $cache_bin = 'field';
+  protected $cache_key = 'field_formatter_types';

All of these need docblocks explaining the variable type and type hint.

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/LegacyDiscoveryDecorator.phpundefined
@@ -0,0 +1,67 @@
+
+  protected $hook;

Needs a docblock.

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Widget/WidgetBase.phpundefined
@@ -0,0 +1,447 @@
+  /**
+   * Constructs a WidgetBase object.
+   *

Other constructor methods in this patch do not have docblocks. I am unsure on what the documentation policy is, but we should be consistent throughout this patch.

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Widget/WidgetBase.phpundefined
@@ -0,0 +1,447 @@
+  /**
+   * Special handling to create form elements for multiple values.
+   *
+   * Handles generic features for multiple fields:
+   * - number of widgets
+   * - AHAH-'add more' button
+   * - table display and drag-n-drop value reordering
+   */
+  protected function formMultipleElements(EntityInterface $entity, array $items, $langcode, array &$form, array &$form_state) {

I believe all of these variables need @param directives with type hinting and explanations. Also needs @return for $elements variable.

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Widget/WidgetBase.phpundefined
@@ -0,0 +1,447 @@
+  protected function formSingleElement(EntityInterface $entity, array $items, $delta, $langcode, array $element, array &$form, array &$form_state) {

This needs @param and @return directives.

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Widget/WidgetLegacyDiscoveryDecorator.phpundefined
@@ -0,0 +1,38 @@
+
+  protected $hook = 'field_widget_info';
+
+  public function processDefinition(array &$definition) {

Missing docblocks....

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Widget/WidgetPluginManager.phpundefined
@@ -0,0 +1,90 @@
+  protected $defaults = array(
+    'settings' => array(),
+    'multiple_values' => FALSE,
+  );
+
+  protected $cache_bin = 'field';
+  protected $cache_key = 'field_widget_types';
+  protected $hook = 'field_widget_info';

Missing docblocks...

@yched... I am not sure what happened in #10, but patch is zero bytes.

StatusFileSize
new98 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field-plugins-formatters-1785748-13.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Some of #11 applies to this issue, some apply to #1785256: Widgets as Plugins.
I updated the patches to take those into account.

Updated formatters patch attached.

@yched: I am not sure if I am in the right issue, but here is my review of your patch in #13.

+++ b/core/modules/field/field.api.phpundefined
@@ -2113,27 +1919,27 @@ function hook_field_info_max_weight($entity_type, $bundle, $context) {
+ * @param array $context
  *   An associative array containing:
  *   - entity_type: The entity type; e.g., 'node' or 'user'.
+ *   - bundle: The bundle: e.g., 'page' or 'article'.
  *   - field: The field being rendered.
  *   - instance: The instance being rendered.
- *   - entity: The entity being rendered.
  *   - view_mode: The view mode, e.g. 'full', 'teaser'...

I know that it is not part of our coding standards, but it would be super helpful as a programmer to know what was expected for each of these array elements.

Does it make sense to do something like - entity_type: (string) The entity type ...

Based on the current documentation, I am unsure of what is expected for 'instance' key.

+++ b/core/modules/field/field.attach.incundefined
@@ -197,6 +197,135 @@ function field_invoke_method($method, \Closure $target_closure, EntityInterface
+ *   An array of entities, keyed by entity ID.
+ * @param mixed $a
+ *   A parameter for the invoked method. Defaults to NULL.
+ * @param mixed $b
+ *   A parameter for the invoked method. Defaults to NULL.

Both of these declarations should start with '(optional)'. Actually same goes for the $options variable as well.

+++ b/core/modules/field/field.attach.incundefined
@@ -197,6 +197,135 @@ function field_invoke_method($method, \Closure $target_closure, EntityInterface
+ * @return
+ *   An array of returned values keyed by entity id.

Can we add a type hint here for consistency?

+++ b/core/modules/field/field.attach.incundefined
@@ -1187,7 +1336,8 @@ function field_attach_prepare_view($entity_type, $entities, $view_mode, $langcod
+  // @todo not sure about that...

Can you expand upon this comment? Very unclear what you mean...

+++ b/core/modules/field/field.info.incundefined
@@ -49,22 +49,10 @@ function field_info_cache_clear() {
- *     module, giving the module that exposes the formatter type.
  *   - 'storage types': Array of hook_field_storage_info() results, keyed by

I am pretty sure that keys of an array should not be surrounded by single quotes.

+++ b/core/modules/field/field.info.incundefined
@@ -471,25 +388,21 @@ function field_info_widget_types($widget_type = NULL) {
- * @param $formatter_type
+ * @param string $formatter_type
  *   (optional) A formatter type name. If omitted, all formatter types will be

This should reference what the default result is.

+++ b/core/modules/field/lib/Drupal/field/FieldInstance.phpundefined
@@ -27,6 +27,13 @@ class FieldInstance implements \ArrayAccess {
   /**
+   * The formatter objects used for this instance, keyed by view mode..

Small nit... ends in double periods, should be a single period.

+++ b/core/modules/field/lib/Drupal/field/FieldInstance.phpundefined
@@ -68,6 +75,89 @@ class FieldInstance implements \ArrayAccess {
+   *     @todo : from_field_view_field() : "If no display settings are found for
+   *     the view mode, the settings for the 'default' view mode will be used." ??
+   *   - An array of display properties, as accepted by setDisplayConfig().

This should be rewrapped for 80 character maximum.

+++ b/core/modules/field/lib/Drupal/field/FieldInstance.phpundefined
@@ -68,6 +75,89 @@ class FieldInstance implements \ArrayAccess {
+   *
+   * @return Drupal\field\Plugin\Type\Formatter\FormatterInterface

For consistency, this should be 'FormatterInterface|null'

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterBase.phpundefined
@@ -0,0 +1,147 @@
+
+use Drupal\Component\Plugin\Discovery\DiscoveryInterface;
+use Drupal\Core\Entity\EntityInterface;
+use Drupal\field\Plugin\PluginSettingsBase;
+use Drupal\field\FieldInstance;

Not sure, but should not all use declarations be in alphabetical sort order?

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterInterface.phpundefined
@@ -0,0 +1,104 @@
+   * @return string

Silly question from reading code comments... Can this summary string include HTML tags or is it expected to be plain text?

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterInterface.phpundefined
@@ -0,0 +1,104 @@
+   *   Array of entities being displayed, keyed by entity ID.
+   * @param string $langcode
+   *   The language the field values are to be shown in. If no language is
+   *   provided the current language is used.

Unclear what is meant by 'If no language is provided'. First, there should be a comma after provided. However, not clear what happens if for instance $langcode is set to NULL in call.

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterPluginManager.phpundefined
@@ -2,27 +2,27 @@
use Drupal\Component\Plugin\PluginManagerBase;
use Drupal\Core\Plugin\Discovery\CacheDecorator;
use Drupal\Core\Plugin\Discovery\AnnotatedClassDiscovery;
-use Drupal\field\Plugin\Type\LegacyDiscoveryDecorator;
+use Drupal\field\Plugin\Type\Formatter\FormatterLegacyDiscoveryDecorator;
+use Drupal\Component\Plugin\Factory\ReflectionFactory;

Same comment as above about ordering of multiple use statements. Alphabetical?

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/LegacyDiscoveryDecorator.phpundefined
@@ -11,14 +11,19 @@ use Drupal\Component\Plugin\Discovery\DiscoveryInterface;
- *

Should not this @todo remain?

StatusFileSize
new122.63 KB
PASSED: [[SimpleTest]]: [MySQL] 41,976 pass(es).
[ View ]

Home for a couple days between two flights :-)

- Rerolled on current 8.x (includes widgets as plugins and FieldInfo changes)
- Merged #1785750: Move number formatters to the new API by pcambra, with a couple refactoring from myself.
- Added a couple doc adjustments after #14

@yched - when you're off again, let us know if there's anything we should focus on while you're gone so we can bring this forward

Note - have you got my mail re: how to arrange the train/flight stuff easily - man that sound stupids to type that here :)

I think the only blocker now is some performance checks.

I guess relevant testing cases would be :

  1. single node view:
    one node
    10 simple text fields,
    5 values in each field.
  2. multiple node view, same bundle:
    10 teasers of nodes of the same node type,
    10 simple text fields in each node,
    5 values in each field.
  3. multiple node view, different bundles:
    10 teasers of nodes of 10 different node types,
    9 simple text fields and 1 taxonomy_term field in each node,
    5 values in each field.

Last case is the worst case, because :
- different node types mean different $instances, and thus formatter objects are not reused across nodes, we instanciate more of them.
- taxonomy_term formatters have a 'prepare_view' step (taxonomy_field_formatter_prepare_view()) that does taxonomy_term_load_multiple(). The new architecture means prepare_view steps run in smaller groups (one per instance instead of one per formatter module), so that's more load_multiple() with less ids in each, which should have a perf impact that would be good to assess.

I'll see whether I can move forward on this before tuesday.

Alright, I'll check the drush scripts I've already written, it already allows to create a content type with a random number of textfields, adding a taxonomy term field option should be easy, I'll try to update in an hour.

Done, added option to generate term fields/instances as well. It uses the tags vocabulary that comes default with core install. makes it easier to setup scenarios faster.

command: drush fact 9 1

http://drupal.org/sandbox/swentel/1781486 (note, you're maintainer too, so feel free to make adjustments)

I'll run some tests tomorrow, rest of the day is dedicated to do nothing :)

StatusFileSize
new121.9 KB
PASSED: [[SimpleTest]]: [MySQL] 41,974 pass(es).
[ View ]

Updated patch - mostly :
- de7b973: forward-port the same workaround that was added in #1751234: Convert option widgets to Plugin system in the LegacyDiscovery to allow info_alter hooks on new-style widgets
- 141001d thus, we can remove the hack in mail field tests (mail fields use formatter_info_alter to use text formatter as their default formatter)

What's left before this issue can be moved to the core queue? Would be great to get a bot check plus increase visibility.

re @effulgentsia :
This should be mostly ready, aside from possible doc / code style nitpicks.
Patch is green (I've been using #1785690: Testbot helper issue for 'formatters as plugins' to run bots while we are in the sandbox)

Last thing we'd need is performance checks, cause moving from "one hook does the job of all formatters in a module" to "one formatter object per field instance" has inherent performance costs (more function calls, more objects instantiation, less grouping of multiple loads in perpare_view).
I've outilined testing cases in #17 above, but I'll probably go afk before I get a chance to run them myself :-/

Title:[META] Formatters as pluginsConvert field formatters to plugins
Project:D8 Field API» Drupal core
Version:» 8.x-dev
Component:Code» field system

Sounds to me like this should be in the core queue then, so doing so. Please reset if I'm wrong though.

@yched when would you like a detailed review for possible doc / code style issues?

@Lars Toomre: thanks for the offer :-). Preferably after the perf test results validate the architecture.

Per #22, this patch is nearly done, so I think a docs and code style review would be appropriate now, unless yched says otherwise. Also, this issue isn't blocking any other major or critical issues that I'm aware of, so incorporating that feedback should be done as part of this issue, not a follow up.

Did some basic performance tests, not being able todo all the use cases though, and no xhprof so far, I'll try todo one before wednesday, but then I'm afk after that until next monday as well. The cool thing is that we already profit from the _field_info_collate_patch() that got in :)

I have a public dump available if anyone in the meantime wants to have a go: http://dl.dropbox.com/u/15116672/dump-d8.zip
It contains 10 content types, each with 9 text fields and 1 term field. It has a lot of terms and 5000 nodes.

Single node view
before: Page execution time was 65.91 ms. Memory used at: devel_boot()=1.2 MB, devel_shutdown()=4.89 MB, PHP peak=5 MB.
after: Page execution time was 68.29 ms. Memory used at: devel_boot()=1.24 MB, devel_shutdown()=5.07 MB, PHP peak=5.25 MB.

Front page (I think it lists 7 different node types now)
before: page execution time was 141.57 ms. Memory used at: devel_boot()=1.24 MB, devel_shutdown()=7.55 MB, PHP peak=9.25 MB.
after: page execution time was 143.88 ms. Memory used at: devel_boot()=1.24 MB, devel_shutdown()=7.85 MB, PHP peak=9.5 MB.

So far, not really alarming if you tell me.

Thanks. 68.29 vs. 65.91 = 3.6%, which is enough to warrant digging into.

Thanks @swentel ! I hope I can give a deeper look with xhprof tonight,

From what we've seen in the "widgets as plugins" patch, I kind of hope those 3% are mostly caused the class autoloading overhead - "hooks -> plugins" inherently means more classes to load, and we know our autoloader is slow.

@yched - Happy to help. Let me know when! I really like your work here.

Note about the patch: type hinting with \Closure is a really bad idea. Closures were at the origin an implementation detail not supposed to be public, even if it may be now, it's still more flexible to type hint using callbable instead. Note that the callable type hint is not permited until 5.4 but at least you can document it as a callable and just not type hint it.

If callable is not available until 5.4, it is inappropriate to use while Drupal requires 5.3.x. In my mind, type hinting is a MUST for all new code being added to D8.

If you think Closure is wrong to use as a type hint, open up an issue to change it once 5.4 becomes a minimal requirement for Drupal. Until then, this should be type hinted as accurately as possible.

StatusFileSize
new122.89 KB
PASSED: [[SimpleTest]]: [MySQL] 42,085 pass(es).
[ View ]

So, those were the numbers I found using @swentel's db dump in #27:

- /node/%nid (9 text fields, 1 taxo field)
before: 97.081 ms / after: 99.295 ms (+2%)
XHProf results:
overhead in node_show() : +5.711 (patch only affects the callstack below)
with:
UniversalClassLoader::loadClass(): +3.809
ReflectionFactory: +1.200
Which pretty much adds up.

- /node (10 nodes, 10 fields each: 9 text + 1 taxo)
before: 247.598 ms / after: 259.321 ms (+4.7%)
XHProf results:
overhead in node_view_multiple(): +16.005 (patch only affects the callstack below)
with:
UniversalClassLoader::loadClass(): +3.584
ReflectionFactory: +8.927
Which leaves ~3.500 ms that I wasn't really able to track down to a specific place.

So:
- Silly me for forgetting to add a dedicated FormatterFactory instead of Reflectionfactory, like we did for widgets. Attached patch adds it.
See commitdiff.
- The rest of the overhead is mostly spent in the ClassLoader.

Unfortunately, my poor laptop seems to have overheated after that, and doesn't produce consistent before / after benchmarks for this new version of the patch. Will retry tomorrow.

If callable is not available until 5.4, it is inappropriate to use while Drupal requires 5.3.x. In my mind, type hinting is a MUST for all new code being added to D8.

We technically can't type hint here, because it makes impossible to use any other form of callable. This can't be commited with a \Closure type hint.

PHP has technical limitations, and this is one <5.4 specific, we must live with it: you still don't type hint for strings and integers right? Then the same behavior for callables apply, don't type hint.

If we pass a callable we must not type hint else we loose great flexibility for no reason. We can replace the type hint by: if (!is_callable($my_param)) { throw new \InvalidArgumentException(); } in place to reproduce the fatal behavior of the wrong call.

Updated benchmarks with patch in #33:

- /node/%nid (9 text fields, 1 taxo field)
after: 84.139 ms / after: 85.101 ms (+1.1%)

- /node (10 nodes, 10 fields each: 9 text + 1 taxo)
before: 192.044 ms / after: 194.785 ms (+1.4%)

As explained in #33, and as was the case for "widgets as plugins", the overhead mostly comes from the autoloader.

So, I think we're ready !

Ready for a detailed code /doc review too?

@Lars Toomre : yup :-)

re #34: makes sense I guess. I'll reroll later today removing the Closure type hint.

Status:Needs review» Needs work

Here is what I observed in #33. Please realize that is intended to be a nit picking code / documentation style review.

+++ b/core/modules/field/field.api.phpundefined
@@ -2145,27 +1952,27 @@ function hook_field_info_max_weight($entity_type, $bundle, $context) {
+ * @param array $context
  *   An associative array containing:
  *   - entity_type: The entity type; e.g., 'node' or 'user'.
+ *   - bundle: The bundle: e.g., 'page' or 'article'.
  *   - field: The field being rendered.
  *   - instance: The instance being rendered.
- *   - entity: The entity being rendered.

Just to be consistent, let's change 'entity type;' to 'entity type:'.

+++ b/core/modules/field/field.api.phpundefined
@@ -2180,23 +1987,23 @@ function hook_field_display_alter(&$display, $context) {
- * @param $display
+ * @param array $display_properties
  *   The display settings that will be used to display the field values, as

The variable used in the function call is $display. This needs to be adjusted.

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterBase.phpundefined
@@ -0,0 +1,147 @@
+
+use Drupal\Component\Plugin\Discovery\DiscoveryInterface;
+use Drupal\Core\Entity\EntityInterface;
+use Drupal\field\Plugin\PluginSettingsBase;
+use Drupal\field\FieldInstance;
+
+/**

Are use statements suppose to be orders in alphabetical order?

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterBase.phpundefined
@@ -0,0 +1,147 @@
+   *
+   * @param array $plugin_id
+   *   The plugin_id for the formatter.
+   * @param Drupal\Component\Plugin\Discovery\DiscoveryInterface $discovery

Glad that this is type hinted. I would have guessed that plugin_id was a string otherwise.

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterInterface.phpundefined
@@ -0,0 +1,108 @@
+  /**
+   * Return a short summary for the current formatter settings.

I think this needs to be 'Returns'.

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterInterface.phpundefined
@@ -0,0 +1,108 @@
+  /**
+   * Allow formatters to load information for field values being displayed.
+   *
+   * This should be used when a formatter needs to load additional information

I think this needs to be 'Allows'.

+++ b/core/modules/field/modules/number/lib/Drupal/number/Plugin/field/formatter/NumberDecimalFormatter.phpundefined
@@ -0,0 +1,73 @@
+
+use Drupal\Core\Annotation\Plugin;
+use Drupal\Core\Annotation\Translation;
+use Drupal\field\Plugin\Type\Formatter\FormatterBase;
+use Drupal\number\Plugin\field\formatter\DefaultNumberFormatter;
+use Drupal\Core\Entity\EntityInterface;

I think that these should be sorted into alphabetical order.

+++ b/core/modules/field/modules/number/lib/Drupal/number/Plugin/field/formatter/NumberIntegerFormatter.phpundefined
@@ -0,0 +1,45 @@
+
+use Drupal\Core\Annotation\Plugin;
+use Drupal\Core\Annotation\Translation;
+use Drupal\field\Plugin\Type\Formatter\FormatterBase;
+use Drupal\number\Plugin\field\formatter\DefaultNumberFormatter;
+use Drupal\Core\Entity\EntityInterface;

Ibid.

+++ b/core/modules/field/modules/number/lib/Drupal/number/Plugin/field/formatter/NumberUnformattedFormatter.phpundefined
@@ -0,0 +1,44 @@
+
+use Drupal\Core\Annotation\Plugin;
+use Drupal\Core\Annotation\Translation;
+use Drupal\field\Plugin\Type\Formatter\FormatterBase;
+use Drupal\Core\Entity\EntityInterface;

Ibid.

+++ b/core/modules/field/modules/text/lib/Drupal/text/Plugin/field/formatter/TextDefaultFormatter.phpundefined
@@ -0,0 +1,45 @@
+
+use Drupal\Core\Annotation\Plugin;
+use Drupal\Core\Annotation\Translation;
+use Drupal\field\Plugin\Type\Formatter\FormatterBase;
+use Drupal\Core\Entity\EntityInterface;

Ibid.

Just to be consistent, let's change 'entity type;' to 'entity type:'.

Actually, they should both be commas.

Are use statements suppose to be orders in alphabetical order?

I think that these should be sorted into alphabetical order.

Please stop posting this on issues, it's not in our coding standard. They can be in any order.

Status:Needs work» Needs review
StatusFileSize
new7.61 KB
new125.07 KB
PASSED: [[SimpleTest]]: [MySQL] 42,098 pass(es).
[ View ]

Thanks for the review, Lars.

+++ b/core/modules/field/field.api.phpundefined
@@ -2145,27 +1952,27 @@ function hook_field_info_max_weight($entity_type, $bundle, $context) {
+ * @param array $context
  *   An associative array containing:
  *   - entity_type: The entity type; e.g., 'node' or 'user'.
+ *   - bundle: The bundle: e.g., 'page' or 'article'.

Just to be consistent, let's change 'entity type;' to 'entity type:'.

Consistent with what ? AFAIK, we're rather moving away from those array keys with spaces in them, so I'm for keeping this as is, esp. if it avoids a needless API break.

+++ b/core/modules/field/field.api.phpundefined
@@ -2180,23 +1987,23 @@ function hook_field_display_alter(&$display, $context) {
- * @param $display
+ * @param array $display_properties
  *   The display settings that will be used to display the field values, as

The variable used in the function call is $display. This needs to be adjusted.

Actually, that was the case in the old field_get_display() function, that is not used anymore, but the new calling code (in FieldInstance::getFormatter()) is actually correct. Updated patch removes field_get_display().

- Fixed the other remarks (except those about the ordering of 'use' statements)
- Removed the '\Closure' type hint in field_invoke_method() and field_invoke_method_multiple(), and use 'callable' in the phpdoc, as per #34. Also improved consistency of the variable names around there.

Interdiff with #33 attached.

StatusFileSize
new2.41 KB
new126.24 KB
PASSED: [[SimpleTest]]: [MySQL] 42,108 pass(es).
[ View ]

Oops, sorry, finally got what you meant about 'entity type:'
Fixed.

Thanks @yched. The docs look much good.

Title:Convert field formatters to pluginsField formatters as plugins

Re-title to match #1785256: Widgets as Plugins

Status:Needs review» Needs work

#42 patch still includes Closure type hint and documentation in some various points (in @return statement for example).

Another minor, but yet important detail: if you use callable as type hint (at least as document type for now) you should call it using call_user_func($var, $a1, $a2) instead of direct $var($a, $a2); Performance penalty will be almost insignificant and it will avoid PHP WSOD if you pass there an array($object, 'methodName') instead of a Closure object.

Re #45: Ack.
However, I won't be able to easily reroll patches for the next couple weeks.
Anyone ? :-)

Also, #1705702: Provide a way to allow modules alter plugin definitions got in, which means HookDiscovery no longer runs the hook_field_formatter_info_alter().
- AlterDecorator needs to be added to the discovery used in FormatterPluginManager,
- and the workaround in LegacyDiscoveryDecorator can be removed (denoted by a @todo)

I'm not able to roll patches right now - anyone up for it ?

I'll reroll it today and enforce the suggested changes.

StatusFileSize
new126.32 KB
PASSED: [[SimpleTest]]: [MySQL] 42,288 pass(es).
[ View ]
  • Rerolled the patch
  • changed
    <?php
    * @return Closure
    *   A 'target closure' for field_invoke_method().
    ?>

    into
    <?php
    * @return callable $target_function
    *   A 'target function' for field_invoke_method().
    ?>
  • Changed the invocation to call_user_func array in field_invoke_method(). Please review if that's correct and sufficient
  • Added the AlterDecorator to the FormatPluginManager.
  • Removed the workaround in LegacyDiscoveryDecorator

Tried to create interdiff as well but then I needed to do the reroll again.

SideQuestion:
Are there already issues created to let widgets use the AlterDecorator too? maybe some other stuff too?

Status:Needs work» Needs review

Great! @pounard - if you're ok with this one, can you push RTBC ? Or let us know where changes are needed ? Thanks!

field_invoke_method_multiple() needs call_user_func() as well :-)

StatusFileSize
new694 bytes
new126.34 KB
PASSED: [[SimpleTest]]: [MySQL] 42,285 pass(es).
[ View ]

This ?

Nice swentel, that's it!
@yched, thanks again!
Really hope it's ready to go now.

I won't RTBC it because I prefer to let people that know better the field API do it. Just know that callable type hint sounds fine now and I'm OK with the latest patch regarding that specific point.

@pounard fair enough, thanks for the OK on the remarks you had :)

Maybe we can create a change record modelled after http://drupal.org/node/1796000 in the mean time - we can safely assume this will go in (and I can remove the change record afterwards in case this won't make it.).

edit: change notice at http://drupal.org/node/1805846

Status:Needs review» Reviewed & tested by the community
Issue tags:+Needs committer feedback

The patch is well-tested in sandbox and manually so some clean-ups could be done in follow-ups. Let's get them commited

@andypost: why "needs committer feedback"?

@yched because this change requires @catch feedback according http://drupal.org/node/1207020

"because the community was not able to come to a consensus and needs a top-down decision"

This is RTBC, that on its own means the committers will look at it.

#1778942: Discovery::getDefinition() / getDefinitions() : inconsistent return values for "no result" got in, so we can remove the workaround in FormatterPluginManager (denoted by a @todo refenrencing the issue URL).
Doesn't do any harm, though, so keeping RTBC.

Oh, almost forgot :

@ core committers : @pcambra should be credited too in the commit message.

@yched did you forget the patch?

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterPluginManager.phpundefined
@@ -0,0 +1,106 @@
+  /**
+   * Overrides Drupal\Component\Plugin\PluginManagerBase::getDefinition().
+   *
+   * @todo Remove when http://drupal.org/node/1778942 is fixed.
+   */
+  public function getDefinition($plugin_id) {
+    $definition = $this->discovery->getDefinition($plugin_id);
+    if (!empty($definition)) {
+      $this->processDefinition($definition, $plugin_id);
+      return $definition;;
+    }
+  }
+

Yup, missed this

Status:Needs work» Reviewed & tested by the community

Yup. As I said, though, this does no harm right now and can still be removed in a followup. Thus, keeping RTBC.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new125.94 KB
PASSED: [[SimpleTest]]: [MySQL] 42,164 pass(es).
[ View ]

Let's do that now :)

Status:Needs review» Reviewed & tested by the community

#67: 1785748-67.patch queued for re-testing.

Title:Field formatters as pluginsChange notice (+minor follow-up): Field formatters as plugins
Priority:Normal» Critical
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

Ok sorry for the delay on this folks; this took some time to go through.

In general this looks very similar to the widgets patch, so once again, great work on the clean-up there.

I committed and pushed to 8.x, but had some follow-up questions/observations. Also tagging for a change notice.

+++ b/core/modules/field/field.api.phpundefined
@@ -2145,27 +1952,27 @@ function hook_field_info_max_weight($entity_type, $bundle, $context) {
+ * @param array $context
...
- *   - entity: The entity being rendered.
...
-function hook_field_display_alter(&$display, $context) {
+function hook_field_display_alter(array &$display_properties, array $context) {
@@ -2180,23 +1987,23 @@ function hook_field_display_alter(&$display, $context) {
+ * @param array $context
...
- *   - entity: The entity being rendered.
...
-function hook_field_display_ENTITY_TYPE_alter(&$display, $context) {
+function hook_field_display_ENTITY_TYPE_alter(array &$display_properties, array $context) {

Why remove entity here? That seems like valuable context. I noticed 'module' was also removed from another place (can't recall where, sorry).

+++ b/core/modules/field/field.attach.incundefined
@@ -577,6 +711,26 @@ function _field_invoke_widget_target() {
+ * @param mixed $display

+++ b/core/modules/field/lib/Drupal/field/FieldInstance.phpundefined
@@ -68,6 +75,88 @@ public function getWidget() {
+   * @param mixed $display_properties

I am really not a fan of these "mixed" type variables. Would it make sense here to make view mode (or whatever) an explicit function parameter?

+++ b/core/modules/field/field.attach.incundefined
@@ -577,6 +711,26 @@ function _field_invoke_widget_target() {
+function _field_invoke_formatter_target($display) {
+  return function ($instance) use ($display) {
+    return $instance->getFormatter($display);
+  };
+}

woah. crazy syntax. didn't know you could do that. I can't quite parse what's happening there, so a comment would be great.

Priority:Critical» Normal

Change notice is at http://drupal.org/node/1805846, so this is normal again.

Also, @yched, should we just bundle the plugins and widgets conversions into one issue per module ?

@webchick : yay, thacaanks. I will comment on your remarks when i can access a real keyboard in a couple days :-)

@swentel: some widgets (options, file...) are a bit complex, so my gut feeling would be to keep separate issues. But maybe for simple stuff like mail, url... we can group.
We should probably open a meta issue to track conversion tasks.

Title:Change notice (+minor follow-up): Field formatters as plugins(minor follow-up): Field formatters as plugins

Change notice has been taken care of.

Oops, followup for a bug this introduced in Field UI: #1848040: Fatal error on "Manage display" after creating new field

Status:Active» Needs review
StatusFileSize
new2.8 KB
PASSED: [[SimpleTest]]: [MySQL] 49,343 pass(es).
[ View ]

Provided the patch to
- remove 'mixed' from @params in function header
- comments for the use of php closure

Title:Field formatters as plugins(minor follow-up): Field formatters as plugins

So, this is long overdue, but re: @webchick's #70

- $entity in hook_field_display_alter() / hook_field_display_ENTITY_TYPE_alter() :
Those hooks don't receive $entity anymore because they are not called on a per-entity basis anymore. This was just not doable efficiently. Formatter plugin objects are created and persisted through the request per field instance (i.e for all entities in a bundle). If every individual entity can come and specify they want a different formatter for the field, then we have no way to share formatter objects, and we would need to create one formatter for each field x each entity. Massive perf impact on listing pages.
This was discussed with @chx, who introduced the request for "per entity hook_field_display_alter()" in #830020: field_get_display does not get the entity, and it was deemed a reasonable regression. The use cases for "alter display settings for a field for a given entity" can use hook_entity_view_mode_alter(), that runs entity by entity.

- "mixed" parameter in _field_invoke_widget_target() / FieldInstance::getWidget():
That area gets refactored in #1852966: Rework entity display settings around EntityDisplay config entity, both functions disappear, and the "mixed" param too.
So I'd rather we leave it that way for now, or it will just cause yet another reroll in that other issue.

- Comment on the closure on _field_invoke_formatter_target()
As said above, this is refactored in #1852966: Rework entity display settings around EntityDisplay config entity, and a comment is added over there.

All in all,
- sorry for the delay :-/
- thanks @aritra.ghosh for patch #77, it is technically valid, but those points are adressed in #1852966: Rework entity display settings around EntityDisplay config entity, let's rather get that one in.

Thus, setting to 'fixed', we're done here.

Title:(minor follow-up): Field formatters as pluginsField formatters as plugins
Status:Needs review» Fixed

resetting title, too.

Title:(minor follow-up): Field formatters as pluginsField formatters as plugins
Issue tags:-Needs change record

Removing tags as well

Status:Fixed» Closed (fixed)

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

Issue summary:View changes

Added new issues for converting the rest of formatters