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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

agentrickard’s picture

Status: Active » Needs review
FileSize
1.7 KB

Here'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.

agentrickard’s picture

Status: Needs review » Needs work

Ugh. If this goes in, then we also have to reset all implementations of this hook. That should be addressed here, too.

agentrickard’s picture

Status: Needs work » Needs review
FileSize
8.31 KB

So let's try that.

agentrickard’s picture

Issue tags: +#d7ux
retester2010’s picture

Issue tags: -ahah, -#d7ux

#3: 690828-block-ahah.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +ahah, +#d7ux

The last submitted patch, 690828-block-ahah.patch, failed testing.

agentrickard’s picture

Status: Needs work » Needs review
FileSize
9.11 KB

Not sure why that failed. Re-roll.

Status: Needs review » Needs work

The last submitted patch, 690828-block-ahah_0.patch, failed testing.

agentrickard’s picture

Status: Needs work » Needs review
FileSize
9.91 KB

Oh, module_invoke, why dost thou fail me so?

Fixed by using a custom invoke handler to pass $form_state by reference.

agentrickard’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7

Moving.

tstoeckler’s picture

Status: Needs review » Needs work
+++ modules/block/block.admin.inc	22 Jul 2010 18:59:22 -0000
@@ -583,3 +583,27 @@ function template_preprocess_block_admin
+ *   The block delta for this configuration.
+ * @return

Missing line between @param and @return

+++ modules/block/block.admin.inc	22 Jul 2010 18:59:22 -0000
@@ -583,3 +583,27 @@ function template_preprocess_block_admin
+ ¶

Trailing whitespace

+++ modules/block/block.api.php	22 Jul 2010 18:59:22 -0000
@@ -91,6 +91,9 @@ function hook_block_info_alter(&$blocks,
+ *   The current form state. Required for AHAH callbacks to function
+ *   properly.

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.

Damien Tournoud’s picture

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.

As 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.

effulgentsia’s picture

This 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.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
7.9 KB

Here's a reroll with $form_state as last parameter.

agentrickard’s picture

Issue tags: +Needs tests

Looks good. I suppose it needs a test :-p. A simple "can we alter $form_state from a module" check should do.

tstoeckler’s picture

I 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.

tstoeckler’s picture

Issue tags: -Needs tests

Thinking 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.

agentrickard’s picture

Fair enough. Has anyone actually code-tested the behavior?

agentrickard’s picture

Updated patch for D8 core.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

I 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.

catch’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs tests

Adding 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.

tstoeckler’s picture

As 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. :)

tstoeckler’s picture

tstoeckler’s picture

Status: Fixed » Closed (duplicate)

Actually "closed (duplicate)" seems more fitting.

ParisLiakos’s picture

Version: 8.x-dev » 7.x-dev
Status: Closed (duplicate) » Needs work
Issue tags: -Needs backport to D7

it would still be cool to fix this in d7

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
7.9 KB

Let's see if #14 applies to D7.

Status: Needs review » Needs work

The last submitted patch, 690828-block-form_state-14.patch, failed testing.

foopang’s picture

Status: Needs work » Needs review
FileSize
7.72 KB

Re-roll the patch.