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.
Comment | File | Size | Author |
---|---|---|---|
#88 | drupal_8_entity_module.patch | 334.88 KB | fago |
#87 | drupal_8_entity_module.patch | 336.01 KB | fago |
#83 | entity_module_upgrade_enable-D8.patch | 546 bytes | fago |
#80 | drupal_8_entity_module.patch | 332.55 KB | fago |
#77 | drupal_8_entity_module.patch | 333.8 KB | fago |
Comments
Comment #1
catchBit 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.
Comment #2
sunAwesome.
Comment #3
Anonymous (not verified) CreditAttribution: Anonymous commentedsubscribe.
Comment #4
catchAdded hunks from system.api.php, no other changes.
Comment #5
fagoFor 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.
Comment #6
catchActions issue is here #1008166: Actions should be a module :)
Comment #7
catchtagging.
Comment #8
sunTrailing white-space.
Sounds odd - Field API is for fields, Entity is for... well, just "handling" entities.
Id keywords need to be removed.
I'm not 100% sure whether EFQ really belongs to Entity module - perhaps @fago can provide some feedback on this question?
Powered by Dreditor.
Comment #9
catchFixed 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.
Comment #11
catchAdding required and version = 8.x
Comment #13
catchFixed the installer.
Comment #15
podaroksubscribe
Comment #16
mirocow CreditAttribution: mirocow commentedsubscribe
Comment #17
Dave ReidSubscribe, this could be helpful to provide UUIDs for all entities in D8.
Comment #18
pwolanin CreditAttribution: pwolanin commentedSounds like a good approach to get new CRUD/CRAP functions working before ripping out the legacy ones.
Comment #19
pounardSub, this is a really good idea, API will be much more clearer if centralized into such module.
Comment #20
gddTagging
Comment #21
tinyrobot CreditAttribution: tinyrobot commentedSubscribe
Comment #22
Dave ReidIf 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.
Comment #23
jherencia CreditAttribution: jherencia commentedSubscribing...
Comment #24
catchComment #25
catchRemoved 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.
Comment #27
catchShould be two fails left, need to look at the entity info cache tests.
Comment #29
catchThe 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.
Comment #30
Dave ReidWould 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.
Comment #31
catchmodulename.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.
Comment #32
pillarsdotnet CreditAttribution: pillarsdotnet commentedComment #33
catchNow working from heyrocker's cmi sandbox, but I'll keep patches here too.
http://drupalcode.org/sandbox/heyrocker/1145636.git
Comment #34
fagoI 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?
Comment #35
aspilicious CreditAttribution: aspilicious commentedMade 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.
Comment #36
bojanz CreditAttribution: bojanz commentedYes, 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)?
Comment #37
catchI'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.
Comment #38
plachComment #39
mikeryanSubscribe.
Comment #40
fagoI'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.
Comment #42
fagoRe-rolled patch against the latest 8.x branch.
Comment #43
Crell CreditAttribution: Crell commented+1 subscribe. :-)
Comment #44
catchLeaving 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.
Comment #45
Crell CreditAttribution: Crell commentedAs 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.
Comment #46
sunFYI, work in progress: #375397: Make Node module optional
Comment #47
ksenzeeSubscribing.
Comment #48
catchmoshe 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.
Comment #49
fagoWell, 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.
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.
Comment #50
catchI 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).
Comment #51
fagore-rolled the patch
Comment #52
catchI'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.
Comment #53
renat CreditAttribution: renat commentedSubscribe
Comment #54
xjmTagging issues not yet using summary template.
Comment #55
moshe weitzman CreditAttribution: moshe weitzman commented#51: drupal_8_entity_module.patch queued for re-testing.
Comment #57
dixon_sub.
Comment #58
andypostsubscribe
Comment #59
fabsor CreditAttribution: fabsor commentedSubscribing
Comment #60
Shellingfox CreditAttribution: Shellingfox commented+1 subscribe. :-)
Comment #61
fagore-rolled the patch and fixed merge conflicts
Comment #63
fagohopefully fixed the tests + added in entity_help() as proposed in #1229492: hook_help() for entity module
Comment #64
catchI think we're good here, but I rolled a lot of this so can't RTBC it.
Comment #65
klausiis this a left over or has it a future purpose?
5 days to next Drupal core point release.
Comment #66
fagothanks, removed that.
Comment #67
rlmumfordIs there a way I can help with this?
Comment #68
catchrlmumford, 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 :)
Comment #69
sunReviewed more closely, can't see any glitches with this patch, testbot passes, so this looks ready to fly for me.
Comment #70
tstoecklerI 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.
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.
Comment #71
klausiYes 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.)
Comment #72
fagoWe 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.
Comment #73
tstoeckler@fago: That's a very valid point, I didn't realize we already have that pattern in core.
Comment #74
Gábor HojtsyThis would be a great step for the multilingual initiative. <3
Comment #75
fagoAs #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.
Comment #76
klausiBack to RTBC as only the cache API calls have changed.
Comment #77
fagoindeed, 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.
Comment #78
catchcache_*() -> 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.
Comment #79
klausiAh, 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++
Comment #80
fagoagain it broke...
fixed merge conflicts, added in entity-uri patch changes and re-rolled the patch.
Comment #82
webchickAdding to Dries's queue. This is important to both the d8mi initiative as well as the framework initiative.
Comment #83
fagoI 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?
Comment #84
catch#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.
Comment #85
gddI'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.
Comment #86
gddOops nevermind, that wasn't a real patch
Comment #87
fagook, 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.
Comment #88
fagoops, forgot to merge branches. re-rolled patch.
Comment #89
moshe weitzman CreditAttribution: moshe weitzman commentedConsensus 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.
Comment #90
catchI 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 :)
Comment #91
Dries CreditAttribution: Dries commentedCommitted 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.
Comment #92
fagoGreat! :)
@files:
Looks good to me too.
Comment #93
webchickWe 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?
Comment #94
catchSince 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.
Comment #95
Dries CreditAttribution: Dries commentedComment #97
plachRelated issue: #1331350: [Upgrade path broken] Entity module not enabled during D8 upgrade.
Comment #98
Gábor HojtsyTagging for the content handling leg of D8MI.
Comment #99
sunAt DrupalCon Munich, a few of us discussed to sorta "revert" this change; see:
#1760786: Move entity system "back" into a Drupal\Core component
Comment #100
webchickWe also never got docs of hook_schema_0(). What a bizarre hack.
Filed #1929816: Remove all references to/implementations of hook_schema_0().