Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
I just found this working on D6, and it is true in D7.
Since the module_invoke doesn't pass $form_state, it is not possible to use AHAH form elements on hook_block_configure().
This may be true of other core form hooks as well.
Comment | File | Size | Author |
---|---|---|---|
#28 | 690828-block-form_state-28.patch | 7.72 KB | foopang |
#26 | 690828-block-form_state-14.patch | 7.9 KB | tstoeckler |
#19 | 690828-block-form_state.patch | 6.99 KB | agentrickard |
#14 | 690828-block-form_state-14.patch | 7.9 KB | tstoeckler |
#9 | 690828-block-ahah_0.patch | 9.91 KB | agentrickard |
Comments
Comment #1
agentrickardHere's a patch for review. Note that $form_state must be the first parameter passed to the hook; this is because you cannot set a default value on a variable passed by reference.
Comment #2
agentrickardUgh. If this goes in, then we also have to reset all implementations of this hook. That should be addressed here, too.
Comment #3
agentrickardSo let's try that.
Comment #4
agentrickardComment #5
retester2010 CreditAttribution: retester2010 commented#3: 690828-block-ahah.patch queued for re-testing.
Comment #7
agentrickardNot sure why that failed. Re-roll.
Comment #9
agentrickardOh, module_invoke, why dost thou fail me so?
Fixed by using a custom invoke handler to pass $form_state by reference.
Comment #10
agentrickardMoving.
Comment #11
tstoecklerMissing line between @param and @return
Trailing whitespace
I don't know if the second sentence is actually needed or if it would actually confuse people.
Also marked #1117590: Pass $form_state to block configure forms as duplicate.
Powered by Dreditor.
Comment #12
Damien Tournoud CreditAttribution: Damien Tournoud commentedAs far as I know, that's not true.
Could we find a way to keep the signature of
hook_block_configure()
compatible? That's the only way to eventually get this into D7.Comment #13
effulgentsia CreditAttribution: effulgentsia commentedThis issue is about AHAH, but there are non-AHAH use-cases for wanting $form_state in hook_block_configure() as well. See #430886-114: Make all blocks fieldable entities.
Comment #14
tstoecklerHere's a reroll with $form_state as last parameter.
Comment #15
agentrickardLooks good. I suppose it needs a test :-p. A simple "can we alter $form_state from a module" check should do.
Comment #16
tstoecklerI would like to challenge that. We would basically be testing module_invoke(), except that we can't use that because of the reference thing, so we roll our own.
webchick's or Dries' call, though, and if desired and I get to it, I'll roll some tests.
Comment #17
tstoecklerThinking about this, the lack of a block_test_configure() shows that blocks are completely untested anyway. Let's not fix that here, please. I posted #1168644: Test hook_block_info() and friends for that. Removing tag.
Comment #18
agentrickardFair enough. Has anyone actually code-tested the behavior?
Comment #19
agentrickardUpdated patch for D8 core.
Comment #20
tstoecklerI rolled some former version of this patch, but I'm going to mark it RTBC anyway. It's a super simple fix. As said above tests for this would be somewhat redundant, but more importantly complete scope-creep for this issue.
Comment #21
catchAdding tests for a bug report isn't scope creep, it's adding tests for a bug report.
This looks easy enough to test to me so we ought to be able to do it here.
Comment #22
tstoecklerAs I pointed out above hook_block_configure() and friends are completely untested as it stands currently. So writing tests doesn't just mean testing this particular bug, but writing at least basic tests for hook_block_info(), hook_block_configure(), hook_block_save() and hook_block_view(). If someone wants to do that, all the better, but I don't think we should hold this up on that.
But that's just me. :)
Comment #23
tstoecklerThis got fixed by #1535868: Convert all blocks into plugins. See http://drupalcode.org/project/drupal.git/blob/HEAD:/core/modules/block/l...
Comment #24
tstoecklerActually "closed (duplicate)" seems more fitting.
Comment #25
ParisLiakos CreditAttribution: ParisLiakos commentedit would still be cool to fix this in d7
Comment #26
tstoecklerLet's see if #14 applies to D7.
Comment #28
foopang CreditAttribution: foopang commentedRe-roll the patch.