It doesn't really make sense for entity.inc to be a .inc - at the moment, it only holds the entity classes, not the now quite numerous entity helper functions, which are all stuffed in common.inc. Also there isn't a use case for calling entity functions before full bootstrap, the closest is file.inc, which is included by _drupal_bootstrap_full().

I'd like to get this done before we start adding entity_save() and the rest, since that will make it a lot easier to backport the new functions to a Drupal 7 contrib module.

So.. let's make it a module, I also split entity.inc into entity.query.inc and entity.controller.inc

First Drupal git patch, hopefully it won't blow up.

I left system_entity_info() as is, since that's the 'file' system implementing the entity API, which it can't, so system module does it instead. If anywhere I'd rather see this moved to file module or somewhere but that's not for this patch.

Didn't do full sweep of /includes for any lingering entity function calls, but it'd make sense to do so to either add @todos or move them into entity.module before commit.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

FileSize
269.33 KB

Bit of patch cleanup.

Places that will need cleaning up, either with this patch or as followups:

module_enable() calls entity_info_cache_clear() - moved this to hook_modules_enabled() but I'm sure that won't pass tests.

url() has some phpdoc mentioning the entity_type and entity array keys for $options, however there's no code there that depends on the entity system so that's not a true dependency. At worst this could be updated to something more generic like "some calls to url() may add additional metadata in $options, @see entity_uri()".

The only place that calls entity functions is file.inc, which if we're going to mess with where managed files are handled, is going to need to be a separate patch anyway. And actions.inc mentions 'entities', but there's already a patch for that #1008166: Actions should be a module.

sun’s picture

Awesome.

Anonymous’s picture

subscribe.

catch’s picture

FileSize
302.99 KB

Added hunks from system.api.php, no other changes.

fago’s picture

For d8, I'd really like to see the entity API becoming our main data / CRUD api. In that case pretty much any drupal module would require it, so we'd get another always-required module.

Still, I think this is a good idea as it really helps code organization + helps to better decouple it from the various sub-systems. However, then similarly it might be good way to follow for other sub-systems too (e.g. actions).

So we might want to think about introducing the notion of API modules, which could be treated different in the UI.

catch’s picture

Actions issue is here #1008166: Actions should be a module :)

catch’s picture

tagging.

sun’s picture

Status: Needs review » Needs work
+++ modules/entity/entity.info
@@ -0,0 +1,8 @@
+name = Entity ¶

Trailing white-space.

+++ modules/entity/entity.info
@@ -0,0 +1,8 @@
+description = Entity API to add fields to entities like nodes and users.

Sounds odd - Field API is for fields, Entity is for... well, just "handling" entities.

+++ modules/entity/entity.query.inc
@@ -0,0 +1,839 @@
+// $Id$

Id keywords need to be removed.

+++ modules/entity/entity.query.inc
@@ -0,0 +1,839 @@
+class EntityFieldQuery {

I'm not 100% sure whether EFQ really belongs to Entity module - perhaps @fago can provide some feedback on this question?

Powered by Dreditor.

catch’s picture

Priority: Normal » Major
Status: Needs work » Needs review
FileSize
304.23 KB

Fixed sun's comments except for entityFieldQuery - I think it does belong in here.

Bumping to major since this needs to be done before a lot of other entity changes.

Status: Needs review » Needs work

The last submitted patch, entity_module.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
304.24 KB

Adding required and version = 8.x

Status: Needs review » Needs work

The last submitted patch, entity_module.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
306.63 KB

Fixed the installer.

Status: Needs review » Needs work

The last submitted patch, entity_module.patch, failed testing.

podarok’s picture

subscribe

mirocow’s picture

subscribe

Dave Reid’s picture

Subscribe, this could be helpful to provide UUIDs for all entities in D8.

pwolanin’s picture

Sounds like a good approach to get new CRUD/CRAP functions working before ripping out the legacy ones.

pounard’s picture

Sub, this is a really good idea, API will be much more clearer if centralized into such module.

gdd’s picture

Issue tags: +Configuration system

Tagging

tinyrobot’s picture

Subscribe

Dave Reid’s picture

+++ b/modules/user/user.infoundefined
@@ -6,5 +6,6 @@ core = 8.x
+dependencies[] = entity

If entity.module is a required core module, then we shouldn't need to add it as a dependency in other core modules' .info files. I don't think we do that anyway with node/system/user.

Powered by Dreditor.

jherencia’s picture

Subscribing...

catch’s picture

Component: base system » entity system
catch’s picture

Status: Needs work » Needs review
FileSize
311.74 KB

Removed the user module dependency.

Moved entity.controller.inc into an include instead of in the module file.

This is mainly for the bot still, have a feeling there's one or two more test failures to resolve.

Status: Needs review » Needs work

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

catch’s picture

Status: Needs work » Needs review
FileSize
312.2 KB

Should be two fails left, need to look at the entity info cache tests.

Status: Needs review » Needs work

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

catch’s picture

Status: Needs work » Needs review
FileSize
313.52 KB

The last test fails due to the entity_info_cache_clear() in module disable.

That cache has to be cleared prior to installation of a module (so that if the module relies on its entity type during install it's available in the info array).

However there's no hook available here - hook_modules_installed() and hook_modules_enabled() are far too late.

So... I had to add hook_modules_preinstalled() and hook_modules_preenabled(). Only hook_modules_preeenabled() is implemented, but trying to come up with a single hook name was much harder than than mirror hooks for the existing two, and other modules may well benefit from something like this.

 35 files changed, 4197 insertions(+), 4151 deletions(-)
Dave Reid’s picture

Would it make more sense to name the various module entity files modulename.entity.inc and not modulename.controller.inc? Also, if entity is now its own module plz plz plz use hook_hook_info() (so that we can use the include group name 'entity') - if that's best left as a follow-up that's fine. But then we can offload hook_entity_info() and hook_entity_* to the modulename.entity.inc files as well.

catch’s picture

modulename.entity.inc sounds fine.

hook_hook_info() is definitely a separate issue please, I'm trying to keep this to moving code around as much as humanly possible.

pillarsdotnet’s picture

catch’s picture

FileSize
313.5 KB

Now working from heyrocker's cmi sandbox, but I'll keep patches here too.

http://drupalcode.org/sandbox/heyrocker/1145636.git

fago’s picture

FileSize
314.12 KB

I took a closer look at it and did some comment fixes and added back some missing 8.x changes of moved code thankfully spotted by Git, for details pls check the cmi Git repo.

So patch looks good to me, I just wonder why the file entity got moved over to the entity module?

aspilicious’s picture

Made some minor code fixes...
I wish I could push my changes somewhere on d.o. without this time consuming process of patching and interdif making.

You can use the interdif for your sandbox.

bojanz’s picture

Yes, why is the file entity in there?
Having it in the system module still baffles me, but why not use this occasion to move it where it belongs (the file module)?

catch’s picture

I'm all for moving that to file module but if we do that the functions as well as the info hook should move there, which I think will make it a required module. Not at computer to check but that felt like a separate patch/discussion last time I looked at this.

plach’s picture

mikeryan’s picture

Subscribe.

fago’s picture

FileSize
313.19 KB

I've added the fixes from #35 to the repository + reverted the file entity to be provided by the system module for now.

Updated patch attached.

Status: Needs review » Needs work

The last submitted patch, drupal_entity_module.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
313.2 KB

Re-rolled patch against the latest 8.x branch.

Crell’s picture

+1 subscribe. :-)

catch’s picture

Leaving file entity in system module is fine with me, but we should open a followup task to move managed files to a module (I think we have a module that might be suited for this purpose already!) - they are entirely dependent on the module system at the moment and having it split between file.inc and system.module (and system.install) is an historical accident.

Crell mentioned in irc he wasn't keen on adding new required modules to core. I agree with required modules being annoying, but currently the only thing making this required is user module, and node module, and managed files all being required. Well that is three things but if they weren't required then nor would entity module be so it will be just a .info file thing that could as well be dependencies, rather than the brain splitting that happens when you try to make user or system module optional.

I have been trying to think of reasons to keep entity as not a module, just as devils advocate sort of thing, but couldn't really come up with any unless we completely ripped out hook_entity_info() - if something depends on the hook system, then it depends on the module system, then it is usually a module.

It is possible that we might want to have some basic aspects of entities available at a lower level than modules (people were talking about a persistable interface at some point?) - but then we can factor those out and the entity module can use them. Even if we eventually moved it all to a class in includes one day (probably Drupal 9 or 10) having all the code centrally located in a module will make that easier than spread all over the place.

Crell’s picture

As far as Drupal 8 goes, my hope is to have a much better separation between Drupal the REST framework and Drupal the CMS. Drupal the REST framework belongs in /includes (or whatever), with extensions provided by modules. Drupal the CMS is entirely extensions on top of Drupal the REST framework, and therefore the CMS belongs in modules. Entities are content, and thus belong to Drupal the CMS, and thus to modules.

Thus, yes, +1 on moving entities entirely to modules. Making node.module, user.module, etc. optional is another good and important goal but separate from this cleanup. Later patches.

sun’s picture

FYI, work in progress: #375397: Make Node module optional

ksenzee’s picture

Subscribing.

catch’s picture

moshe asked in irc if this needed anything to be rtbc. Unfortunately we need to add a hook_help() here. There is no UI provided by the entity system whatsoever, suggestions from irc:

- boilerplate documentation about what it is.
- try to explain what entities are (oh joy).
- link to the admin screens for all the different entity types (field module does something like this).

I'm not marking CNW, we should thrash the text out either in here or a side issue, then patch it at the last minute IMO -which means that needs review.

fago’s picture

I have been trying to think of reasons to keep entity as not a module, just as devils advocate sort of thing, but couldn't really come up with any unless we completely ripped out hook_entity_info() - if something depends on the hook system, then it depends on the module system, then it is usually a module.

Well, we could use the same argument for quite some other sub-systems, e.g. the menu system also requires hook_menu() but lives in /includes.

Drupal the CMS is entirely extensions on top of Drupal the REST framework, and therefore the CMS belongs in modules. Entities are content, and thus belong to Drupal the CMS, and thus to modules.

While I would not agree with that anyway, you are mixing up modules implementing the entity API - which are about content - and the API itself. For example, content modules providing entities like "node" or "user" also implement the menu API, what does not mean the menu system *itself* needs to be a module either. Also I'd argue that the entity API belongs to the REST framework. Then the "framework -> /includes, CMS -> /modules assumption" doesn't fly, as something like Field API and much of system module is part of Drupal the framework too.

Altogether, I don't think there is any specific reason to put the entity stuff into a module, beside: It helps us with properly separating it from other sub-systems, as well as with code-organization. By having a module, we have a place that keeps all the various pieces together. So it helps others chiming into the code to get an overview of what's there and how the parts play together.
We have gone that way already with the Field API and it works. So let's please stop putting stuff into /includes and mixing everything that needs to be in a module into system.module (remember the file entity?). So if there are only advantages, but no disadvantages, let's do it! Let's have API modules wherever it helps!

@docs:
Uhm, let's deal with that in a separate issue at all. Else, I fear the discussion would just hold up everything now.

catch’s picture

I more or less agree on modules vs. includes, that is not where the real separation is or should be between framework and application, and there is currently zero separation at all - the very lowest levels of bootstrap and the installer are entirely dependent on system.module, so all we can do for now is trying to disentangle things from each other, which this patch is a part of.

docs - I agree that is best handled in a separate issue, but if we want to get this committed, we either need to do it here, or get agreement from Dries, webchick and the documentation gate keepers that we can tackle it in a follow-up (probably at some kind of threshold-breaking status so it doesn't get left forever).

fago’s picture

FileSize
313.2 KB

re-rolled the patch

catch’s picture

I've opened #1229492: hook_help() for entity module to work on hook_help() we might want to commit that together, but it'll be easier to track the discussion in its own issue.

renat’s picture

Subscribe

xjm’s picture

Tagging issues not yet using summary template.

moshe weitzman’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +Framework Initiative, +API clean-up, +Configuration system

The last submitted patch, drupal_8_entity_module.patch, failed testing.

dixon_’s picture

sub.

andypost’s picture

subscribe

fabsor’s picture

Subscribing

Shellingfox’s picture

+1 subscribe. :-)

fago’s picture

Status: Needs work » Needs review
FileSize
333.01 KB

re-rolled the patch and fixed merge conflicts

Status: Needs review » Needs work

The last submitted patch, entity_module.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
333.42 KB

hopefully fixed the tests + added in entity_help() as proposed in #1229492: hook_help() for entity module

catch’s picture

I think we're good here, but I rolled a lot of this so can't RTBC it.

klausi’s picture

Status: Needs review » Needs work
+++ b/includes/install.core.inc
@@ -253,12 +253,14 @@ function install_begin_request(&$install_state) {
+  drupal_load('module', 'entity');
+  //require_once DRUPAL_ROOT . '/modules/entity/entity.controller.inc';

is this a left over or has it a future purpose?

5 days to next Drupal core point release.

fago’s picture

Status: Needs work » Needs review
FileSize
333.35 KB

thanks, removed that.

rlmumford’s picture

Is there a way I can help with this?

catch’s picture

rlmumford, the patch is mainly moving code around, plus the addition of #1229492: hook_help() for entity module. A quick review to make sure nothing obvious looks wrong should be enough - if we broke anything the tests would tell us.

We are mainly waiting for someone other than me or fago who did most of the work on the patch to RTBC it, so if you agree with the plan, feel free to do so :)

sun’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed more closely, can't see any glitches with this patch, testbot passes, so this looks ready to fly for me.

tstoeckler’s picture

+++ b/includes/module.inc
@@ -425,8 +425,9 @@ function module_enable($module_list, $enable_dependencies = TRUE) {
+      // Allow modules to react prior to the installation of a module.
+      module_invoke_all('modules_preinstall', array($module));

@@ -451,6 +452,9 @@ function module_enable($module_list, $enable_dependencies = TRUE) {
+      // Allow modules to react prior to the enabling of a module.
+      module_invoke_all('modules_preenable', array($module));

I think these should be called "hook_pre_install()" and "hook_pre_enable()", respectively. "preenable" and "preenable" aren't real words.
I think it would make sense to rename "hook_install()" and "hook_enable()" to "hook_post_install()" and "hook_post_enable()", respectively, to make the distinction clearer. That's definitely for another issue, though.

+++ b/modules/comment/comment.info
@@ -4,6 +4,7 @@ package = Core
+dependencies[] = entity

I don't think we usually add dependencies on required modules, so this would be setting a precedent. I personally think that's a good thing to do, just want to make sure it's intentional.

Since this is (presumably) a rather tedious patch to roll, I'm leaving this at RTBC and volunteer to roll a follow-up, if this gets in, with the above changes and anything else that comes up.

klausi’s picture

Yes this is RTBC, let's do the bike shedding about hook names elsewhere (I say we should rename to "hook_modules_pre_enabled" and "hook_modules_pre_installed" and leave "hook_modules_enabled" and "hook_modules_installed" as they are for consistency, compatibility, people already know it ... etc.)

fago’s picture

We already have hook_preprocess() or hook_entity_presave(), so the patch maintains consistency with existing hooks as is. I agree that the bikeshed on whether we should use "hook_pre_something" instead should be a separate issue.

tstoeckler’s picture

@fago: That's a very valid point, I didn't realize we already have that pattern in core.

Gábor Hojtsy’s picture

Issue tags: +D8MI

This would be a great step for the multilingual initiative. <3

fago’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
333.81 KB

As #1184944-29: Make entities classed objects, introduce CRUD support has shown the patch does not apply any more. Merged in latest changes and fixed merge-conflicts in common.inc.

klausi’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC as only the cache API calls have changed.

fago’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
333.8 KB

indeed, but that patch does not account for that yet.

I've added in the cache-system improvements. Also, I've looked for other patches that might have affected the moved code, but I didn't find any new ones.

Please review.

catch’s picture

Status: Needs review » Reviewed & tested by the community

cache_*() -> cache() changes look great.

Back to RTBC. Hopefully we can get this in sooner rather than later since a lot of stuff is backed up behind it.

klausi’s picture

Ah, sorry I misinterpreted the interdiff between the patches in #76 - the cache() API changes appeared only in the removal hunks. Now it's fine as they are also in the addition hunks ==> RTBC++

fago’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
332.55 KB

again it broke...

fixed merge conflicts, added in entity-uri patch changes and re-rolled the patch.

Status: Needs review » Needs work

The last submitted patch, drupal_8_entity_module.patch, failed testing.

webchick’s picture

Assigned: Unassigned » Dries

Adding to Dries's queue. This is important to both the d8mi initiative as well as the framework initiative.

fago’s picture

I was able to fix the upgrade by manually loading the entity.controller.inc file, see attached patch.

The new upgrade tests have shown us an issue of that patch: It doesn't deal with enabling entity.module during upgrades yet. What would be the best place to do so?

catch’s picture

#1239370: Module updates of new modules in core are not executed - short version, we have no working method to enable new modules in update functions yet.

gdd’s picture

Status: Needs work » Needs review

I'd say we should get this committed and worry about the upgrade issues in a followup.

Changing status so the bot picks up the last patch.

gdd’s picture

Status: Needs review » Needs work

Oops nevermind, that wasn't a real patch

fago’s picture

Status: Needs work » Needs review
FileSize
336.01 KB

ok, I've incorporated the patch from #1239370: Module updates of new modules in core are not executed and enabled the entity module during the upgrade. The workaround of #83 is still needed to make the tests work though. Patch including the work around attached.

@workaround:
Update.php seems to do a full bootstrap before attempting updates, however entity providing modules like comment or node module have a custom entity controller extending the drupal-default-controller. Thus, it attempts to load that class what is failing without the work-a-round.

Possible alternative fix: Move all controller classes into includes... That basically means it's not safe to have any class in the .module file. It seems wrong to me that the update system relies on full bootstrap to run updates? That's a totally different issue though.

fago’s picture

FileSize
334.88 KB

ops, forgot to merge branches. re-rolled patch.

moshe weitzman’s picture

Consensus is growing around changing major version upgrade process away from hook_update_N to #1052692: New import API for major version upgrades (was migrate module). So, I think it is OK to workaround major version upgrade fails or disable those failed tests.

catch’s picture

Status: Needs review » Reviewed & tested by the community

I think we have to gloss over the full bootstrap on update issue here, we already have #534594: [meta] system info cache writing and registry rebuilding is broken without a full bootstrap open for that mess.

Classes in .module files is an autoloader issue partly, and there's active work on changing that (mainly for /includes but it may filter to modules later), so we can punt that for a bit as well.

The changes to update the module look fine, and that's ensuring the update_module_enable() function works at least to an extent (although it doesn't need to actually do much in this case).

I'm going to mark this back to RTBC. If we commit this as it is then #1239370: Module updates of new modules in core are not executed still needs more fundamental fixes for Drupal 7, and possibly follow-up for Drupal 8 but we will only likely be able to properly evaluate that with the more complex issues that exist with the D7 upgrade path (which is where it lived until a few hours ago).

In case anyone wonders how the patch was passing for months with this underlying bug, it's because we only just committed upgrade path tests - so they are catching bugs in patches that don't even provide update handlers which is great :)

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x.

I hope I got it right and that I added all the new files. I double checked so I think I got it right.

fago’s picture

Great! :)

@files:
Looks good to me too.

webchick’s picture

Priority: Major » Critical
Status: Fixed » Needs work
Issue tags: +Needs change record

We should add a change notice for this. At the very least, it seems any module depending on entity* functions need to add a dependency on entity module, no?

catch’s picture

Priority: Critical » Normal
Status: Needs work » Fixed
Issue tags: -Needs change record

Since it's a required module there's no dependency needed. However I added a change notification anyway since it may turn out useful - it may be that Drupal 7 sites with entity module need to take an extra step during upgrade etc. but we don't know what that will be yet.

Tentatively moving back to fixed.

Dries’s picture

Assigned: Dries » Unassigned

Status: Fixed » Closed (fixed)

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

plach’s picture

Gábor Hojtsy’s picture

Issue tags: +language-content

Tagging for the content handling leg of D8MI.

sun’s picture

At DrupalCon Munich, a few of us discussed to sorta "revert" this change; see:
#1760786: Move entity system "back" into a Drupal\Core component

webchick’s picture

We also never got docs of hook_schema_0(). What a bizarre hack.

Filed #1929816: Remove all references to/implementations of hook_schema_0().