Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
After a discussion with Catch, I've created this patch to allow modules to change the order of hooks. Should be highly useful in contrib, and light weight with pretty much 0 impact, since the info is cached.
Invokes hook_module_implements_alter(&$implementations, $hook). See patch for details.
Comment | File | Size | Author |
---|---|---|---|
#26 | module_implements_alter.26.patch | 8.75 KB | effulgentsia |
#23 | module_implements_alter.23.patch | 8.47 KB | effulgentsia |
#18 | module_implements_alter.18.patch | 8.49 KB | effulgentsia |
#17 | module_implements_alter.17.patch | 2.21 KB | effulgentsia |
#8 | module_implements_alter.patch | 1.75 KB | chx |
Comments
Comment #1
catchLooks very nice, subscribing for a proper look later.
Comment #3
catchAhh, except calling module_implements from module_implements() would cause an infinite loop. Probably needs a re-implementation to make it work.
Comment #4
aaron CreditAttribution: aaron commentedodd. when i ran it (using a dpm() in a hook_module_implement) it worked fine... ok, i'll look at it more on monday.
Comment #5
chx CreditAttribution: chx commentedI am wondering whether just disallowing altering the module_implements hook is enough.
Comment #6
moshe weitzman CreditAttribution: moshe weitzman commentedsubscribe. code looks good to me. should be rtbc after green.
Comment #8
chx CreditAttribution: chx commentedI mean module_implements_alter, of course...
Comment #9
catchSo this is really late, but it'd be so, so nice ;)
* When we added the registry, one thing we had planned was to allow individual hook weighting, but there were lots of other things to worry about, then it got ripped out. This is a minimal, completely optional solution and I can't see a way we'd actually get it cleaner than this.
* Using module weights for hook ordering is really nasty and gets us into all kinds of pickles. It's not a critical bug, but it is confusing - and new developers often run into problems doing things like trying to alter form elements which don't exist yet because another module is adding them later. The phpdoc here makes the reasons for this explicit, and provides a workaround which should mean less of the current module weights arms race (see #601642: Old databases might have a small {system}.weight for a critical bug that arms race is causing).
Comment #10
moshe weitzman CreditAttribution: moshe weitzman commenteddunno what bot is weighting for. rtbc.
Comment #11
effulgentsia CreditAttribution: effulgentsia commentedOh, this is just so damn cool! Huge +1 and subscribing. I have often wanted the ability for a module to run certain hooks at a different weight than the module weight, and I opened #195275: Proposing change to allow modules to specify weight(s) per hook over two years ago, but it didn't get any traction. Thank you, aaron, catch, and chx!
Comment #12
webchickTsk, tsk, tsk you guys. :P Feature freeze was 5 months ago. :P~ Though catch summarized well on IRC when he said something to the effect that this gets us all the things we wanted from the registry in like 4 lines of code.
If this is going to go in, it needs tests. I'll need to run this past Dries, though. I'm terrified at what kind of subtle bugs this might introduce that we spend the next 6 months cleaning up and not cleaning up the criticals queue.
Comment #13
catchI'm unable to find this in HEAD - please commit again?
Comment #14
chx CreditAttribution: chx commentedcatch, webchick said "needs tests".
Comment #15
effulgentsia CreditAttribution: effulgentsia commentedI'm a little confused why this needs tests. Do we have tests for other places where we add a drupal_alter() step?
Comment #16
moshe weitzman CreditAttribution: moshe weitzman commentedI was thinking the same thing. There is nothing to test here AFAICT. drupal_alter() is already tested. Please elaborate and move back to CNW if we are mistaken.
Comment #17
effulgentsia CreditAttribution: effulgentsia commentedSee. Who needs new tests when we're already failing 800 existing ones (see #8). CNR for bot. Maybe we need tests after all because of the bootstrappiness issue (see patch), but I'm not thinking of what to test at the moment. Anyone else have any ideas?
Comment #18
effulgentsia CreditAttribution: effulgentsia commentedAnd here's a variant that doesn't introduce the bootstrappiness mind-bender, and just puts drupal_alter() into module.inc.
Comment #19
moshe weitzman CreditAttribution: moshe weitzman commented#18 looks great to me.
Comment #20
aaron CreditAttribution: aaron commentednice.
Comment #21
Anonymous (not verified) CreditAttribution: Anonymous commented+
Comment #22
sunDuplicate leading space everywhere here.
Powered by Dreditor.
Comment #23
effulgentsia CreditAttribution: effulgentsia commentedPlease RTBC if satisfied.
Comment #24
sunThanks!
Comment #25
chx CreditAttribution: chx commentedLove your patch. Really. But
this made even me go o_O for a second until I realized that $implementations will be used in array order so it actually makes sense to remove and re-set the same key-value in an array but if you don't comment so this is a major WTF.
Comment #26
effulgentsia CreditAttribution: effulgentsia commentedWith #25. If this still seems like a WTF, please see #66183: Add helper methods to inject items into a particular position in associative arrays which I bumped to D8, but maybe we want to bring it back to D7.
Removing "needs test" tag as per #15-#17.
Comment #27
mattyoung CreditAttribution: mattyoung commented.
Comment #28
aaron CreditAttribution: aaron commentedI think that #26 is as clear as it could be made. Not really a contrived example, either, as it's just about the way I would use that.
array_unshift()would be the other way; if we wanted to be picky, I guess we could use array_push(), though it would have the same effect as the example, and is not recommended in general; from http://us3.php.net/manual/en/function.array-push.php: "Note: If you use array_push() to add one element to the array it's better to use $array[] = because in that way there is no overhead of calling a function.".
I didn't know about #66183: Add helper methods to inject items into a particular position in associative arrays: if that were in core, I would *definitely* use that in the example, as I suspect it would be a common need in this case. However, I don't think that's a blocker for this, as we need this functionality, just because it should probably work in most cases even w/o it, as it's easy enough in most cases to get the effect of > or < than another hook with $array[] = and array_shift().
Comment #29
chx CreditAttribution: chx commentedI am fine with this. I asked for a comment and yay i got a nice one.
Comment #30
bleen CreditAttribution: bleen commentedsubscribe
Comment #31
kbahey CreditAttribution: kbahey commentedAs I said elsewhere, I like the potential superpowers this provides: Imagine doing one thing with module A before module B, and another thing with module B before A! Now that is really eeeeevillll ... And I love it!
Comment #32
Owen Barton CreditAttribution: Owen Barton commentedWow, this is pretty jaw dropping functionality indeed. Code looks sane to me and comments are clear.
Comment #33
EvanDonovan CreditAttribution: EvanDonovan commentedI definitely wanted this the other day in D6. Exciting stuff.
Comment #34
webchick#692950 by effulgentsia, chx, aaron, catch, moshe weitzman, sun: Added hook_module_implements_alter() to allow modules to alter the weight of hooks in module_implements().
Talked this over with Dries, and we both agreed this is a far better solution than the previous attempt, and gives the nice advantage of achieving what we set out with the registry in just a couple of lines of code. He says he's cool with it, soooo....
Committed to HEAD! Great work, folks.
Comment #35
aaron CreditAttribution: aaron commentedOK, I've added documentation at http://drupal.org/node/224333#hook_module_implements_alter and a related coder issue at #842632: New hook_module_implements_alter; I figure we will do wonders by alerting devs using coder if they are changing their module's system weight.