Problem/Motivation
It's confusing that we now have mostly everything below \Drupal\Core\Entity but some stuff remains in the entity.module
.
Plugins can be implemented in Core components now, so that's not a cause for having the module anymore.
entity.module
also introduces a circular dependency (see #16).
Proposed resolution
Remove entity.module in two steps:
- Move entity display and display modes config entities below
\Drupal\Core\Entity
, making entity module not required (this issue). - Move all entity UI related code and config to
field_ui.module
- #2224395: Move entity UI code to field_ui module and remove entity.module. Whether or notfield_ui
will be renamed in D8 to actually reflect the fact that it really isentity_ui.module
is a separate issue.
This issue takes care of step 1 and makes entity.module already optional. #2224395: Move entity UI code to field_ui module and remove entity.module can do the rest and remove entity module.
Remaining tasks
Both 1 and 2 from above.
User interface changes
None.
API changes
Some code and namespaces of interface are moved around. We'll have to update Entity Display mode related change records to the new namespaces. Changes:
- \Drupal\entity\EntityDisplayModeInterface -> \Drupal\Core\Entity\EntityDisplayModeInterface
- \Drupal\entity\EntityFormModeInterface -> \Drupal\Core\Entity\EntityFormModeInterface
- \Drupal\entity\EntityViewModeInterface -> \Drupal\Core\Entity\EntityViewModeInterface
Original report by @fago
We discussed that in Portland but I noted that we do not even have an issue for it yet:
- It's confusing that we now have mostly everything below \Drupal\Core\Entity but some stuff remains in the module
- Plugins can be implemented in Core components now, so that's not a cause for having the module anymore.
- Do we still need a module as CMI-config owner?
So even if CMI config remains in a module, imo the main API, i.e. the interfaces you type-hint on should be all in the Core component.
Comment | File | Size | Author |
---|---|---|---|
#67 | move-entity-types-2031717-67.patch | 71.39 KB | Berdir |
Comments
Comment #1
fagoSo let's get started with ensuring that all the API is below \Drupal\Core\Entity.. -> #2031725: Move all entity display interfaces to the core component
Comment #2
yched CreditAttribution: yched commentedIf entity.module is removed, it means its config files need to be prefixed by 'system.*.yml'
entity.display.node.page.default
entity.display.node.page.teaser
entity.form_display.node.article.default
entity.form_display.node.page.default
entity.view_mode.node.full
entity.view_mode.node.teaser
become:
system.display.node.page.default
system.display.node.page.teaser
system.form_display.node.article.default
system.form_display.node.page.default
system.view_mode.node.full
system.view_mode.node.teaser
That would be a net regression IMO :-(, those are very much about entities, not about some broad system-wide configuration. It becomes much less clear what those are if we remove 'entity' in the file name.
Or it becomes:
system.entity_display.node.page.default
system.entity_form_display.node.article.default
system.entity_view_mode.node.full
That's beginning to be quite long, and I still think that you don't look for those lost in the middle of system.*
Comment #3
yched CreditAttribution: yched commentedAlso, entity.module contains hooks, routes and controllers for the "view modes" UI. Those make more sense in an entity.module than in a huge catch-all system.module IMO ?
Comment #4
fagoThey do, but that sounds more like an entity-UI module. They are newer than the issue.
I agree that system.display.node.page.default or system.entity_display.node.page.default sucks a lot and is a deal-breaker, however afaik there was talk about having CMI work with core sub-systems as well. Not sure whether this has gone anywhere.
Right now, it seems arbitrary that the display stuff is in the module and makes the role of the module confusing - what is supposed to go in there, and what not?
Comment #5
yched CreditAttribution: yched commentedNo, I think it's fairly consistent IMO ?
Config entity types are supposed to be owned by a module (at least for now, I wasn't aware of plans to have CMI allow sub-systems as owners), so the EntityDisplay, EntityFormDisplay, EntityFormMode, EntityViewMode entity types and their supporting classes are in entity.module - along with the code that maintains them (housekeeping on bundle renames, etc...)
Comment #6
fagoI don't think this is consistent. There is no real reason EntityDisplay Interfaces belong to the entity.module and say EntityStorage Interfaces not. It's just that entity display happens to need CMI somewhere what requires us to do it.
Comment #7
yched CreditAttribution: yched commentedAgreed - but that constraint is real currently, what I'm saying is that within that constraint, the code layout has a consistency :-)
Comment #8
tstoeckler@yched and I talked about view/form modes at DrupalCamp Vienna. @yched proposed that we implement a similar architecture to base fields vs. configurable fields. In other words:
Apart from having the above concerns about entity.module introducing the concept of view/form modes @yched brought up that as the author of an entity type you want to be able to rely on certain view/form modes of the entity type in your module because there is business logic attached to that. We could for example provide a full-page view/form mode for all entity types by default (this is just an example, I haven't thought about this in detail). And then node.module could add the 'teaser' view mode in-code. And user.module could add the 'register' form mode in-code as well.
Notes:
* We didn't specifically talk about splitting entity.module into entity.module and entity_ui.module but I think it makes a lot of sense and would be consistent with field.module/field_ui.module as seen above.
* Entity displays are a whole different matter. I was originally going to propose that we specify entity view/form display information arrays in the entity class instead of the view/form modes. That would support the use-case of having business logic around the placement and display of certain fields in certain view modes, which makes sense IMO. But then we have to account for additional fields being there due to field_ui.module, so it seems like that's a can of worms.
Comment #9
yched CreditAttribution: yched commentedSo, yeah, +1 on that plan :-)
Just, having an entity_ui.module that just provides the admin pages to add views modes sounds overkill, adds clutter in the "module admin" page for little added value.
field_ui is already currently an "entity UI" at this point anyway (aside from dealing with configurable fields, it's also the place where you edit EntityDisplays), so we might as well move those "view mode" pages in field_ui. Whether we do rename field_ui to entity_ui at some point, is, well, to be debated separately :-)
Other than that, yeah, EntityDisplays should really stay in config land IMO.
Once #1988612: Apply formatters and widgets to rendered entity base fields, starting with node.title is in, The FieldDefintions for base fields will need to be able to specify "default display/form settings" (default formatter, default weight...), a bit like "extra fields" currently do in hook_field_extra_fields(), but I don't see full EntityDisplays being specified in code.
Comment #10
fagoOne could use default config for that as well, not?
Else, yeah the plan makes a lot of sense. I agree with #9 though that we shouldn't split things further up, but move it to field-ui instead of entity-ui.
#2031725: Move all entity display interfaces to the core component got committed, so we could move on here with moving everything UI related from entity.module into field_ui.module. Then, maybe we could add least improve the field_ui module description to clarify it's about entity & field UI ?
Comment #11
yched CreditAttribution: yched commentedview modes / form modes:
Yes, view modes were moved entirely to config land earlier in the cycle, but that's washy. The moment node module has a, say, "node_view($node, 'rss')" somewhere, it assumes the existence of the view mode, it's not "configuration". Likewise (worse) for user 'register' form mode.
Comment #12
fagoIt happens often that you have code that refers to some default config, that's why d7 default config solutions do not allow deleting defaults, but only reverting. Not sure what's or whether there is a CMI solution to that?
Comment #13
yched CreditAttribution: yched commentedbecause in D7 those defaults live in code in hook_default_xxx() rather than in the config tables.
That's what we should have here :-)
IIRC, the issue that moved view modes to config considered having a 'locked' property in the CMI files for the view modes, but it doesn't seem to be there currently.
Also, it would at most be a hint for the UI. Nothing prevents you from actually deleting the config entry / yaml file. $field / $instance have a 'locked' property too for "business logic fields where you want the flexibility of UI / storage / widgets / formatters", that always felt brittle IMO.
If something shouldn't be treated as config, we can put in config and then try to add safeguards, but it seems more reasonable to not put in config in the first place :-)
Comment #14
tstoecklerWell there's #2084567: Implement a EntityLockedInterface and service to allow locking entities and currently a bunch of config entities implement a locking mechanism manually.
I agree with yched, however, that conceptually that this is (better: can be) in-code information vs. in-configuration.
Comment #15
fagoIf locking isn't enough, I guess a hook for defining default entities would solve that - but that really duplicates the CMI solution for defaults then.
I don't see this situation from being any different from having a e.g. a view you code with - so finding a CMI solution would be probably worthwhile. Maybe, locking could be considered reliable enough if CMI enforces it itself. Howsoever, I'm not sure why we are discussing this in this issue - should we open a separate one or is there one already?
Comment #16
sun+1 in general, because entity.module causes circular module dependencies.
IMHO, removing the circular dependencies should be the primary driver and motivation for this issue/effort. If those still exist afterwards, then we didn't really gain something aside from different class namespaces.
The main reasons for why CMI requires a module are
.\Entity\
(1) is no longer an issue, I guess.
For (2), I guess we could introduce a facility to allow core subsystems to expose themselves (their plugins) with a custom provider name + hard-code the core subsystem config names into CMI as "do not check, always installed"?
A custom configuration file name prefix is supported already; part of the @EntityType annotation.
Comment #17
tim.plunkettRegardless of whether we want to remove part of the entity.module or move it or whatever, we should keep part of that as entity_ui.module
Comment #18
yched CreditAttribution: yched commented@tim: this can move to field_ui.module as well.
Given what it currently does, field_ui de facto is an entity_ui already (UI for configurable fields + EntityDisplay objects). Whether it will actually get renamed in D8 is another issue :-), but there's little benefit in adding a separate *_ui module just for the view modes UI IMO.
Comment #19
scor CreditAttribution: scor commentedJust talked to effulgentsia about this: being able to have config entities owned by sub-systems rather than just by modules would also help split the RDF module into generic RDF functionality (moved to Drupal/Core/Rdf) and its serialization in HTML (rdf.module renamed to rdfa.module).
Comment #20
sunComment #21
tim.plunkettTagging, at least for the entity_ui part.
Comment #22
fagoOpened #2224395: Move entity UI code to field_ui module and remove entity.module for getting started with the easy part first.
Comment #23
catchComment #24
dixon_Updated the issue summary to reflect the latest discussion/consensus from this issue and Berdir's comment here: #1688162-8: Replace required flag of modules with proper dependencies.
Comment #25
xjmComment #26
BerdirSo we can now actually have entity types in Drupal\Core\Entity...
Comment #28
BerdirMoving required hook implementations to places where they are always called, move all the controllers, links and stuff on form and view mode to entity_entity_type_alter().
Comment #30
BerdirFixing mistake in the display rename method, fixing form classes, and enabling entity.module in standard.profile.
Comment #31
andypostI think this needs follow-up to move entity tests to core.
Just minor nitpics
Any reason in this *Entity\* prefix?
suppose this needs clean-up too
awesome!
Comment #32
catchComment #33
fagoThe patch looks great. Another remark:
Misses visibility.
Not sure about the issue rename, does it mean removing is left-over to #2224395: Move entity UI code to field_ui module and remove entity.module ?
Comment #34
fagoYeah, we need it as entities have to be defined below an "Entity" namespace. Attached patch addresses point 2. and my own remark.
Comment #35
fagoComment #36
fagoupdate the summary.
Comment #37
fagoComment #41
BerdirRe-roll.
Comment #42
BerdirCross-RTBC'ing our changes, the changes in #34 look fine to me and are minor.
Comment #43
swentel CreditAttribution: swentel commentedLet's ship it.
Comment #44
sunThis looks very weird — why are we moving
EntityFormDisplay
into a "Entity" sub-namespace of the "Entity" namespace?Comment #45
andypostOnly a typo in doc-block left and suppose change record is needed
This shortcut still useful
Still don't get the reason to not include this to use clause
the same
typo
Comment #46
Berdir@sun: Because Plugin discovery, see #34.
@andypost: Oh, I thought you meant the discovery thing, as sun did. You didn't. That should be fixed.
One thing to consider here are the entity type ID's of form_mode and view_mode, they are just form_mode/view_mode, and not entity_view_mode/entity_form_mode. That was maybe less a problem when it was in entity.module but it was already a global ID then. So arguably a separate problem/issue?
Comment #47
sunOh, it wasn't clear to me that these are actual Entity type plugin files. That's a bit confusing, but now it makes sense, OK. :)
@andypost cross-posted with some minor issues. I agree that we should add proper use statements.
Comment #48
andypostonce this changed in the issue, let's unify entity names with their config prefixes
Comment #49
Berdir1. Not sure what you meant exactly, but I refactored it to use entity query and no the storage directly for deleting.
2. & 3. & 4. Fixed.
About the entity_type names, agreed that they should be consistent, but I think I'd prefer a different issue for this. It affects different code path and I made the config prefix consistent, so we don't have to change configuration files if we rename them. If others prefer to unify them here, I'll update the patch.
Comment #50
yched CreditAttribution: yched commented+1 for 'entity_form_mode' / 'entity_view_mode', and a followup seems fine IMO.
Comment #52
BerdirWell, that refactoring didn't went very well, so I did some more ;)
Also, while waiting on CommentFieldsTest to complete, I renamed view_mode and form_mode entity types, looks like we all agree on the name. Probably missed something, but there are only very few references about the actual entity type ID's.
On change record, not sure for which parts we need a new change record. The config prefix changes probably, because that's the part people are interacting with (like providing default config for their view/form modes and displays). I don't think we need to talk about the entity type ID changes much.
Comment #54
BerdirForgot the routes, I find it so sad that they only updated the ones that are explicitly mentioned and not list and so on too.
Also had to use getOriginalId() for getting the displays, doesn't matter for delete but it does for rename.
Comment #55
fagoThanks, good improvements.
Not sure why the entity prefix goes away here, I'd think it gives you good context for the config entity names also?
Comment #56
BerdirThe prefix does not exist in HEAD, I added it in the previous patch and removed it again because the entity UI relies on them and ha tests that assume it is "view mode", not "entity view mode". I wasn't sure about changing the tests, because things like page titles there are also just "View mode".
Comment #57
sunLooks great to me.
Comment #58
fagoI see, thanks. I agree that those UI changes should be handled separately (if even).
Comment #59
BerdirRe-roll after #1976158: Rename entity storage/list/form/render "controllers" to handlers went in.
Comment #60
alexpottentity_entity_bundle_rename()
,entity_entity_bundle_delete()
,entity_module_preuninstall()
are still in the entity module. At least the first two can move to the EntityManager.Comment #61
andypostAll functions are removed here, for the rest there's #2319171: Move entity_invoke_bundle_hook() to EntityManager
Comment #63
swentel CreditAttribution: swentel commentedrerolled after node preview went in.
Comment #65
andypostAs explained in #2319171-16: Move entity_invoke_bundle_hook() to EntityManager this would live in EM to delete bundles that provided by other modules then uninstalled
Comment #67
BerdirSimple merge conflict on the use statements.
Comment #71
fagoComment #72
catchCommitted/pushed to 8.x, thanks!
Comment #74
andypostThis needs follow-u to move part of
field.schema.yml
tocore.entity.schema.yml
, isn't it?Comment #75
swentel CreditAttribution: swentel commentedThat's already done in #2325999: Move config schema for Core field types out of field.module
Comment #76
andypostNo,
entity_form_display.*
andentity_view_display.*
still thereComment #77
swentel CreditAttribution: swentel commentedOh right, a couple left yes
Comment #78
andypostFiled patch in new issue #2331627: Move config schema for Core field types out of field.module
Comment #79
larowlanThis needs a change notice
Comment #80
larowlan/me working on it
Comment #81
larowlanDraft change-record here
https://www.drupal.org/node/2335315
Comment #82
BerdirThanks, sorry, forgot about that. Change record published.