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

Files: 
CommentFileSizeAuthor
#36 drupal8.entity-core-comment.36.patch622 bytessun
PASSED: [[SimpleTest]]: [MySQL] 41,295 pass(es).
[ View ]
#23 entity-core.sh_.txt2.05 KBsun
#23 entity.core_.23.patch141.13 KBsun
PASSED: [[SimpleTest]]: [MySQL] 41,240 pass(es).
[ View ]
#21 entity-core.sh_.txt2.05 KBsun
#21 entity.core_.21.patch141.49 KBsun
FAILED: [[SimpleTest]]: [MySQL] 41,217 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#18 entity-core.sh_.txt2.05 KBsun
#18 entity.core_.18.patch142.91 KBsun
FAILED: [[SimpleTest]]: [MySQL] 41,153 pass(es), 9 fail(s), and 0 exception(s).
[ View ]
#18 entity.core_.post-script.18-do-not-test.patch8.02 KBsun
#16 entity.core_.16.patch138.34 KBsun
FAILED: [[SimpleTest]]: [MySQL] 41,150 pass(es), 16 fail(s), and 0 exception(s).
[ View ]
#16 entity.core_.post-script.16-do-not-test.patch3.13 KBsun
#14 entity-core.sh_.txt1.99 KBsun
#14 entity.core_.14.patch137.65 KBsun
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#14 entity.core_.post-script.11-do-not-test.patch2.43 KBsun
#11 entity.core_.11.patch135.38 KBsun
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#11 entity-core.sh_.txt1.59 KBsun
#11 entity.core_.post-script.11-do-not-test.patch2.43 KBsun
#10 entity is missing.png47.66 KBcosmicdreams
#5 framework.entity.4.patch162.83 KBsun
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#3 framework.entity.2.patch162.83 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch framework.entity.2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#1 framework.entity.1.patch162.49 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch framework.entity.1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new162.49 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch framework.entity.1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new162.83 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch framework.entity.2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Sorry, forgot a commit.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new162.83 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

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.

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.

StatusFileSize
new47.66 KB

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

Status:Needs work» Needs review
StatusFileSize
new2.43 KB
new1.59 KB
new135.38 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

+++ 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! :(

Status:Needs work» Needs review
StatusFileSize
new2.43 KB
new137.65 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
new1.99 KB

Fixed the script.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new3.13 KB
new138.34 KB
FAILED: [[SimpleTest]]: [MySQL] 41,150 pass(es), 16 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new8.02 KB
new142.91 KB
FAILED: [[SimpleTest]]: [MySQL] 41,153 pass(es), 9 fail(s), and 0 exception(s).
[ View ]
new2.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.

+++ 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

Status:Needs work» Needs review
StatusFileSize
new141.49 KB
FAILED: [[SimpleTest]]: [MySQL] 41,217 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
new2.05 KB

Thanks! Attached patch should fix those.

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

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!

Status:Needs work» Needs review
StatusFileSize
new141.13 KB
PASSED: [[SimpleTest]]: [MySQL] 41,240 pass(es).
[ View ]
new2.05 KB

Assigned:Unassigned» fago

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

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.

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.

Assigned:Unassigned» catch

Assigning as per #26.

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.

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

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

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.

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.

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

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?

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.

Status:Active» Needs review
StatusFileSize
new622 bytes
PASSED: [[SimpleTest]]: [MySQL] 41,295 pass(es).
[ View ]

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.

Status:Needs review» Reviewed & tested by the community

Comment move looks good :D

Priority:Critical» Normal

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

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.