Suggested commit message:
Issue #2001310 by chx, berdir, yched, effulgentsia, YesCT: Disallow firing hooks during update.
Problem/Motivation
In order to get a reliable and testable update environment, we have actively tried to avoid hooks during update and preached against them. It doesn't quite work tho.
Proposed resolution
ModuleHandler is pluggable, replace it with an UpdateModuleHandler during update. UpdateModuleHandler should throw an exception on you if a hook is fired. Also move module_enable,disable and uninstall into the ModuleHandler family, move update_module_enable under UpdateModuleHandler::enable.
Remaining tasks
Commit it the moment the testbot approves. (Aka. run and don't look back.)
User interface changes
None.
API changes
None. Although module_enable/disable/uninstall is deprecated
Related Issues
- #1941000: update_module_enable() does not update ModuleHandler::$moduleList
- #1848998: Problems with update_module_enable()
- #2010376: Add ModuleHandler::setSchema
- #2010380: Deprecate module_load_install() and replace it with ModuleHandler::loadInstall
- #2010382: Convert ModuleHandler and UpdateModuleHandler to use injection
- #2010386: Resolve module disable/uninstall during update
- #1998204: config_install_default_config() is not safe to use in hook_update_N()
Comment | File | Size | Author |
---|---|---|---|
#57 | 2001310_57.patch | 24.23 KB | chx |
#57 | diffdiff.txt | 580 bytes | chx |
#49 | 2001310_47.patch | 24.27 KB | Berdir |
#49 | 2001310_47-interdiff.txt | 617 bytes | Berdir |
#46 | 2001310_46.patch | 24.24 KB | Berdir |
Comments
Comment #1
BerdirThe comment still mentions the non-longer existing "update specific version of this function"
Why do we need to move this?
This is "interesting", as that would essentially mean that at some point, we might want to inject that. Which is not possible, because this is the thing that's injected into everything else. Fun.
Nitpick, Contains...
Needs some basic documentation, especially because it's an empty class :)
If we just need the constants and nothing else, should wes simply hardcode the value? That's what I usually do with constants defined in .module
The reason I added this to the function is that we don't want to enforce the schema if image.module was already enabled before, in that case we want to run those updates. So you probably will have to move this into the if below.
Comment #2
chx CreditAttribution: chx commented> The comment still mentions the non-longer existing "update specific version of this function"
Nuked.
> Why do we need to move this?
Wait. Are you asking *me*? Please see #1848998-96: Problems with update_module_enable(). It might be familiar ;) This issue contains both that and yched's fixes.
> If we just need the constants and nothing else, should wes simply hardcode the value?
No. There are too many. It'd make the update code even more ugly.
> So you probably will have to move this into the if below.
I so did.
Comment #3
catchThis issue is very similar to #922996: Upgrade path is broken since update.php does not enforce empty module list.
I think the CRUD operations for modules should be in their own class, partly for this reason. Also system_rebuild_module_data() is another place we'll easily run into circular dependencies.
Comment #4
chx CreditAttribution: chx commentedSee #1833592: [META] The road out of module build circular dependency hell for that circular dependency.
The empty module list issue accomplishes two things: hooks can't run and module code is not loaded. Hooks can't run with this patch either but instead of a silent nothing you get a nice and loud exception. Out of the remaining hooks entity_info deserves more investigation -- it'd be good to get an exception on that one. Perhaps switching to the normal handler once the upgrades finished running would accomplish that and further simplify the update handler. I am not quite sure whether it's worth holding up this issue for it, though. Not loading modules is an interesting idea which probably deserves a followup -- it's much easier to do that once this is in place.
Actually, that exception already showed bugs which are fixed -- bugs that berdir and yched fixed but the tests didn't fail before. Well, they would do now.
Comment #5
chx CreditAttribution: chx commentedHere's a couple lines removed from HEAD that are no longer necessary. Yay, cleanup!
Comment #7
ParisLiakos CreditAttribution: ParisLiakos commented#5: 2001310_5.patch queued for re-testing.
Comment #9
ParisLiakos CreditAttribution: ParisLiakos commented#5: 2001310_5.patch queued for re-testing.
Comment #10
ParisLiakos CreditAttribution: ParisLiakos commentedthis looks like awesome cleanup!
i did a code review, and i will now test it with some upgrade issues i have:)
some of them might be totally wrong or nitpicky:)
needs inheritdoc
i think we can inject config right?
could be $this->loadInclude($module, 'install') i think
i am sure we can do something a lot better here, at least get rid of drupal_container() for \Drupal, if we dont want to make module handler container aware..or dunno injecting kernel
needs public visibility
$this->invoke
although if we inject container, we wouldn't need this call to \Drupal
adding the types here would be great
Comment #11
effulgentsia CreditAttribution: effulgentsia commentedJust merges HEAD; does not implement any feedback.
Comment #12
effulgentsia CreditAttribution: effulgentsia commentedI split off the easy move code around part of this patch to #2004784: Move module install/uninstall implementations into ModuleInstaller. That'll make reviewing the rest of this easier.
Comment #13
effulgentsia CreditAttribution: effulgentsia commented#12 landed, so this is rerolled to exclude it. Note that #2004784-5: Move module install/uninstall implementations into ModuleInstaller contains follow up improvements related to that issue, and when that lands, this will need another reroll.
Comment #15
chx CreditAttribution: chx commentedComment #16
alexpott#13: 2001310-13.patch queued for re-testing.
Comment #18
chx CreditAttribution: chx commentedFinally, this passed!
Comment #19
tstoecklerI cannot claim to be an expert in neither the update system nor the module system so the fact that I was able to (I think !) fully grasp such a fundamental patch with wide-reaching consequences is in itself a proof that it is very clearly written and well documented. It makes huge amounts of sense IMO. I especially like that there is a single code path were core can decide which hook implementations to invoke for which hook. Awesome!
That said, I have some minor remarks, mostly related to (additional) comments.
I think this deserves a comment: (assuming I understand this correctly) "Use a named key to be able to unset the bundle after performing the update. @see update_flush_all_caches()"
These two could be moved out of the foreach
Is the assumption correct that this will have to be figured out before commit?
If I understand this correctly, it seems this is an update-specific version of the router builder, as it only contains routes related to the update. If so, should this be named UpdateRouteBiulder, instead?
Minor: "@todo" instead of "@TODO:"
Is this still necessary after #1620010: Move LANGUAGE constants to the Language class?
Comment #20
effulgentsia CreditAttribution: effulgentsia commentedEven though #13 isn't that big in terms of bytes, there's still a lot there that's hard for me to review properly (it could simply be that I'm not proficient enough with the details of our update.php related code). I filed another spin off, #2009484: Move (most of) update_module_enable() into UpdateModuleHandler::enable() in an attempt to make what's needed to review here easier. That said, if others can get this to RTBC and committed without me, I'll happily close the spin off as a duplicate after this lands.
Comment #21
chx CreditAttribution: chx commentedI am superb thank for the review, having a fresh set of eyes and having someone not normally associated with this thorn bush getting entangled is a joy to see.
Enough of the splitting, however. The more the patches, the longer they linger and the more they attract bikeshed. Bigger patches noone can review fly in without review regardless whether they are broken, slow or introduce reflection into the normal code path (although this usually only applies to WSCCI patches but I hope they will apply to update patches too) -- so let's not split this further. Not much would remain of the patch if we were to do that, anyways.
No, let's get this in as fast as possible without much of bikeshed (so far the reviews have actually been useful, miracles!) because this is absolutely critical to make sure the forthcoming update path patches are solid. It already caught an entity system call in file updates that I try to remove in, guess what, another issue.
> These two could be moved out of the foreach
> Is the assumption correct that this will have to be figured out before commit?
We are not changing update_module_enable, we are just moving it. These are coming verbatim from update_module_enable.
> so, should this be named UpdateRouteBiulder, instead?
No, it's a static version of the Route builder which helps the update but there's hardly anythign update specific in there. In the future, the installer might need it too, for example.
> Is this still necessary after #1620010: Move LANGUAGE constants to the Language class
Yes, I think so, yes. That issue changed the language constants, this is about a language negotiation constant.
With this said, I am simply setting this to RTBC as there have been no disagreement. If you think this violates core process, well the core process is dead already so let's no try to pretend it is not. If it is not dead, then roll back the router and we can talk.
We can iterate after it is in, and I intend to -- we should quite probably have this override only for the time the upgrades are actually running and have something better than the update_flush_caches but that's no reason not to have this guardian in core.
Comment #22
catchComment #23
chx CreditAttribution: chx commentedI will get #625958: Support Uploading Multiple Files for HTML5 Browsers via #multiple attribute to pass and then stop working on the update path until this is in.
Comment #24
DamienMcKenna+1 for this in theory but I've not reviewed the code yet, will try to do that later on using my personal site.
Comment #25
Crell CreditAttribution: Crell commentedOn the whole I agree that if hooks are a no-go in update functions we should ensure that is enforced. Swapping out ModuleHandler is a sensible way to do that.
Shouldn't this be a method on ModuleHandler?
This should be injected.
Shouldn't this function be a method on ModuleHandler at this point?
DB connection should be injected.
The config factory should be injected.
Ibid.
This actually concerns me. Lots of D6/D7 sites use update functions to enable/disable modules as a deployment mechanism. (Eg, disable a module you're no longer using, enable some newly written Views plugin module, enable a new Feature module, etc.)
With the changes brought by CMI is that no longer necessary? Vis, does CMI offer a way for me to "deploy and enable" or "deploy and disable" such that I don't need update functions anymore?
If so, then we're good. If not, then this becomes a regression.
Uh, it doesn't look like it builds much of anything. :-) What's intended here?
(Also, if this is a real class then we may want to consider adding an interface for this and the normal RouteBuilder to share.)
Grammar nitpick: "This will need to be run once all (if any) updates have been run."
Also, I hate to throw a wrench in the works, but... what about Events? What happens to those during module update? (I have no idea, personally.)
Comment #26
YesCT CreditAttribution: YesCT commentedpatch coming to fix some docs stuff.
Comment #27
YesCT CreditAttribution: YesCT commentedI dont know how to do most of what @Crell asked for in #25. So that still needs to be addressed.
I did coding style, spelling, and docs changes. The comments I added need to be checked by someone who better understands what is going on.
Comment #28
YesCT CreditAttribution: YesCT commentedIs there an issue for this already?
Comment #29
Anonymous (not verified) CreditAttribution: Anonymous commentedre #29 and CMI for modules - yes, CMI will support that.
the code hasn't landed yet, but i'm pretty sure heyrocker considers that functionality a release-blocker for CMI.
Comment #30
chx CreditAttribution: chx commentedRe #25, thanks for the review. Most of the questions are easy to answer:
> update_set_schema a method on ModuleHandler?
Followup. Has zero to do with firing hooks. #2010376: Add ModuleHandler::setSchema
> module_load_install a method on ModuleHandler?
Followup. Has zero to do with firing hooks. #2010380: Deprecate module_load_install() and replace it with ModuleHandler::loadInstall
> Injection conversions.
Followup. Has zero to do with firing hooks. The normal one isn't converted either. #2010382: Convert ModuleHandler and UpdateModuleHandler to use injection
> disable/uninstall during update
Followup. Has zero to do with firing hooks. #2010386: Resolve module disable/uninstall during update
> If so, then we're good. If not, then this becomes a regression.
Erm, nope. It's a bugfix: right now, module_disable during a major version update is broken. I understand the concern for minor upgrades and we need to address that in a followup, however that's not pressing. Getting in this patch is.
> Uh, it doesn't look like it builds much of anything. :-) What's intended here?
As the comment says: once the path 'batch' is converted to a route, add that here. All of the routebuilding mechanisms are gone, this will be the only place where routes can be stored.
Comment #30.0
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #31
chx CreditAttribution: chx commentedLet's try this -- entity_info now throws an exception. yay!
Comment #33
tim.plunkettI filed #1998204: config_install_default_config() is not safe to use in hook_update_N() for different reasons than invoking hooks, but it is now doubly a problem.
In each of the failing tests, it is called first by user_update_8011() to install image styles.
I see you fixed system_update_8046(), which was the one that caused me to file that issue in the first place. I guess this will need the same treatment.
EDIT: This process of cherry-picking new config to install will very likely become a common need.
Since we'll already be using it twice it would probably be a good idea to add a helper function in config.inc
function config_install_single_config($type, $name, $config_name){}
config_install_single_config('module', 'node', 'views.view.frontpage');
Comment #34
chx CreditAttribution: chx commented#27 is the one to review/commit. #31 I tried, didn't work yet. Wish it did -- we need to work on that -- shows how important this patch is cos it really shouldn't happen.
Comment #35
tim.plunkettNot having this extra function to confuse people is awesome.
Okay, I went through this line by line, and I think this with the reasonable list of follow-ups (which are normal conversions anyway), is good to go.
We can use the dedicated config_install_default_config() issue to tackle hook_entity_info().
And yes, the patch I reviewed and that is ready to commit is in #27.
Comment #35.0
tim.plunkettadded followups
Comment #35.1
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #35.2
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #36
chx CreditAttribution: chx commentedI added a suggested commit message to the top because this patch contains one patch from berdir and one from yched.
Comment #36.0
chx CreditAttribution: chx commentedhtml formatting
Comment #36.1
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #37
catchOverall this looks great, couple of things though:
These could use comments as to why they're explicitly allowed. Why are some of them returning empty arrays rather than throwing exceptions etc. Any of these hooks we're special casing is likely a case we'll want to clean up separately so it'd be good to document why they're in here.
I'm not sure that config staging is going to be enough for this - sometimes you want to run an update, then subsequently disable a module - particularly for custom updates rather than major version upgrades. However I think we can punt on this for now and there's already a follow-up open.
Comment #38
catchThe reason there are so many un-injected dependencies in ModuleHandler is because #2004784: Move module install/uninstall implementations into ModuleInstaller was split out of this issue - but they were in the main added by that patch. Had that not been split out, they'd be in this patch too, so they're not pre-existing in any real sense.
Comment #39
catchComment #40
chx CreditAttribution: chx commentedComment #41
YesCT CreditAttribution: YesCT commentedLooks like all the concerns have been addressed.
Comment #42
pounardDoes this means this will not be possible to disable a module anymore during an update procedure?
Comment #43
DamienMcKennaFollow-on from #42: there needs to be a way of enabling & disabling modules during update scripts otherwise that'd be a regression that would affect many developers. If this is the intention then we would need a comparable solution that can be triggered in the correct sequence while other changes are being made.
A common workflow for our (Mediacurrent) dev team today is running two update scripts - one to enable a module and then another to either assign some initial settings via SQL queries / API calls or revert a feature; how would this workflow be handled were this to be committed?
Comment #44
tim.plunkettDisabling modules is out of scope for this discussion.
Please feel free to disagree with Dries, catch, and alexpott over in #1199946-148: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed where it was decided that modules will no longer have a disabled state.
Comment #45
DamienMcKenna@tim: Ok, so the entire module installing/enabling/disabling/uninstalling process is changing so it'll no longer be possible to "disable" a module. However, it should still be possible to uninstall a module via an update script, right?
Comment #46
BerdirWell, removing that @todo was easier than expected ;)
The UpdateModuleHandler already takes care of installing the default config in a safe way, no need to do it twice for image.module.
Upgrade path tests are passing and I know we have test coverage that the image styles are there.
Comment #47
David_Rothstein CreditAttribution: David_Rothstein commentedIt's in scope if this is the issue that's going to make it impossible to do.... Also, there's definitely no agreement at #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed about removing disabled modules at the API level (there's not even total agreement about removing them at the core UI level, although that one is closer).
@chx did file a followup for this whole issue already at #2010386: Resolve module disable/uninstall during update so that seems to handle it, but we should probably be prepared for that to be bumped to major (at least)?
Comment #48
David_Rothstein CreditAttribution: David_Rothstein commentedSorry, crosspost.
Also, #922996: Upgrade path is broken since update.php does not enforce empty module list seems like a related issue (but I guess that one proposes going even further than this one does, which is why this was filed separately?).
Comment #49
BerdirRemoved one line too much. The case above used the same return array();
Comment #50
chx CreditAttribution: chx commentedRe #48 -- enforcing an empty module list does not throw exceptions -- it hides the bugs -- this reveals them.
We will deal with events too.
Comment #51
David_Rothstein CreditAttribution: David_Rothstein commentedActually, I thought that issue proposed not loading .module files at all, so in many cases it would be a fatal error rather than a silent one... However, I agree the approach here makes more sense, especially as a first step.
Comment #52
catchNot loading module files at all would mean that hooks silently didn't run, so people would still be free to fire hook-invoking functions (assuming they don't live in a non-loaded .module file) - just no hooks would be fired. That was my main concern with the other issue since it's exactly the non-predictability of hook firing with API functions that has led to so many problems in the upgrade path.
Right now trying to uninstall a module during the major version upgrade path is de-facto not supported - we don't have an update_* function for doing that. However I personally in the past couple of weeks have written custom update handlers for uninstalling modules that had dependencies - but that I knew were only going to run in a 7-7 upgrade path in a specific site.
This is where the division between major and minor updates gets us into serious difficulty, if we'd gone the migrate route for the 7-8 upgrade path we'd not have to deal with this all over again. However this patch enforces the actual limitations with the update API, if it's so limited that people can no longer do what they used to, hopefully they'll step in to fix it. The alternative is 2-3 years of broken 7-8 upgrade path again due to all the hidden, hard to reproduce, and easy to introduce bugs this patch exposes.
Comment #53
effulgentsia CreditAttribution: effulgentsia commentedWell, presumably a custom scenario like that could swap in yet some other *ModuleHandler service. Do we want a follow up to provide a MinorUpdateModuleHandler implementation in core, or is that ok to leave to contrib?
Comment #54
catchI think #2010386: Resolve module disable/uninstall during update covers it well enough. The current patch here enforces the status quo (if you strictly follow not using API functions in hook_update_N()), that issue might make it possible cleanly, so we're not really losing anything here that's not already broken.
Comment #55
tim.plunkettI think opening #2010386: Resolve module disable/uninstall during update, and the possibility for something like MinorUpdateModuleHandler, makes this safely RTBC again.
Comment #56
alexpottNeeds a reroll
Comment #57
chx CreditAttribution: chx commentedYeah because drupal_container()->get('module_handler') was replaced by Drupal::moduleHandler(); meanwhile.
Comment #58
alexpottCommitted 9d73599 and pushed to 8.x. Thanks!
Comment #59
chx CreditAttribution: chx commentedupdate_module_enable() didn't exist in Drupal 7 and it didn't have a change notice. Nothing needs to be done at this point.
Comment #60
David_Rothstein CreditAttribution: David_Rothstein commentedDon't we need one for the fact that doing anything which invokes a hook during an update function is now prevented?
Comment #61
chx CreditAttribution: chx commentedNo. You were always told to use the update versions of the functions. This is not new. But, people fail to follow instructions (because it's hard to catch all of the things firing hooks like the infamous field_read_fields in a since-removed file_update_8002) so it's important to force them.
Comment #63
David_Rothstein CreditAttribution: David_Rothstein commentedI still don't see where people were previously told this was forbidden.
In both the hook_update_N() and Update versions of API functions documentation it only says:
(And it still says the exact same thing in Drupal 8 now, which I suppose needs to be changed.)
I'll see about writing a quick change notification.
Comment #64
David_Rothstein CreditAttribution: David_Rothstein commentedCreated a change notification: https://drupal.org/node/2026515
We probably need a followup to change the above-mentioned API documentation in the codebase also.
Comment #65
chx CreditAttribution: chx commentedHuh. We didn't forbid before? Well. Thanks much for the nice change notice.
Comment #67
tim.plunkettComment #68
Fabianx CreditAttribution: Fabianx commentedWe have some problems in the theme system with that.
It seems calling theme() is allowed during updates?
But we want to actively go into a route of depending less and less on the theme registry and push the responsibility to the ModuleHandler instead.
Should we disallow usage of theme() during updates?
Comment #68.0
Fabianx CreditAttribution: Fabianx commentedUpdated issue summary.
Comment #69
sunUnfortunately, this causes serious problems, because the update.php page itself still has to be composed and rendered, which involves plenty of call chains that cannot really be controlled in the same way as hook invocations within update functions:
#2233403: Let update.php invoke hooks again