Problem

  • Having the entity system in a module causes a circular dependency for other base modules in core.

Details

  • The entity system was moved into entity.module, but that happened before we introduced PSR-0 and OO code in Drupal\Core.
  • The entity system itself is pure API code/infrastructure. It does not have a database schema or anything else attached to it that would require the module system for itself to function.
  • hook_entity_info() will soon most probably be replaced with plugins.
  • The entity system invokes module hooks, but that doesn't prevent itself from being autoloadable and located in Drupal\Core.

Related issues

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Status: Active » Needs review
FileSize
162.49 KB

Essentially these commands:

git mv modules/entity/lib/Drupal/entity/*.php lib/Drupal/Core/Entity/
git mv modules/entity/entity.module includes/entity.inc
git mv modules/entity/lib/Drupal/entity/Tests/* modules/system/lib/Drupal/system/Tests/Entity/
git mv modules/entity/tests/modules/* modules/system/tests/modules/

Then, globally search & replace: (trailing backslash is important)

Drupal\entity\  »  Drupal\Core\Entity\

Lastly,

  1. cat modules/entity/entity.api.php >> modules/system/system.api.php and clean up the resulting artifact:
    /**
     * @} End of "addtogroup hooks".
     */
    <?php
    
    /**
     * @file
     * Hooks provided the Entity module.
     */
    
    /**
     * @addtogroup hooks
     * @{
     */
    

    » We should consider to allow .api.php files in core/includes/ though.

  2. git rm -r modules/entity
  3. Delete entity_help().
  4. Move hook_modules_preenable() and hook_modules_disabled() implementations directly into module.inc.
  5. Add to _drupal_bootstrap_code():
    require_once DRUPAL_ROOT . '/core/includes/entity.inc';
    

Status: Needs review » Needs work

The last submitted patch, framework.entity.1.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
162.83 KB

Sorry, forgot a commit.

Status: Needs review » Needs work

The last submitted patch, framework.entity.2.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
162.83 KB

Merged in HEAD. I so love those typo fix commits. :(

Status: Needs review » Needs work

The last submitted patch, framework.entity.4.patch, failed testing.

catch’s picture

The entity system invokes module hooks, but that doesn't prevent itself from being autoloadable and located in Drupal\Core.

We won't be able to do that properly until the module/hook system is also a core component, but it's no worse than it being a module.

sun’s picture

Issue tags: +Configurables

Another highly important aspect to this is:

Every module that uses Configurables needs to have a dependency on config.module (which in turn has a dependency on entity.module).

Since Configurables are just merely a minor extension of Entities, that entire dependency chain makes even less sense.

The early/original patches in #1668820: Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc) added the Configurable class into Drupal\Core to circumvent that problem, but when it got reworked onto the entity system, that no longer made sense, so the Configurable class was added to config.module instead.

Lastly, there's also a plan to #1763974: Convert entity type info into plugins, which further removes dependencies on procedural code and has been discussed a couple of times already, but didn't exist as an issue yet, so I just created it.

dagmar’s picture

cosmicdreams’s picture

FileSize
47.66 KB

I tried to apply this patch and update my site and was able to get the following feedback :
entity is missing.png

sun’s picture

Status: Needs work » Needs review
FileSize
2.43 KB
1.59 KB
135.38 KB

I've converted the instructions in #1 into a shell script.

Status: Needs review » Needs work

The last submitted patch, entity.core_.11.patch, failed testing.

sun’s picture

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityApiInfoTest.php
@@ -2,10 +2,10 @@
- * Definition of Drupal\entity\Tests\EntityApiInfoTest.
+ * Definition of Drupal\Core\Entity\Tests\EntityApiInfoTest.
...
-namespace Drupal\entity\Tests;
+namespace Drupal\Core\Entity\Tests;

dang! :(

sun’s picture

Status: Needs work » Needs review
FileSize
2.43 KB
137.65 KB
1.99 KB

Fixed the script.

Status: Needs review » Needs work

The last submitted patch, entity.core_.14.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
3.13 KB
138.34 KB

No change to the script; only needed an additional tweak to the post-script patch.

Status: Needs review » Needs work

The last submitted patch, entity.core_.16.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
8.02 KB
142.91 KB
2.05 KB

Manually searched for remaining "'entity'" references and hit a couple of instances that need to be corrected in the post-script patch.

Status: Needs review » Needs work

The last submitted patch, entity.core_.18.patch, failed testing.

tim.plunkett’s picture

+++ b/core/modules/system/tests/modules/entity_test/entity_test.moduleundefined
@@ -11,8 +11,8 @@
-    'entity class' => 'Drupal\entity_test\EntityTest',
-    'controller class' => 'Drupal\entity_test\EntityTestStorageController',
+    'entity class' => 'Drupal\Core\Entity_test\EntityTest',
+    'controller class' => 'Drupal\Core\Entity_test\EntityTestStorageController',
     'base table' => 'entity_test',
     'fieldable' => TRUE,
     'entity keys' => array(
@@ -35,7 +35,7 @@ function entity_test_entity_info() {

@@ -35,7 +35,7 @@ function entity_test_entity_info() {
  * @param bool $reset
  *   A boolean indicating that the internal cache should be reset.
  *
- * @return Drupal\entity_test\EntityTest
+ * @return Drupal\Core\Entity_test\EntityTest
  *   The loaded entity object, or FALSE if the entity cannot be loaded.
  */
 function entity_test_load($id, $reset = FALSE) {
diff --git a/core/modules/entity/tests/modules/entity_test/lib/Drupal/entity_test/EntityTest.php b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/EntityTest.php

diff --git a/core/modules/entity/tests/modules/entity_test/lib/Drupal/entity_test/EntityTest.php b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/EntityTest.php
similarity index 97%
rename from core/modules/entity/tests/modules/entity_test/lib/Drupal/entity_test/EntityTest.php
rename to core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/EntityTest.php
index 6c95f75..e112dcd 100644

index 6c95f75..e112dcd 100644
--- a/core/modules/entity/tests/modules/entity_test/lib/Drupal/entity_test/EntityTest.php

--- a/core/modules/entity/tests/modules/entity_test/lib/Drupal/entity_test/EntityTest.php
+++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/EntityTest.phpundefined

+++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/EntityTest.phpundefined
+++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/EntityTest.phpundefined
@@ -2,14 +2,14 @@

@@ -2,14 +2,14 @@
 
 /**
  * @file
- * Definition of Drupal\entity_test\EntityTest.
+ * Definition of Drupal\Core\Entity_test\EntityTest.
  */
 
-namespace Drupal\entity_test;
+namespace Drupal\Core\Entity_test;
 
 use InvalidArgumentException;
 
-use Drupal\entity\Entity;
+use Drupal\Core\Entity\Entity;
 
 /**
  * Defines the test entity class.
diff --git a/core/modules/entity/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestStorageController.php b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestStorageController.php

diff --git a/core/modules/entity/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestStorageController.php b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestStorageController.php
similarity index 85%
rename from core/modules/entity/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestStorageController.php
rename to core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestStorageController.php
index b31e5ce..6966788 100644

index b31e5ce..6966788 100644
--- a/core/modules/entity/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestStorageController.php

--- a/core/modules/entity/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestStorageController.php
+++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestStorageController.phpundefined

+++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestStorageController.phpundefined
+++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestStorageController.phpundefined
@@ -2,26 +2,26 @@

@@ -2,26 +2,26 @@
 
 /**
  * @file
- * Definition of Drupal\entity_test\EntityTestStorageController.
+ * Definition of Drupal\Core\Entity_test\EntityTestStorageController.
  */
 
-namespace Drupal\entity_test;
+namespace Drupal\Core\Entity_test;

I think this is wrong

sun’s picture

Status: Needs work » Needs review
FileSize
141.49 KB
2.05 KB

Thanks! Attached patch should fix those.

Slight adjustment to the script; same post-script patch from #18.

sun’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/tests/modules/entity_cache_test/entity_cache_test.info
@@ -3,5 +3,4 @@ description = "Support module for testing entity cache."
-dependencies[] = entity_cache_test_dependency

whoopsie!

sun’s picture

Status: Needs work » Needs review
FileSize
141.13 KB
2.05 KB
tim.plunkett’s picture

Assigned: Unassigned » fago

Assigning to fago for his opinion, but I think this is RTBC.

fago’s picture

Assigned: fago » Unassigned
Status: Needs review » Reviewed & tested by the community

Yeah, this totally makes sense to me. I was not able to find any glitches in the patch, thus marking RTBC.

The entity system invokes module hooks, but that doesn't prevent itself from being autoloadable and located in Drupal\Core.

Yep. crell mentioned that moving to symfony events would help us to untangle this dependency on the module system. Being able to load an entity without requiring the module system to be loaded would certainly be a good goal.

catch’s picture

We could also just do #1331486: Move module_invoke_*() and friends to an Extensions class to resolve the dependency issues - i.e. make the module sytem a component that can be dependency injected. I'd like to take a proper look at this before it goes in, so assigning to me, that shouldn't stop review from any other core committers though.

plach’s picture

Assigned: Unassigned » catch

Assigning as per #26.

sun’s picture

There's a difference to #1331486: Move module_invoke_*() and friends to an Extensions class in that the primary problem here is that the entity system functionality is a required dependency for almost every module (which even applies to modules that do not provide "content" entities, but only configurable things now).

However, the entity system itself does not have any requirements other than having to be loaded. It doesn't have a db schema, it doesn't need to be installed, and so on. It doesn't have a UI. In the end, it is just code that is required all over the place, and just needs to be loaded.

Those are the main differences from my perspective. Thus, it doesn't really change anything whether the module/extension system will be able to be bootstrapped earlier or similar - the situation with the entity system is still the same; it's not a module, it's a component.

catch’s picture

Sorry sun, I meant an injectable Extensions class as an alternative (or more doable first step) to wholesale moving to Symfony's events, not as an alternative to moving this back to a component which I'm in favour of (despite having started the 'move it to a module' issue but that was a long time ago).

Lars Toomre’s picture

Reading #28 makes me wonder whether the entity system should be registered in DIC. Am I wrong with that thought?

catch’s picture

There's a very long list of dependencies that would need to be registered in the DIC before the entity system could be. Eventually that would be a good idea but we're a long way off at the moment, we need to tackle issues like #1764474: Make Cache interface and backends use the DIC first.

catch’s picture

Assigned: catch » Unassigned
+++ b/core/includes/module.incundefined
@@ -476,6 +476,7 @@ function module_enable($module_list, $enable_dependencies = TRUE) {
       // Allow modules to react prior to the enabling of a module.
+      entity_info_cache_clear();
       module_invoke_all('modules_preenable', array($module));

This doesn't sit right with the comment, should probably be above it. Also we only added those hooks because they were necessary to allow entity.module - should we remove the hooks too?

It's a bit sad we have to re-introduce knowledge of the entity system to module.inc again with this patch, but we might be able get around that by converting hook_entity_info() to plugins or plugins + config or similar.

Apart from the comment agreed with RTBC, unassigning myself.

sun’s picture

#23: entity.core_.23.patch queued for re-testing.

sun’s picture

For now, I wanted to make sure that the entity info cache clears happen at the right time, so I put them right in front of the hook invocations.

Can we make that line/comment change a follow-up patch?

catch’s picture

Priority: Major » Critical
Status: Reviewed & tested by the community » Active

Yep. Committed/pushed to 8.x. We'll need to delete or strike out [#1287074] so leaving open for that and the comment follow-up.

sun’s picture

Status: Active » Needs review
FileSize
622 bytes

The comment fix.

I've deleted the change notice that told about the entity system being moved into a module. Considered to replace it with something new, but alas, there is factually nothing changed between D7 and D8 with the move forth and back. The other entity system changes are covered in other change notices already.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Comment move looks good :D

sun’s picture

aspilicious’s picture

Priority: Critical » Normal

With the change notice deleted this isn't critical anymore.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed the follow-up. Marking fixed. Thanks!

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