Widgets and Formatters are now agnostic as to whether they are applied to field.module fields or base fields, so their plugin managers should move to a Core/Field system, they have no business in field.module anymore.
The FieldType plugin manager has moved to the entity system as #1969728: Implement Field API "field types" as TypedData Plugins progressed, but
- its discovery still looks for classes in 'Plugin/field/field_type' folders,
- its alter hook is still hook_field_info_alter()
API changes
- the field_type plugin manager moves to the Core/Field system (from Core/Entity/Field)
- service ids for the field_type, widget and formatter plugin manager change
- folders for discovery of widget, formatter, field type plugins change
- cache.field has moved to the core caches
Note about "criticalness" (sorry, not a word):
This is mostly a matter of assigning proper scope and responsibility for the "widgets / formatters" system (field.module is now only about handling "configurable fields", and widgets / formatters will be used on other kinds of fields). It can probably not be considered "major" nor "critical" - and is currently not filed so.
Although, one of the options considered in #2052135: Determine how to deal with field types used in base fields in core entities involves placing basic field types + their widgets & formatters in lib/Drupal/Core/Entity.
-> would result in Core holding plugin implementations for a plugin type defined by a module - not too clean, but, again, workable (field.module is required anyway)
Comment | File | Size | Author |
---|---|---|---|
#39 | there-are-too-many-moves-2050835-39.patch | 130.17 KB | Berdir |
#34 | there-are-too-many-moves-2050835-34.patch | 130.85 KB | Berdir |
#34 | there-are-too-many-moves-2050835-34-interdiff.txt | 32.83 KB | Berdir |
#25 | there-are-too-many-moves-2050835-25.patch | 111.14 KB | Berdir |
Comments
Comment #1
yched CreditAttribution: yched commentedclearer title
Comment #2
yched CreditAttribution: yched commentedtagging
Comment #3
tim.plunkettYou 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\
Comment #4
yched CreditAttribution: yched commentedresetting stuff :-)
Comment #5
plachShouldn't we put entity field stuff in:
Drupal\Core\Entity\Field\Widget\
Drupal\Core\Entity\Field\Formatter\
?
Comment #6
tim.plunkett@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
Comment #7
effulgentsia CreditAttribution: effulgentsia commentedHow 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
.Comment #7.0
yched CreditAttribution: yched commentedminor
Comment #8
yched CreditAttribution: yched commentedFully agreed with both points in #7.
"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 ?
Comment #9
yched CreditAttribution: yched commentedAdded considerations about cache bins and alter hooks in the OP.
Comment #10
amateescu CreditAttribution: amateescu commentedThis 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.
Comment #11
yched CreditAttribution: yched commentedWell, the annotation classes are already FieldWidget, FieldFormatter
But that possibly means hook_entity_field_widget_info_alter(), hook_entity_field_formatter_info_alter().
Comment #12
amateescu CreditAttribution: amateescu commentedYes, that's what I was hinting at :)
Comment #13
effulgentsia CreditAttribution: effulgentsia commented@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.
Comment #13.0
effulgentsia CreditAttribution: effulgentsia commentedadded tasks about cache bins and alter hooks
Comment #13.1
yched CreditAttribution: yched commentedAPI changes
Comment #13.2
yched CreditAttribution: yched commentedAPI change
Comment #14
yched CreditAttribution: yched commentedNote for core committers:
This is mostly a matter of assigning proper scope and responsibility for the "widgets / formatters" system (field.module is now only about handling "configurable fields", and widgets / formatters will be used on other kinds of fields). It can probably not be considered "major" nor "critical" - and is currently not filed so.
Although, one of the options considered in #2052135: Determine how to deal with field types used in base fields in core entities involves placing basic field types + their widgets & formatters in lib/Drupal/Core/Entity.
-> would result in Core holding plugin implementations for a plugin type defined by a module - not too clean, but again, workable (field.module is required anyway)
(I'll add that to the issue summary)
Comment #15
Dries CreditAttribution: Dries commentedDiscussed this with Alex Pott, effulgentsia and xjm and we all believe the side-effects of this API change are very small. Approved.
Comment #16
yched CreditAttribution: yched commentedSo, this has been approved, we just need to agree on discovery folders then.
The more I think of this, the more I like @effulgentsia's suggestion to move Field as a subsystem in /Core/Field, instead of just a /Core/Entity/Field subfolder.
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.
We do have a "field system" component in the issue queue, and it also makes things clearer when it comes to identifying maintainers.
Would result in:
Three remarks:
- I think it's best to keep discovery folders the same name as the annotation plugin class. Those are currently @FieldType, @FieldFormatter, and I don't see us renaming them to just @Type, @Formatter... - so the above keeps Field/FieldFormatter etc. as discovery folders
- This results in two subsystems (Core/Entity, Core/Field) depending on each other. Don't know if that's a real issue though - I don't think we'll ever be able to fully decouple entity and field anyway...
- who "owns" hook_field_formatter_info_alter() and documents it in its .api.php file ? entity.module ? or maybe then they simply stay in field.module ?
Comment #17
yched CreditAttribution: yched commentedBump. Any feedback on the proposal in #16 ?
I pinged @fago by email, other opinions welcome - @Berdir ? core committers ?
Comment #18
fagoI'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.Imho that could work out with Core/Entity/Field as well.
Comment #19
yched CreditAttribution: yched commentedYes, 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.
The issue I see with Field being just a subfolder of Core/Entity is that
- supporting code (plugin managers, annotation classs, base classses, interfaces) would be in Core/Entity/Field
- but the field types, formatters, widgets we'd want to ship in Core could not be Core/Entity/Field/Plugin, because the plugin discovery can only scan Core/*/Plugin. So they'd have to be in Core/Entity/Plugin, and then field stuff gets scattered in several places in Core/Entity.
It would also clarify the current messy situation where we have hooks about field, entity_field...
Comment #20
fagoYep, 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.
Comment #21
fagoDuring 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.
Comment #22
yched CreditAttribution: yched commentedYup - meaning the rename plan to implement is the one in #16
Comment #23
BerdirWill 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.
Comment #24
BerdirI 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.
Comment #25
BerdirAnd 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...
Comment #26
amateescu CreditAttribution: amateescu commentedI'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..
Comment #27
yched CreditAttribution: yched commentedNo strong opinion on discovery folder paths, but strong +1 on eeping the last dir named like the annotation class (FieldWidget / FieldFormatter)
Comment #28
yched CreditAttribution: yched commented+ : cannot review atm, but thanks a lot @Berdir for attacking this !
Comment #29
amateescu CreditAttribution: amateescu commentedHow about just a preference then? :) Basically, keep 'Field\' or not?
Comment #30
amateescu CreditAttribution: amateescu commentedAnd now a review:
Shouldn't we remove the 'entity' part from this service id?
This doesn't look right.. and more like it thorughout the patch.
:/
I think a grep for "extends \Drupal\Core\Field" should reveal them all.
lol, missing test coverage much?
Apparently, yep.
Comment #31
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #32
effulgentsia CreditAttribution: effulgentsia commentedxpost
Comment #33
amateescu CreditAttribution: amateescu commentedI 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.)
Comment #34
BerdirFixed 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...
Comment #35
effulgentsia CreditAttribution: effulgentsia commented#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.
Comment #36
fagoImo, 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.
Comment #37
amateescu CreditAttribution: amateescu commentedLet's do it then!
Comment #38
alexpottPatch no longer applies.
Comment #39
BerdirRe-roll powered by git rebase, some other patch already fixed the wrong LegacyFieldTypeDiscoveryDecorator.php Contains line, that's what I get for unrelated changes ;)
Comment #40
amateescu CreditAttribution: amateescu commentedUpdated issue title to reflect what's actually going on here.
Comment #40.0
amateescu CreditAttribution: amateescu commented"criticalness"
Comment #40.1
amateescu CreditAttribution: amateescu commentedUpdated issue summary.
Comment #41
alexpottWe 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.
Comment #42
amateescu CreditAttribution: amateescu commentedDiscussed a bit with @catch and @alexpott in IRC and we have two possible solutions:
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.
Comment #43
catchSo 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.
Comment #44
amateescu CreditAttribution: amateescu commentedHere's the followup: #2115011: We need a place to put documentation for core subsystem hooks, other than system.api.php
Comment #45
alexpottCommitted c1027b9 and pushed to 8.x. Thanks!
I'm sure some change notices need updating.
Fixed the @file block during commit:
Comment #46
effulgentsia CreditAttribution: effulgentsia commentedOpened #2115145: Move field type plugins to Plugin/Field/FieldType as a follow up.
Comment #47
smiletrl CreditAttribution: smiletrl commentedCreate one record https://drupal.org/node/2119163. It's based on the issue summary.
Comment #48
BerdirOk, 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.
Comment #48.0
BerdirUpdated issue summary.