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.
Comment | File | Size | Author |
---|---|---|---|
#93 | drupal_alter-765860-POST-APOCALYPSE.patch | 5.44 KB | xjm |
#89 | 765860-89.drupal_alter-followup.patch | 5.37 KB | sven.lauer |
#86 | 765860-55.drupal_alter-followup-d7.patch | 4.29 KB | mikey_p |
#77 | 765860-77.drupal_alter-followup.patch | 4.29 KB | xjm |
#55 | 765860-55.drupal_alter-followup.patch | 4.72 KB | effulgentsia |
Comments
Comment #1
sunAwesome!
Can we rename $cache_key to $cid?
1) We can prepend $type to $ids here and simply iterate over $type + ids.
2) IMO, each $id should be the full basename of an alter hook, so we want to remove the concatenation of $type and $id. No?
Powered by Dreditor.
Comment #2
sunQuickly discussed 2) with effulgentsia, and we concluded that the auto-concatenation of $type and $id should be fine.
Comment #3
effulgentsia CreditAttribution: effulgentsia commentedWith #1.1. Also filled in comments and optimized to only call module_list() + drupal_alter('module_implements') when it's needed.
Comment #4
sunGreen.
Last step is to implement a test that ensures proper execution order. Possibly a form in a test module, implementing hook_form_alter() and hook_form_FORMID_alter() alter hooks on behalf of core modules, using dsm() to expose hook invocation order, and assert the proper order of messages when requesting that form.
Comment #5
effulgentsia CreditAttribution: effulgentsia commented- Added the test suggested by #4: it fails in HEAD and passes with the patch.
- Updated PHPDoc for hook_form_FORM_ID_alter() and hook_block_view_MODULE_DELTA_alter() to no longer have the caveat about WTF execution order.
- Removed the commented out code for handling #692950: Use hook_module_implements_alter to allow modules to alter the weight of hooks in module_implements as that should be done as part of that issue after this one lands, or if that one lands first, this one can be re-rolled.
Comment #6
sunPlain awesome.
Comment #7
moshe weitzman CreditAttribution: moshe weitzman commentedI agree. This is nifty.
Comment #8
rfaysubscribe
Comment #9
dwwThis is a better solution to #300481: Add hook_form_FORM_ID_post_alter() to run after non-specific hook_form_alter(), but it's a more far-reaching API change. I'm happy either way, so long as *one* of these is committed for D7.
My only concern with this patch is that this doc is a bit hard to follow:
I'm not sure exactly what we can do to improve it, but the
array(FORM_ID)
part was slightly confusing. I almost thought there was some magic going on. It's trying to be an example, but it's still generic, and hence confusing.Does that make more sense to folks?
Cheers,
-Derek
p.s. Best function name ever:
form_test_form_form_test_alter_form_alter()
;)Comment #10
effulgentsia CreditAttribution: effulgentsia commentedThanks for the improvement to the PHPDoc. I completely agree.
Comment #11
sunThanks!
Comment #12
dwwThanks for the re-roll. Sorry I didn't catch this last time, but it seems that the docs for hook_form_alter() in system.api.php should mention the new hook invocation order, especially since it's a change from D6. It's true we don't need the WTF part, but we should explain how it works inside form API, since that's the fundamental Drupal altering API that basically all developers have to learn.
Also, I noticed the block.api.php docs are a bit broken (not due to this patch, but not fixed by this patch, either). The docs for hook_block_view_alter() say
@see hook_block_view_alter()
. They should point to hook_block_view_MODULE_DELTA_alter() instead. In general, any time an alter hook is being invoked with the $ids array defined, the doc block for the general alter hook should include a @see that points to the specific one(s). In the case of Form API, I think we should go further and actually explain in detail the invocation order.I'll reroll this so I don't delay this issue any futher. ;) Stay tuned...
Comment #13
dwwComment #14
dwwIn #13 I accidentally added an extra space to the block.api.php cleanup. Now fixed.
Comment #15
sunThanks!
Comment #16
quicksketchBrilliant, excellent work effulgentsia. +1 on this patch.
A little unfortunate:
$null variables being passed by reference, yuk. But this is just the sort of compromise we made when drupal_alter() was given additional variables by reference.
Comment #17
effulgentsia CreditAttribution: effulgentsia commentedI think we reached a point where Dries needs to decide between this one and #300481: Add hook_form_FORM_ID_post_alter() to run after non-specific hook_form_alter(). Getting either one in is critical. This one is better DX but potentially breaks BC (for any module currently relying on a higher weight hook_form_FORM_ID_alter() running before a lower weight hook_form_alter()). The other one does not break BC but is acknowledged as an inelegant stopgap until we do something better for D8.
Therefore, marking both as RTBC and critical. If this one makes it, the other one can be "won't fix". If the other one makes it, this one can be "normal" and possibly bumped to D8.
Comment #18
moshe weitzman CreditAttribution: moshe weitzman commentedDoes it make sense to move hook_page_build+hook_page_alter into this new system? That can be done in a follow-up.
This is a critical RTBC patch. Lets do this.
Comment #19
effulgentsia CreditAttribution: effulgentsia commentedSorry to do this folks, but sun suggested what I think is a good idea, which is to remove the suggested $ids parameter, and instead make the $type parameter optionally be an array. This patch does it. So back to "needs review", but hopefully worth it. It fixes the complaint in #16 too.
Comment #20
jpmckinney CreditAttribution: jpmckinney commentedI much prefer this API (the one without the $ids argument). I find it easier to read, too. Not to mention getting rid of the complaint in #16.
Comment #21
catchLooks good to me too. Should only be an extra is_array() call per drupal_alter() call, all other processing only happens if the $extra_types condition is true. It's an API addition rather than an API change, doesn't conflict with the hook_module_implements_alter() patch, and saves the adding hook_form_FORM_ID_post_alter() as a one-off to get around it.
Comment #22
sunSome performance considerations:
We could define $cid in that if/else statement already, like so:
We can use !isset() here.
We can use isset() here.
110 critical left. Go review some!
Comment #23
jpmckinney CreditAttribution: jpmckinney commentedIf $type is a single-valued array, for example, array('hook'), then $extra_types will become an empty array. !isset($extra_types) will be FALSE, but empty($extra_types) will be TRUE.
The change to the setting of $cid can go ahead; I don't think there was a reason for the colon in the original $cid.
Comment #24
effulgentsia CreditAttribution: effulgentsia commentedWith #22, taking into account #23, plus added a few more inline code comments to drupal_alter().
Comment #25
sunThanks!
Comment #26
drewish CreditAttribution: drewish commentedI really like this approach.
One thing that is minor but seems like it could cause test failures if they're run on a non-English site, we don't translate the expected strings:
But we pass the messages through t():
Seems like it should be one or the other.
While I'm complaining:
Could that say "we do not need to call function_exists()" ?
Comment #27
sun1) Actually correct, we shouldn't translate those messages.
2) Nice nitpick. :)
Comment #28
dwwBased on #26 (agreeing with sun in #27) -- we don't want t() for those dsm() calls and that comment is rather clunky. Needs a quick re-roll.
Comment #29
effulgentsia CreditAttribution: effulgentsia commentedWith #28.
Comment #30
moshe weitzman CreditAttribution: moshe weitzman commentedback to rtbc
Comment #31
chx CreditAttribution: chx commentedOh. My. effulgentsia++ This is one lovely patch.
Comment #32
sunCan someone distract Dries and/or webchick for a few minutes to commit this? :)
Also, #692950: Use hook_module_implements_alter to allow modules to alter the weight of hooks in module_implements went in - not sure whether this patch needs to be updated for that. (earlier versions contained some commented out lines)
Comment #33
rfay#29: 765860-29.drupal_alter-multiple.patch queued for re-testing.
Comment #34
rfaywebchick has been provided with rainbow sherbet ice cream, one of the prerequisites.
Comment #35
webchickI'm getting this:
Comment #36
webchickComment #37
Berdirdrupal_alter() moved to module.inc in http://drupal.org/cvs?commit=358286
Comment #38
mikey_p CreditAttribution: mikey_p commentedReroll.
Comment #40
dawehnerRerole
Comment #42
dawehnerdrupal_alter moved to includes/module.inc
Sadly by local git was too old.
So i moved the patched drupal_alter from my git to a current cvs checkout.
Comment #43
moshe weitzman CreditAttribution: moshe weitzman commentedLooks good again.
Comment #44
sunFixed missing newline, cleaned up phpdoc, and removed unnecessary docblock changes in block.api.php. Also, *bump*.
Comment #45
cha0s CreditAttribution: cha0s commentedGreat job all. Love the new approach.
Comment #46
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #47
andypostHook execution order is changed for hook_form_alter() and hook_form_FORM_ID_alter()
So this should be documented.
Comment #48
rfayFor docs, I'll add a note to the d6->d7 module update page. If anybody has a suggestion of anywhere else besides the API docs (which are taken care of here I think) then let me know.
Comment #49
dww@andypost #47: from the patch...
Yes, this API change needs a reference at http://drupal.org/update/modules/6/7 but the patch itself corrected the API documentation. We were careful to make sure of that fact before the patch was RTBC...
Comment #50
andypost@dww I mean to reflect this change at http://drupal.org/update/modules/6/7
Comment #51
effulgentsia CreditAttribution: effulgentsia commentedYay! Thanks all for helping push this through, especially over the last week that I was away.
Here's a followup due to #692950: Use hook_module_implements_alter to allow modules to alter the weight of hooks in module_implements landing.
Comment #52
moshe weitzman CreditAttribution: moshe weitzman commentedWe are adding a call to drupal_alter() from within drupal_alter()? This is OK?
Comment #53
effulgentsia CreditAttribution: effulgentsia commentedIt's safe from infinite recursion, because it only happens when drupal_alter() is called with an array for $type, and the inner call is with a scalar for $type. What else would make it not ok?
Comment #54
yched CreditAttribution: yched commentedAwesome patch. Could be really useful for hook_field_attach_X() hooks. Created #785104: enrich hook_field_attach_X() with per-entity-type variants.
Comment #55
effulgentsia CreditAttribution: effulgentsia commentedRe-roll of #51 plus a code comment addition to address #52/#53.
Exploring #54 can be done in the linked issue.
I'm a little slow, so I only now realized what Moshe was asking for in #18: please review #797626: Add hook_page_SPECIFIC_alter(), similar to hook_form_FORM_ID_alter().
Comment #56
moshe weitzman CreditAttribution: moshe weitzman commentedThx for the clarifying code comment. The function names in the api docs are a bit nasty but it seems needed when you dare to alter drupal_alter().
Comment #57
Dries CreditAttribution: Dries commentedI think this should be postponed to Drupal 8. I think this is becoming a little crazy, so I feel that this needs more discussion. We can do that in the Drupal 8 development cycle.
Comment #58
effulgentsia CreditAttribution: effulgentsia commented@Dries: please reconsider this one. I think I failed to classify the issue correctly in the #51 follow-up. Perhaps I should have made a new issue out of it. Anyway, changing the title and category here to be clearer.
#692950: Use hook_module_implements_alter to allow modules to alter the weight of hooks in module_implements went in. hook_form_alter() is one of the likely candidates of a hook that someone using hook_module_implements_alter() will want to re-order. With HEAD, for a particular $form_id, if all modules that implement hook_form_FORM_ID_alter() also implement a hook_form_alter(), then modules will be ordered as dictated by hook_module_implements_alter(), as one would expect. However, if even one module implements hook_form_FORM_ID_alter() that does not implement hook_form_alter(), then drupal_alter() needs to re-order the modules by calling module_list(), but without #55, it does not call hook_module_implements_alter(), as module_implements() does after calling module_list(). This creates a pointless inconsistency.
This was not unforeseen. The #3 patch included this, but I took it out of #5, because #692950: Use hook_module_implements_alter to allow modules to alter the weight of hooks in module_implements had not yet landed at that time. Please see the comment in #5: I had always intended to re-roll one of the two patches, after whichever one landed first. Alas, they both landed while I was away (that's not a complaint: when I came back, I was thrilled to see both land), requiring a separate follow-up.
Comment #59
andypostLooks like reasonable bug fix not a feature.
Comment #60
marcingy CreditAttribution: marcingy commented#55: 765860-55.drupal_alter-followup.patch queued for re-testing.
Comment #61
rfayAdded the requested doc to 6/7 module upgrade page: http://drupal.org/update/modules/6/7#form_alter_change
Comment #62
effulgentsia CreditAttribution: effulgentsia commented#55: 765860-55.drupal_alter-followup.patch queued for re-testing.
Comment #63
sunI realize that I haven't yet, so I totally have to express my full support for this patch now. I can only guess that other module system maintainers are on board as well, as this patch merely completes the functionality.
Comment #64
ksenzeeI also want to see this go in. It is a straight-up bugfix. Without it, hook_module_implements_alter is just plain broken - it won't change the order of hook_form_alter implementations, which is pretty much the point of its existence - and we're right back to messing around with module weights in the database.
Comment #65
sun#55: 765860-55.drupal_alter-followup.patch queued for re-testing.
Comment #66
drunken monkeyI ran into this bug, too, seems like an important fix to me.
hook_module_implements_alter()
failing seemingly randomly just isn't nice for module developers.Comment #67
tom_o_t CreditAttribution: tom_o_t commented#55: 765860-55.drupal_alter-followup.patch queued for re-testing.
Comment #68
David_Rothstein CreditAttribution: David_Rothstein commentedDoes this still need the "API change" tag, or is that a relic of the earlier committed patch?
(In any case, it really seems like this one should go in...)
Comment #69
effulgentsia CreditAttribution: effulgentsia commentedGood question. This has the potential of changing the order in which hook_form(_FORM_ID)_alter() implementations run, so it is technically a BC break. But, it changes the order from being broken (counter to the design of hook_module_implements_alter()) to being not broken. Basically, it just fixes a bug where hook_module_implements_alter() is not being called where it needs to be. So, leaving the "API change" tag for now, but I'm open to an alternate interpretation of whether it really is or not.
Comment #70
webchickDries was not a fan of this back in #57. Leaving to him to decide.
Comment #72
effulgentsia CreditAttribution: effulgentsia commented#55: 765860-55.drupal_alter-followup.patch queued for re-testing.
Comment #73
dawehnerComment #74
moshe weitzman CreditAttribution: moshe weitzman commented#55: 765860-55.drupal_alter-followup.patch queued for re-testing.
Comment #75
xjm#55: 765860-55.drupal_alter-followup.patch queued for re-testing.
Comment #77
xjmRerolled against current D8 codebase; setting back to RTBC.
Comment #78
xjmTagging issues not yet using summary template.
Comment #79
sven.lauer CreditAttribution: sven.lauer commented+1 for fixing this right away. Just had a major WTF/hair pulling session over this.
I also do not agree that this changes the API. The API as designed says what hook_module_implements_alter() will do. It does not do that this thing in case of _alter() hooks with multiple variants. Which means the implementation is plain broken.
Also, what would it mean if someone relied on the broken behavior? That would be someone who implements hook_module_implements_alter() to change the order of, say a form_alter and then relies on the fact that his hook implementation is ineffective, right? Which ... sounds a LEEEETLE crazy.
This is also incredibly hard to debug, because hook_module_implements_alter() implementations will still be invoked in these cases. Meaning that once a module author gets the idea to check if maybe his hook implementation is not being called, he will find that it is and rule that out as being the problem.
Comment #80
ralf.strobel CreditAttribution: ralf.strobel commentedI second the opinion of #79. This should be fixed in 7.x since it doesn't really change the API, it rather fixes it.
What I have done now as a hack to get past this issue is to completely unset the hook I want executed after mine in hook_module_implements_alter() and call it manually at the end of my function.
Comment #81
sunStill looks ready to me.
Comment #82
drywall CreditAttribution: drywall commented+1 to comments #79 and #80.
Comment #83
BenK CreditAttribution: BenK commentedSubscribing
Comment #84
webchickMoving to Dries's queue.
The gist is this breaks hook_module_implements_alter() and hook_form_alter() when used together.
Comment #85
carn1x CreditAttribution: carn1x commentedsubscribe
Comment #86
mikey_p CreditAttribution: mikey_p commentedUpdated patch for D7 (with -p1 for those of us using drush_make).
Comment #87
franzkewd CreditAttribution: franzkewd commentedSubscribe
Comment #88
ksenzeeWe've been running this patch in Drupal Gardens for quite some time. As best I can tell, with this patch, the only way to change the order of hook_form_mymodule_form_alter implementations is to change the order of hook_form_alter itself. I'm wondering a) if my understanding is correct, b) if that's the behavior we want or need, and c) if so, where and how we should document it.
Comment #89
sven.lauer CreditAttribution: sven.lauer commentedAccording to the in-code comments, this is what is intended. Not sure if it what we "want or need", but what would be an alternative? I suppose someone MIGHT want to change the order within a hook type, so as to have mymodule_form_FORM_ID_alter be run before mymodule_form_alter, but this would never be strictly necessary.
I do agree, though, that this would need to be documented, at least in the doc of hook_module_implements_alter(). Problematically, there is no information, generally, what the "primary hook" is (as far as I can see, the current implementation will just use the first element of the "hook" parameter as "primary"), so, basically, a doc comment would need to be added to each alter-hook documentation that has multiple "variants". I think this can happen in a follow-up patch (seeing as right now, there is NO WAY to influence the order of execution for hook_alter()-implementations (except futzing with system weights in the database).
Attaching a re-roll of #77, with the only addition being a paragraph in the documentation of hook_module_implements_alter().
Comment #90
donahoche CreditAttribution: donahoche commenteddrupal_alter-with_ids.patch queued for re-testing.
Comment #91
agentrickardI don't like that this is in the D8 queue. It's a serious D7DX error.
The patch just fixed the head-desk moment i was experiencing, since hook_module_implements_alter() had no effect on hook_form_alter() execution order. This seems reasonable, and the problem with child hooks and the alter order could be addressed later.
Tempted to mark RTBC, since the patch is green. I'll wait for one more supporter.
Comment #92
yoyek CreditAttribution: yoyek commentedThis is a must for D7
Comment #93
xjmRerolled for Patchmageddon.
Comment #94
sven.lauer CreditAttribution: sven.lauer commentedI want to point out that the last re-roll (mine) before #93 was a doc-only change. So anyone who was in favor of the functionality-part of this before, still should be.
Comment #95
xjmRegarding #88 and the specific case of form alters, there's an issue open to clean up their docs:
#1229868: hook_form_BASE_FORM_ID_alter(), hook_form_alter(), and hook_form_FORM_ID_alter() doc inconsistent.
We might want to fact-check that based on what's in this issue.
Comment #96
Dries CreditAttribution: Dries commentedCommitted to 8.x and back ported to 7.x.
Comment #97
chx CreditAttribution: chx commentedRoll this back from D7, you changed an API. http://drupal.org/node/1123140 is the D7 issue it's diamond hard.
Edit: you changed the order alters fire, I mean. The other linked issue is about fixing the bug without doing that but it got out of hand as it tried to solve everything. Just fixing the "specific alter does not fire if the generic alter is not implemented" bug without trying to keep ordering in self-contradictory situations is a good idea.
Comment #98
marcingy CreditAttribution: marcingy commentedComment #99
effulgentsia CreditAttribution: effulgentsia commented@chx: #79 argues for why this wasn't an API change. What about that argument do you disagree with, or have you found an unanticipated API breakage here?
Comment #100
chx CreditAttribution: chx commentedI am way too sleepy to deal with this. marcingy i think reproduced this or the other issue, I let him deal with this: if he can't repro a problem, re-close this.
Comment #101
marcingy CreditAttribution: marcingy commentedThe issue was there was a conflict with patch in http://drupal.org/node/1123140 which I discovered was part of code base after flagging a potential issue with webchick breakage in what will be d7.10. So the breakage I was seeing came from the 2 patches clashing. I am away to test further and makes sure that the site still behaves as expected with this patch. Although it does appear to involve a significant change in how we determine implematations for a d7 point release.
Comment #102
marcingy CreditAttribution: marcingy commentedWith this patch the implements_alters that we have still seem to work as expected. So the main issue seems to be is this an API change or not for d7 as the alters I expect to get called are being called.
Comment #103
effulgentsia CreditAttribution: effulgentsia commentedPersonally, I agree with the reasoning in #79 that the commit in #96 was not an API change and therefore, ok to have done in D7. Quickly scanning the comments, I find no disagreement with this, so removing the "API change" tag, reflecting current consensus. However, leaving the issue open in case someone wants to make a claim for why this needs to be reverted. Tomorrow's the last Wednesday of the month, so there might be the next point release, so if there's a reason to revert this, please say so soon. I'm not seeing in #100 or #102 a strong argument for reverting.
Comment #104
andypostThis means an slight API change.
_form_alter's hooks now invokes in unpredictable order? If so then a lot of contrib could have troubles....
1 days to next Drupal core point release.
Comment #105
webchickGiven the release on Wednesday, it seems like backing out this change in D7 seems prudent, postponed on ample time for discussion. I'll do so in a couple of hours, unless someone can provide evidence that this won't cause unexpected problems, given it already has at at least one production Drupal 7 site.
Comment #106
webchick.
Comment #107
effulgentsia CreditAttribution: effulgentsia commentedThe situation is this:
As per #79, this might be a BC break for sites that are currently relying on some forms having the alter sequence go counter to what is programmed within a hook_module_implements_alter() implementation, but that's not an API change, it's fixing Drupal core to obey hook_module_implements_alter() consistently, for all forms.
If we need to revert this for one more month of evaluation, then ok, but I'm not yet clear on what specific problem this has caused on any production site, except ones running alternate core patches trying to fix the same bug.
Comment #108
catchThat sounds like an issue running two different patches for the same bug, rather than because of the actual core change introduced here.
Makes sense to me to not push this straight out into a 7.x release though. It is a shame we are still discussing this after several point releases because it was deferred for so long in the first place, but another month can't do any more harm.
Comment #109
marcingy CreditAttribution: marcingy commentedIndeed the issue on the site in question was caused by a conflict between 2 different patches that were attempting to do the same thing. Using this patch alone things appear to work as expected and strategic placement of dpms gave me the expected results.
Comment #110
David_Rothstein CreditAttribution: David_Rothstein commentedBacking it out for now seems OK. It was committed without being marked RTBC, so it's worth discussing more until everyone is on board. Also, for a patch like this, a few days after a point release is a much better time to commit it than a few days before.
That said, I don't see any evidence for a problem here either. #104 is not an API change; it refers to code that previously would have been ignored (due to the bug) and documents how it behaves now that it no longer is ignored, while also documenting the fact that some code is ignored which was ignored both before and after the patch...
Anyway, I was pretty confused by this issue for a while, and the following is how I eventually understood it, so maybe this will help others. (This example is based only on reading the patch and has not been tested with actual code, so take it with a grain of salt. Maybe someone would be interested in confirming it with actual code :)
Scenario: You have Module A, which implements both hook_form_alter() and hook_form_FORM_ID_alter(), and Module B, which just implements hook_form_alter(). Here is the order in which functions get called in various scenarios when the FORM_ID form is being built:
Before the patch
I.e., no change from above; the hook implementation was simply ignored. This is the bug.
Still no change from above.
After the patch
Same behavior as before the patch.
New behavior. This fixes the bug.
Same behavior as before the patch; the hook_module_implements_alter() implementation is ignored. This seems a bit odd to me (and also limiting); however, the patch documents it as expected behavior, and changing it would probably require a more serious code refactoring so doesn't make sense to do in this issue.
Comment #111
David_Rothstein CreditAttribution: David_Rothstein commentedAll that said, I think this patch is legitimately "needs work" for a couple reasons:
There are two grammatical mistakes ("have to have to" and "change the order of hook_form_alter() implementation"), and it's also hard to understand what this is saying. It's no surprise given that this is a very complicated issue to explain, but I think we need to give people a more concrete statement here. They need to be told directly that their hook implementation will be ignored if they try to target one FORM_ID only, so their only choices are to reorder hooks for all forms or none at all.
Comment #112
webchickOk, since the one and only report of this breaking anything was from a site that was already hacking core to get around this problem, I think we can leave this in for now. Gulp. Hope so anyway. December does not offer a lot of opportunities for do-overs in terms of core releases. :\
The rest of the stuff David outlined sound like good ideas, but this doesn't feel like a critical bug report any longer. More like normal follow-ups.
Comment #113
Jody LynnComment removed because I was just behind the times and updating to 7.10 fixed my issue.
Comment #114
farald CreditAttribution: farald commentedI get this issue with geocoder and a custom module trying to reorder hook_field_attach_presave. Should be straight forward as done exacly as in the example on the api reference page. But it doesn't work.
I ended up changing weights with
Comment #115
andypostlooks not fixed for d7