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:

  1. Move entity display and display modes config entities below \Drupal\Core\Entity, making entity module not required (this issue).
  2. 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 not field_ui will be renamed in D8 to actually reflect the fact that it really is entity_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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fago’s picture

So 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

yched’s picture

If 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.*

yched’s picture

Also, 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 ?

fago’s picture

They 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?

yched’s picture

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?

No, 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...)

fago’s picture

I 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.

yched’s picture

It's just that entity display happens to need CMI somewhere what requires us to do it.

Agreed - but that constraint is real currently, what I'm saying is that within that constraint, the code layout has a consistency :-)

tstoeckler’s picture

@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:

  • Entity classes can provide view/form modes in a view/formModes() method (this is the equivalent of baseFieldDefinitions())
  • We provide a hook_entity_view/form_mode_info() (this is the equivalent of hook_entity_field_info())
  • entity.module provides configurable view/form modes as config entities (this is the equivalent of field.module providing fields as config entities)
  • entity_ui.module provides the UI to create/edit/delete view/form modes (this is the equivalent of field_ui.module providing the UI to create/edit/delete fields)

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.

yched’s picture

So, 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.

fago’s picture

Entity classes can provide view/form modes in a view/formModes() method (this is the equivalent of baseFieldDefinitions())

One 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 ?

yched’s picture

view 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.

fago’s picture

It 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?

yched’s picture

d7 default config solutions do not allow deleting defaults, but only reverting

because in D7 those defaults live in code in hook_default_xxx() rather than in the config tables.
That's what we should have here :-)

Not sure what's or whether there is a CMI solution to that?

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 :-)

tstoeckler’s picture

Well 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.

fago’s picture

If 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?

sun’s picture

+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

  1. to place and register the entity type plugins themselves. → .\Entity\
  2. to verify whether the $owner/$provider module is installed so configuration can be imported.

(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.

tim.plunkett’s picture

Regardless 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

yched’s picture

@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.

scor’s picture

Issue summary: View changes

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"?

Just 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).

sun’s picture

tim.plunkett’s picture

Issue tags: +rename module

Tagging, at least for the entity_ui part.

fago’s picture

Opened #2224395: Move entity UI code to field_ui module and remove entity.module for getting started with the easy part first.

catch’s picture

Priority: Normal » Major
Issue tags: +beta target
dixon_’s picture

Issue summary: View changes

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.

xjm’s picture

Issue tags: +beta deadline
Berdir’s picture

Status: Active » Needs review
FileSize
39.07 KB

So we can now actually have entity types in Drupal\Core\Entity...

Status: Needs review » Needs work

The last submitted patch, 26: move-entity-types-2031717-26.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
49.45 KB
14.45 KB

Moving 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().

Status: Needs review » Needs work

The last submitted patch, 28: move-entity-types-2031717-28.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
49.78 KB
2.07 KB

Fixing mistake in the display rename method, fixing form classes, and enabling entity.module in standard.profile.

andypost’s picture

I think this needs follow-up to move entity tests to core.
Just minor nitpics

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
    @@ -9,7 +9,6 @@
    -use Drupal\entity\Entity\EntityFormDisplay;
    
    @@ -77,7 +76,7 @@ protected function init(FormStateInterface $form_state) {
    -    $form_display = EntityFormDisplay::collectRenderDisplay($this->entity, $this->getOperation());
    +    $form_display = Entity\EntityFormDisplay::collectRenderDisplay($this->entity, $this->getOperation());
    

    Any reason in this *Entity\* prefix?

  2. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
    @@ -2,15 +2,14 @@
    -use Drupal\entity\EntityDisplayBase;
    
    @@ -19,14 +18,13 @@
    -class EntityFormDisplay extends EntityDisplayBase implements EntityFormDisplayInterface {
    +class EntityFormDisplay extends \Drupal\Core\Entity\EntityDisplayBase implements EntityFormDisplayInterface {
    

    suppose this needs clean-up too

  3. +++ b/core/modules/entity/entity.info.yml
    @@ -4,4 +4,3 @@ description: 'Generic entity functionality.'
    -required: true
    
    +++ b/core/profiles/standard/standard.info.yml
    @@ -19,6 +19,7 @@ dependencies:
    +  - entity
    

    awesome!

catch’s picture

Title: Remove entity.module - once again. » Make entity module not required
fago’s picture

The patch looks great. Another remark:

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBundleBase.php
@@ -18,6 +20,60 @@
+  function deleteDisplays() {

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 ?

fago’s picture

Any reason in this *Entity\* prefix?

Yeah, we need it as entities have to be defined below an "Entity" namespace. Attached patch addresses point 2. and my own remark.

fago’s picture

Issue summary: View changes
fago’s picture

update the summary.

fago’s picture

Issue tags: +Drupalaton 2014

fago queued 34: d8_entity_module.patch for re-testing.

Berdir queued 34: d8_entity_module.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 34: d8_entity_module.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
50.17 KB

Re-roll.

Berdir’s picture

Cross-RTBC'ing our changes, the changes in #34 look fine to me and are minor.

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Let's ship it.

sun’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
-    $form_display = EntityFormDisplay::collectRenderDisplay($this->entity, $this->getOperation());
+    $form_display = Entity\EntityFormDisplay::collectRenderDisplay($this->entity, $this->getOperation());

.
rename from core/modules/entity/src/Entity/EntityFormDisplay.php
rename to core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php

This looks very weird — why are we moving EntityFormDisplay into a "Entity" sub-namespace of the "Entity" namespace?

andypost’s picture

Only a typo in doc-block left and suppose change record is needed

  1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBundleBase.php
    @@ -18,6 +20,60 @@
    +  protected function deleteDisplays() {
    ...
    +    entity_delete_multiple('entity_view_display', $ids);
    ...
    +    entity_delete_multiple('entity_form_display', $ids);
    

    This shortcut still useful

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
    @@ -77,7 +76,7 @@ protected function init(FormStateInterface $form_state) {
    -    $form_display = EntityFormDisplay::collectRenderDisplay($this->entity, $this->getOperation());
    +    $form_display = Entity\EntityFormDisplay::collectRenderDisplay($this->entity, $this->getOperation());
    

    Still don't get the reason to not include this to use clause

  3. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
    @@ -10,14 +10,13 @@
    -use Drupal\entity\Entity\EntityViewDisplay;
    +use Drupal\Core\Entity\Entity\EntityViewDisplay;
    
    @@ -393,7 +392,7 @@ public function viewField(FieldItemListInterface $items, $display_options = arra
    -      $display = EntityViewDisplay::collectRenderDisplay($entity, $view_mode);
    +      $display = Entity\EntityViewDisplay::collectRenderDisplay($entity, $view_mode);
    

    the same

  4. +++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
    @@ -251,7 +251,7 @@ public function postSave(EntityStorageInterface $storage, $update = TRUE) {
    -   * {@inheritdoc}
    +   * {@inheritdoc}NodeTy
    

    typo

Berdir’s picture

@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?

sun’s picture

we need it as entities have to be defined below an "Entity" namespace.

Oh, 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.

andypost’s picture

+++ b/core/config/schema/core.entity.schema.yml
@@ -1,6 +1,6 @@
-entity.view_mode.*.*:
+core.entity_view_mode.*.*:

@@ -29,7 +29,7 @@ entity.view_mode.*.*:
-entity.form_mode.*.*:
+core.entity_form_mode.*.*:

@@ -47,7 +47,7 @@ entity.form_mode.*.*:
-entity.view_display.*.*.*:
+core.entity_view_display.*.*.*:

@@ -79,7 +79,7 @@ entity.view_display.*.*.*:
-entity.form_display.*.*.*:
+core.entity_form_display.*.*.*:

+++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
@@ -19,7 +19,6 @@
  *   id = "entity_form_display",
...
- *   config_prefix = "form_display",

+++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormMode.php
@@ -26,27 +26,14 @@
  *   id = "form_mode",
...
+ *   config_prefix = "entity_form_mode",

+++ b/core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php
@@ -19,10 +19,6 @@
  *   id = "entity_view_display",
...
- *   config_prefix = "view_display",

+++ b/core/lib/Drupal/Core/Entity/Entity/EntityViewMode.php
@@ -32,22 +32,10 @@
  *   id = "view_mode",
...
+ *   config_prefix = "entity_view_mode",

once this changed in the issue, let's unify entity names with their config prefixes

Berdir’s picture

FileSize
48.51 KB
4.2 KB

1. 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.

yched’s picture

+1 for 'entity_form_mode' / 'entity_view_mode', and a followup seems fine IMO.

Status: Needs review » Needs work

The last submitted patch, 49: move-entity-types-2031717-49.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
59.23 KB
18.38 KB

Well, 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.

Status: Needs review » Needs work

The last submitted patch, 52: move-entity-types-2031717-52.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
70.26 KB
17.02 KB

Forgot 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.

fago’s picture

Thanks, good improvements.

+++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormMode.php
@@ -29,7 +29,7 @@
- *   label = @Translation("Entity form mode"),
+ *   label = @Translation("Form mode"),

+++ b/core/lib/Drupal/Core/Entity/Entity/EntityViewMode.php
@@ -31,7 +31,7 @@
- *   label = @Translation("Entity view mode"),
+ *   label = @Translation("View mode"),

Not sure why the entity prefix goes away here, I'd think it gives you good context for the config entity names also?

Berdir’s picture

The 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".

sun’s picture

Status: Needs review » Reviewed & tested by the community

Looks great to me.

fago’s picture

I wasn't sure about changing the tests, because things like page titles there are also just "View mode".

I see, thanks. I agree that those UI changes should be handled separately (if even).

Berdir’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

entity_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.

andypost’s picture

Status: Needs work » Reviewed & tested by the community
+++ b/core/modules/entity/entity.module
@@ -46,70 +45,26 @@ function entity_permission() {
-function entity_entity_bundle_rename($entity_type_id, $bundle_old, $bundle_new) {
...
-function entity_entity_bundle_delete($entity_type_id, $bundle) {
...
-function entity_module_preuninstall($module) {

All functions are removed here, for the rest there's #2319171: Move entity_invoke_bundle_hook() to EntityManager

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 59: move-entity-types-2031717-59.patch, failed testing.

swentel’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
71.4 KB
1.15 KB

rerolled after node preview went in.

andypost’s picture

+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
@@ -942,7 +942,19 @@ public function uninstall(array $module_list, $uninstall_dependents = TRUE) {
+      // Clean up all entity bundles (including field instances) of every entity
+      // type provided by the module that is being uninstalled.
+      foreach ($entity_manager->getDefinitions() as $entity_type_id => $entity_type) {

As 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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 63: move-entity-types-2031717-63.patch, failed testing.

Berdir’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
71.39 KB

Simple merge conflict on the use statements.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 67: move-entity-types-2031717-67.patch, failed testing.

Status: Needs work » Needs review
fago’s picture

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

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

  • catch committed 30cad25 on 8.0.x
    Issue #2031717 by Berdir, swentel, fago: Make entity module not required...
andypost’s picture

This needs follow-u to move part of field.schema.yml to core.entity.schema.yml, isn't it?

swentel’s picture

andypost’s picture

No, entity_form_display.*and entity_view_display.* still there

swentel’s picture

Oh right, a couple left yes

andypost’s picture

larowlan’s picture

Issue tags: +Missing change record

This needs a change notice

larowlan’s picture

/me working on it

larowlan’s picture

Status: Fixed » Needs review

Draft change-record here
https://www.drupal.org/node/2335315

Berdir’s picture

Status: Needs review » Fixed
Issue tags: -Missing change record

Thanks, sorry, forgot about that. Change record published.

Status: Fixed » Closed (fixed)

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