Move Widget, Formatter, and FieldType plugin types to the Core\Field system

You can now choose any directory you want:
/Plugin/$owner/$type
/Plugin/$owner$type
/$owner$type

For example,
Drupal\telephone\Plugin\field\field_type\TelephoneItem
could become
Drupal\telephone\Entity\FieldType\TelephoneItem

I'd like to see Drupal\Core\Entity\FieldType\ for the manager, annotation classes, etc.

Drupal\Core\Entity\Widget\
Drupal\Core\Entity\Formatter\

Shouldn't we put entity field stuff in:

Drupal\Core\Entity\Field\Widget\
Drupal\Core\Entity\Field\Formatter\

?

@plach It depends. Let's think about where the plugins would go in a module, the Drupal\Core\Entity is ambiguous to me

Given
Drupal\text\Plugin\field\formatter\TextPlainFormatter

Would that become
Drupal\text\Entity\Field\Formatter\TextPlainFormatter
or
Drupal\text\Field\Formatter\TextPlainFormatter

How about Drupal\text\Plugin\Entity\FieldFormatter\TextPlainFormatter? Because:

1. While I support removing the Plugin subfolder for entity types, because those are special, I don't support doing that for other, more normal, plugins that just happen to be in the Entity system.

2. We currently have Drupal\Core\Entity\Field, not Drupal\Core\Field. So I think the "owner" needs to be Entity. If we choose to promote Field to its own top-level subsystem, then I would recommend Drupal\text\Plugin\Field\Formatter\TextPlainFormatter.

- their alter hook should be hook_entity_[widget|formatter]_info_alter(), documented in entity.api.php

This looks a bit wonky because widgets and formatters are still applicable to entity fields, not to entities as a whole..

It might just mean that we need to rename them to FieldWidget and FieldFormatter.

But that possibly means hook_entity_field_widget_info_alter(), hook_entity_field_formatter_info_alter().

Yes, that's what I was hinting at :)

"if we chose to promote Field to its own top level subsystem" ... Sounds appealing, but I'm not sure what this would mean in practice ?

@fago: any feedback on that? That would allow us to do hook_field_widget_info_alter() instead of hook_entity_field_widget_info_alter(). It might also make more sense, given that "entity system" and "field system" are currently separate components in the d.o. issue queue and in MAINTAINERS.txt.

added tasks about cache bins and alter hooks

Discussed this with Alex Pott, effulgentsia and xjm and we all believe the side-effects of this API change are very small. Approved.

@fago: any feedback on that? That would allow us to do hook_field_widget_info_alter() instead of hook_entity_field_widget_info_alter(). It might also make more sense, given that "entity system" and "field system" are currently separate components in the d.o. issue queue and in MAINTAINERS.txt.

I'm not sure.

In general, I'd be totally with having a separate Field component in general and I see the advantages in maintainer/issue component questions. But, I do not really see how to draw the line here?

I mean with previous node properties being node entity fields now, the entity system really depends and makes use of entity fields. When we move that to Core/Field it means that Core/Entity strictly depends on it, what would be fine - but Core/Field would strictly depend on Core/Entity as well and is not usable without it. It's not possible to have fields without entities and fields are something inherent for entities - thus I see it more as part of the Entity component, i.e. Entity/Field.

That's a very code-wise view though.

I can totally second #7, thus Drupal\text\Plugin\Entity\FieldFormatter\TextPlainFormatter would make a lot of sense to me.

Makes it much easier to keep things consistent (name and location of managers, annotation classes, discovery folders), than trying to make everything fit somehow in the already crowded Core/Entity.

Imho that could work out with Core/Entity/Field as well.

Yes, Core/Entity & Core/Field would be two subsystems that depend on each other. As I wrote earlier, i don't really see this as a real problem. De facto they are two code bases that are both intertwined and with IMO separate maintenance responsibilities.

Yep, it's not a problem per se - it's just confusing if you think of them as proper, in-dependent sub-systems. Howsoever that might be an acceptable trade-off to make in order to get the other advantages. I'd love to get berdirs thoughts on this as well though.

During the hard problems discussion today, we agreed on moving to a Core/Field sub-system. -> See https://docs.google.com/document/d/1pgDY-RaTbajNqXm-oj8GdzPD_fPvLb2jhnrt... for the discussion notes.

Will probably start to address this after #2023563: Convert entity field types to the field_type plugin is in. Might make sense to do it per plugin type as it means moving a lot of plugin classes around.

Given that we agreed on doing this and it's an approved API change, adding to the meta issue and changing to major.

I will work on this one after #2023563: Convert entity field types to the field_type plugin and #2094003: Remove use statements in formatters, widgets and field types plugins are in.

And those got in!

So what this does is move the 3 plugin managers, the interface and base classes to Drupal\Core\Field and changes the plugin path for formatters and widgets. The result is this: "91 files changed, 271 insertions(+), 280 deletions(-)"

Haven't moved the field type plugin directory yet, that's going to be huge as well, and not yet sure what to do with all those config and legacy classes...

Tests seem to do pretty well, let's see...

I'm wondering why we need the new subdirs to be Field\FieldWidget(/Formatter/Type). I think we can either drop the first 'Field' and have 'FieldWidget' as subdir, or keep them as before, 'Field\Widget'. Views uses the same pattern, they don't add a 'views_' prefix to their subdirs..

No strong opinion on discovery folder paths

How about just a preference then? :) Basically, keep 'Field\' or not?

And now a review:

1. +++ b/core/core.services.yml
   @@ -164,8 +171,14 @@ services:
      plugin.manager.entity.field.field_type:
   

   Shouldn't we remove the 'entity' part from this service id?

2. +++ b/core/modules/email/lib/Drupal/email/Plugin/Field/FieldFormatter/MailToFormatter.php
   @@ -21,10 +21,10 @@
   +class MailToFormatter extends \Drupal\Core\Field\FormatterBase {
   

   This doesn't look right.. and more like it thorughout the patch.

3. +++ b/core/modules/field/field.module
   @@ -100,6 +100,8 @@ function field_help($path, $arg) {
   +      debug($field_widgets);
   +      debug($field_types);
   

   :/

4. +++ b/core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Plugin/Field/FieldFormatter/TestFieldDefaultFormatter.php
   @@ -25,7 +25,7 @@
   +class TestFieldDefaultFormatter extends \Drupal\Core\Field\FormatterBase {
   

   I think a grep for "extends \Drupal\Core\Field" should reveal them all.

5. +++ b/core/modules/link/lib/Drupal/link/Plugin/Field/FieldFormatter/LinkFormatter.php
   @@ -36,7 +36,7 @@ class LinkFormatter extends FormatterBase {
   +    $elements = \Drupal\Core\Field\parent::settingsForm($form, $form_state);
   

   lol, missing test coverage much?

6. +++ b/core/modules/link/lib/Drupal/link/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -79,7 +79,7 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    $elements = \Drupal\Core\Field\parent::settingsForm($form, $form_state);
   

   Apparently, yep.

keep 'Field\' or not?

I can see going one of two ways on that:

1) We adopt a convention that says that the top-level subdirectory of Plugin should always be the (UpperCamelCased) name of the module or component that defines the type. So block module can define block plugins to be in Plugin/Block, and the ImageToolkit component can define image toolkit plugins to be in Plugin/ImageToolkit. However, since FieldType, FieldFormatter, and FieldWidget are not module or component names, we should locate them in Plugin/Field/FieldType, Plugin/Field/FieldFormatter, and Plugin/Field/FieldWidget. This would mean that we should also move Plugin/ImageEffect to Plugin/Image/ImageEffect, and Plugin/CKEditorPlugin to Plugin/CKEditor/CKEditorPlugin.

2) We adopt a looser convention that allows for plugin types that are prefixed by the module/component name to live at the top. In which case, Plugin/FieldType, Plugin/FieldFormatter, Plugin/FieldWidget, Plugin/ImageEffect, and Plugin/CKEditorPlugin are all ok. It presents a theoretical collision if a contrib or custom module wants to name itself image_effect.module, ckeditor_plugin.module, field_type.module, field_formatter.module, or field_widget.module, but we're already used to these kinds of theoretical collision problems for hook naming as well.

My preference is #1, not because I'm worried about the collision for the ones in core, but because I'd like core to set a precedent for contrib to follow, and I'm more worried about the collision problem if contrib modules start putting names they don't fully own directly into Plugin.

xpost

I think we also have option 3) keep the status quo and let this decision solely in the hands of module developers, with a best-practice rule that if you provide only one or two plugin types, it's ok to not require an additional directory (Plugin/ImageEffect, Plugin/CKEditorPlugin), but if you have three or more, like Field and Views, you're encouraged to add the additional directory (Plugin/Field/FieldType, etc.)

Fixed that crazy stuff, rename the service.

Go and decide what you want :)

I might do another move of those, but not more :p Also haven't moved the field_type classes yet, will probably result in another conflict with #2047229: Make use of classes for entity field and data definitions, we might want to do that in a separate issue of wait with this on that issue...

#34 looks good to me. Whether we decide on #31.1 or #33.3, that decision will only affect ImageEffect and CKEditorPlugin, not this patch.

I'd prefer getting this in as-is and moving field types in a separate issue if that needs to wait on #2047229: Make use of classes for entity field and data definitions. I'm tempted to RTBC this, but waiting on bot and final confirmation from amateescu.

My preference is #1, not because I'm worried about the collision for the ones in core, but because I'd like core to set a precedent for contrib to follow, and I'm more worried about the collision problem if contrib modules start putting names they don't fully own directly into Plugin.

Imo, the looser convention is fine and avoids more unnecessary sub-directories. Of course, we've to make sure contribs are properly namespacing plugin names, but then it should work out as it does for module hooks. And as discussed in Prague, there are probably less plugin types than module hooks.

Patch looks good to me, and yes moving on with as it is now seems to be a good idea. Leaving for amateescu to rtbc.

Let's do it then!

Patch no longer applies.

Re-roll powered by git rebase, some other patch already fixed the wrong LegacyFieldTypeDiscoveryDecorator.php Contains line, that's what I get for unrelated changes ;)

Updated issue title to reflect what's actually going on here.

"criticalness"

Updated issue summary.

We need to resolve what happens to field.api.php now that the hooks for widgets and formatters have moved out of the field.module. The original issue summary mentioned moving them to core/modules/system/entity.api.php - obviously we don't what to move them there but also have two files named field.api.php does not make sense. Perhaps the best option might be to leave them where they are for now?

Setting back to needs work for people to think about this and make a decision.

Discussed a bit with @catch and @alexpott in IRC and we have two possible solutions:

1. move those hooks in system.api.php
2. move those hooks in a new core.api.php

The latter brings up the question if .api.php are discovered outside module folders, so it would definitely be a followup, but the first suggestion can be done here.

So core.ap.php or core/api/subsystem.api.php would help to remove several hooks from system.api.php and make it more obvious where they are. I think I prefer this to moving it to system module, so yeah let's open a follow-up for that.

Here's the followup: #2115011: We need a place to put documentation for core subsystem hooks, other than system.api.php

Committed c1027b9 and pushed to 8.x. Thanks!

I'm sure some change notices need updating.

Fixed the @file block during commit:

diff --git a/core/lib/Drupal/Core/Field/Annotation/FieldFormatter.php b/core/lib/Drupal/Core/Field/Annotation/FieldFormatter.php
index 0bd7f44..1ef899e 100644
--- a/core/lib/Drupal/Core/Field/Annotation/FieldFormatter.php
+++ b/core/lib/Drupal/Core/Field/Annotation/FieldFormatter.php
@@ -2,7 +2,7 @@

 /**
  * @file
- * Contains \Drupal\field\Annotation\FieldFormatter.
+ * Contains \Drupal\Core\Field\Annotation\FieldFormatter.
  */

 namespace Drupal\Core\Field\Annotation;

Opened #2115145: Move field type plugins to Plugin/Field/FieldType as a follow up.

Create one record https://drupal.org/node/2119163. It's based on the issue summary.

Ok, updated the existing change notices:

Field type: https://drupal.org/node/2064123/revisions/view/2877871/2889369
Widget: https://drupal.org/node/1796000/revisions/view/2842847/2889377
Formatter: https://drupal.org/node/1805846/revisions/view/2873951/2889385

Updated https://drupal.org/node/2119163.

Updated issue summary.