Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Awesome!

+++ includes/common.inc	8 Apr 2010 16:48:23 -0000
@@ -4646,11 +4648,38 @@ function drupal_alter($type, &$data, &$c
+  $cache_key = empty($ids) ? $type : ($type . ':' . implode(',', $ids));

Can we rename $cache_key to $cid?

+++ includes/common.inc	8 Apr 2010 16:48:23 -0000
@@ -4646,11 +4648,38 @@ function drupal_alter($type, &$data, &$c
+      foreach ($ids as $id) {
+        $modules = array_merge($modules, module_implements($type . '_' . $id . '_alter'));
+      }

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.

sun’s picture

Quickly discussed 2) with effulgentsia, and we concluded that the auto-concatenation of $type and $id should be fine.

effulgentsia’s picture

With #1.1. Also filled in comments and optimized to only call module_list() + drupal_alter('module_implements') when it's needed.

sun’s picture

Title: Enhance drupal_alter() to support hook_form_alter()/hook_form_FORM_ID_alter() pattern in desired execution order » Make drupal_alter() support multiple alter hooks executed by module weight
Category: feature » task

Green.

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.

effulgentsia’s picture

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

sun’s picture

Status: Needs review » Reviewed & tested by the community

Plain awesome.

moshe weitzman’s picture

I agree. This is nifty.

rfay’s picture

subscribe

dww’s picture

Status: Reviewed & tested by the community » Needs work

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

+ * @param $ids
+ *   (optional) An array of ids for executing specific implementations in
+ *   addition to generic ones. For example, to execute both hook_form_alter()
+ *   and hook_form_FORM_ID_alter() functions, pass 'form' as $type and
+ *   array(FORM_ID) as $ids.

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.

 *   (optional) An array of ids for executing specific implementations in
 *   addition to generic ones. For example, when the Drupal Form API
 *   is using drupal_alter() to execute both hook_form_alter() and
 *   hook_form_FORM_ID_alter() functions, it uses 'form' as $type and
 *   passes array($form_id) as $ids.

Does that make more sense to folks?

Cheers,
-Derek

p.s. Best function name ever: form_test_form_form_test_alter_form_alter() ;)

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
11.89 KB

Thanks for the improvement to the PHPDoc. I completely agree.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

dww’s picture

Status: Reviewed & tested by the community » Needs work

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

dww’s picture

Status: Needs work » Needs review
FileSize
15.2 KB
dww’s picture

In #13 I accidentally added an extra space to the block.api.php cleanup. Now fixed.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

quicksketch’s picture

Brilliant, excellent work effulgentsia. +1 on this patch.

A little unfortunate:

-      drupal_alter('query', $query);
-      foreach ($this->alterTags as $tag => $value) {
-        drupal_alter("query_$tag", $query);
-      }
+      $null = NULL;
+      drupal_alter('query', $query, $null, $null, array_keys($this->alterTags));

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

effulgentsia’s picture

Priority: Normal » Critical

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

moshe weitzman’s picture

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

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
15.48 KB

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

jpmckinney’s picture

Status: Needs review » Reviewed & tested by the community

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

catch’s picture

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

sun’s picture

Some performance considerations:

+++ includes/common.inc	14 Apr 2010 16:42:46 -0000
@@ -4637,14 +4643,52 @@ function drupal_alter($type, &$data, &$c
+  if (is_array($type)) {
+    $extra_types = $type;
+    $type = array_shift($extra_types);
+  }
...
+  $cid = empty($extra_types) ? $type : ($type . ':' . implode(',', $extra_types));

We could define $cid in that if/else statement already, like so:

if (is_array($type)) {
  $cid = implode(',', $type);
  ...
}
else {
  $cid = $type;
}
+++ includes/common.inc	14 Apr 2010 16:42:46 -0000
@@ -4637,14 +4643,52 @@ function drupal_alter($type, &$data, &$c
+    if (empty($extra_types)) {

We can use !isset() here.

+++ includes/common.inc	14 Apr 2010 16:42:46 -0000
@@ -4658,12 +4702,20 @@ function drupal_alter($type, &$data, &$c
+        if (!empty($extra_types)) {

We can use isset() here.

110 critical left. Go review some!

jpmckinney’s picture

If $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.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
16.17 KB

With #22, taking into account #23, plus added a few more inline code comments to drupal_alter().

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

drewish’s picture

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

+    $expected = array(
+      'block_form_form_test_alter_form_alter() executed.',
+      'form_test_form_alter() executed.',
+      'form_test_form_form_test_alter_form_alter() executed.',
+      'system_form_form_test_alter_form_alter() executed.',
+    );
+    $content = preg_replace('/\s+/', ' ', filter_xss($this->content, array()));
+    $this->assert(strpos($content, implode(' ', $expected)) !== FALSE, t('Form alter hooks executed in the expected order.'));
+  }

But we pass the messages through t():

+  drupal_set_message(t('block_form_form_test_alter_form_alter() executed.'));
...
+    drupal_set_message(t('form_test_form_alter() executed.'));
...
+  drupal_set_message(t('form_test_form_form_test_alter_form_alter() executed.'));
...
+  drupal_set_message(t('system_form_form_test_alter_form_alter() executed.'));

Seems like it should be one or the other.

While I'm complaining:

+      // For the more common case of a single hook, we do not need a
+      // function_exists(), since module_implements() only returns modules with
+      // implementations.

Could that say "we do not need to call function_exists()" ?

sun’s picture

1) Actually correct, we shouldn't translate those messages.

2) Nice nitpick. :)

dww’s picture

Status: Reviewed & tested by the community » Needs work

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

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
16.16 KB

With #28.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

back to rtbc

chx’s picture

Oh. My. effulgentsia++ This is one lovely patch.

sun’s picture

Can 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)

rfay’s picture

rfay’s picture

webchick has been provided with rainbow sherbet ice cream, one of the prerequisites.

webchick’s picture

I'm getting this:

patch -p0 < 765860-29.drupal_alter-multiple.patch 
patching file includes/common.inc
Hunk #1 FAILED at 4620.
Hunk #2 FAILED at 4643.
Hunk #3 FAILED at 4719.
3 out of 3 hunks FAILED -- saving rejects to file includes/common.inc.rej
patching file includes/form.inc
patching file includes/database/select.inc
patching file modules/block/block.api.php
Hunk #1 succeeded at 184 (offset 19 lines).
Hunk #2 succeeded at 213 (offset 19 lines).
patching file modules/block/block.module
Hunk #1 succeeded at 762 (offset 24 lines).
patching file modules/simpletest/tests/form.test
patching file modules/simpletest/tests/form_test.module
patching file modules/system/system.api.php
webchick’s picture

Status: Reviewed & tested by the community » Needs work
Berdir’s picture

drupal_alter() moved to module.inc in http://drupal.org/cvs?commit=358286

mikey_p’s picture

Status: Needs work » Needs review
FileSize
15.05 KB

Reroll.

Status: Needs review » Needs work

The last submitted patch, 765860-37.drupal_alter-multiple.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
14.94 KB

Rerole

Status: Needs review » Needs work

The last submitted patch, 765860-37.drupal_alter-multiple.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
16.22 KB

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

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Looks good again.

sun’s picture

Issue tags: +API change
FileSize
14.66 KB

Fixed missing newline, cleaned up phpdoc, and removed unnecessary docblock changes in block.api.php. Also, *bump*.

cha0s’s picture

Great job all. Love the new approach.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

andypost’s picture

Priority: Critical » Normal
Status: Fixed » Needs work
Issue tags: +Needs documentation
+++ includes/form.inc	27 Apr 2010 19:19:29 -0000
@@ -774,11 +774,8 @@ function drupal_prepare_form($form_id, &
-  // Invoke hook_form_FORM_ID_alter() implementations.
-  drupal_alter('form_' . $form_id, $form, $form_state);
-
-  // Invoke hook_form_alter() implementations.
-  drupal_alter('form', $form, $form_state, $form_id);
+  // Invoke hook_form_alter() and hook_form_FORM_ID_alter() implementations.
+  drupal_alter(array('form', 'form_' . $form_id), $form, $form_state, $form_id);

Hook execution order is changed for hook_form_alter() and hook_form_FORM_ID_alter()

So this should be documented.

rfay’s picture

Assigned: Unassigned » rfay

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

dww’s picture

@andypost #47: from the patch...

diff -u -p -r1.158 system.api.php
--- modules/system/system.api.php	27 Apr 2010 09:59:00 -0000	1.158
+++ modules/system/system.api.php	27 Apr 2010 19:19:29 -0000
@@ -748,7 +748,11 @@ function hook_page_alter(&$page) {
  * altering a node form, the node object retrieved at from $form['#node'].
  *
  * Note that instead of hook_form_alter(), which is called for all forms, you
- * can also use hook_form_FORM_ID_alter() to alter a specific form.
+ * can also use hook_form_FORM_ID_alter() to alter a specific form. For each
+ * module (in system weight order) the general form alter hook implementation
+ * is invoked first, then the form ID specific alter implementation is called.
+ * After all module hook implementations are invoked, the hook_form_alter()
+ * implementations from themes are invoked in the same manner.
  *
  * @param $form
  *   Nested array of form elements that comprise the form.

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

andypost’s picture

@dww I mean to reflect this change at http://drupal.org/update/modules/6/7

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
4.53 KB

Yay! 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.

moshe weitzman’s picture

We are adding a call to drupal_alter() from within drupal_alter()? This is OK?

effulgentsia’s picture

It'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?

yched’s picture

Awesome patch. Could be really useful for hook_field_attach_X() hooks. Created #785104: enrich hook_field_attach_X() with per-entity-type variants.

effulgentsia’s picture

Re-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().

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Thx 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().

Dries’s picture

Version: 7.x-dev » 8.x-dev

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

effulgentsia’s picture

Title: Make drupal_alter() support multiple alter hooks executed by module weight » drupal_alter() fails to order modules correctly in some cases
Version: 8.x-dev » 7.x-dev
Category: task » bug

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

andypost’s picture

Looks like reasonable bug fix not a feature.

marcingy’s picture

rfay’s picture

Assigned: rfay » Unassigned
Issue tags: -Needs documentation

Added the requested doc to 6/7 module upgrade page: http://drupal.org/update/modules/6/7#form_alter_change

effulgentsia’s picture

sun’s picture

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

ksenzee’s picture

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

sun’s picture

drunken monkey’s picture

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

tom_o_t’s picture

David_Rothstein’s picture

Does 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...)

effulgentsia’s picture

Does this still need the "API change" tag?

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

webchick’s picture

Dries was not a fan of this back in #57. Leaving to him to decide.

effulgentsia’s picture

dawehner’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7
moshe weitzman’s picture

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +API change, +Needs backport to D7

The last submitted patch, 765860-55.drupal_alter-followup.patch, failed testing.

xjm’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
4.29 KB

Rerolled against current D8 codebase; setting back to RTBC.

xjm’s picture

Tagging issues not yet using summary template.

sven.lauer’s picture

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

ralf.strobel’s picture

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

sun’s picture

Still looks ready to me.

drywall’s picture

+1 to comments #79 and #80.

BenK’s picture

Subscribing

webchick’s picture

Assigned: Unassigned » Dries

Moving to Dries's queue.

The gist is this breaks hook_module_implements_alter() and hook_form_alter() when used together.

carn1x’s picture

subscribe

mikey_p’s picture

Updated patch for D7 (with -p1 for those of us using drush_make).

franzkewd’s picture

Subscribe

ksenzee’s picture

Status: Reviewed & tested by the community » Needs review

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

sven.lauer’s picture

a) if my understanding is correct, b) if that's the behavior we want or need

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

+        // Let modules adjust the order solely based on the primary hook. This
+        // ensures the same module order regardless of whether this if block
+        // runs. Calling drupal_alter() recursively in this way does not result
+        // in an infinite loop, because this call is for a single $type, so we
+        // won't end up in this code block again.
c) if so, where and how we should document it.

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().

donahoche’s picture

drupal_alter-with_ids.patch queued for re-testing.

agentrickard’s picture

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

yoyek’s picture

This is a must for D7

xjm’s picture

Issue tags: +D7DX
FileSize
5.44 KB

Rerolled for Patchmageddon.

sven.lauer’s picture

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

xjm’s picture

Regarding #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.

Dries’s picture

Assigned: Dries » Unassigned
Status: Needs review » Fixed

Committed to 8.x and back ported to 7.x.

chx’s picture

Priority: Normal » Critical
Status: Fixed » Active

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

marcingy’s picture

Version: 8.x-dev » 7.x-dev
effulgentsia’s picture

Issue tags: -Needs backport to D7

@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?

chx’s picture

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

marcingy’s picture

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

marcingy’s picture

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

effulgentsia’s picture

Issue tags: -API change

So the main issue seems to be is this an API change or not for d7

Personally, 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.

andypost’s picture

Status: Active » Needs work
+++ b/core/modules/system/system.api.phpundefined
@@ -1467,6 +1467,15 @@ function hook_mail_alter(&$message) {
+ * a single hook. Thus, to ensure that your implementation of
+ * hook_form_FORM_ID_alter() is called at the right time, you will have to
+ * have to change the order of hook_form_alter() implementation in

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

webchick’s picture

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

webchick’s picture

Issue tags: +Release blocker

.

effulgentsia’s picture

_form_alter's hooks now invokes in unpredictable order?

The situation is this:

  • Before #96, if you implemented hook_module_implements_alter(&$implementations, $hook) and specifically changed the order of $implementations for $hook='form_alter', then whether or not Drupal followed this order for a particular FORM_ID depended on whether or not *any* module implemented hook_form_FORM_ID_alter() without also implementing hook_form_alter(), which for practical purposes meant that for many forms, Drupal did not obey the order explicitly desired by the module that implemented hook_module_implements_alter().
  • After #96, Drupal starts obeying hook_module_implements_alter() the same way for all forms.
  • For sites without any hook_module_implements_alter() implementations targeting $hook='form_alter', there is no change that occurs as a result of #96.

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.

catch’s picture

given it already has at at least one production Drupal 7 site.

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

marcingy’s picture

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

David_Rothstein’s picture

Backing 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

  1. Normal case:
    1. module_a_form_alter()
    2. module_a_form_FORM_ID_alter()
    3. module_b_form_alter()
  2. If Module C implements hook_module_implements_alter() for hook 'form_alter' and tries to put Module B's implementation first:
    1. module_a_form_alter()
    2. module_a_form_FORM_ID_alter()
    3. module_b_form_alter()

    I.e., no change from above; the hook implementation was simply ignored. This is the bug.

  3. If Module C implements hook_module_implements_alter() for hook 'form_FORM_ID_alter' and tries to put Module A's implementation last:
    1. module_a_form_alter()
    2. module_a_form_FORM_ID_alter()
    3. module_b_form_alter()

    Still no change from above.

After the patch

  1. Normal case:
    1. module_a_form_alter()
    2. module_a_form_FORM_ID_alter()
    3. module_b_form_alter()

    Same behavior as before the patch.

  2. If Module C implements hook_module_implements_alter() for hook 'form_alter' and tries to put Module B's implementation first:
    1. module_b_form_alter()
    2. module_a_form_alter()
    3. module_a_form_FORM_ID_alter()

    New behavior. This fixes the bug.

  3. If Module C implements hook_module_implements_alter() for hook 'form_FORM_ID_alter' and tries to put Module A's implementation last:
    1. module_a_form_alter()
    2. module_a_form_FORM_ID_alter()
    3. module_b_form_alter()

    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.

David_Rothstein’s picture

All that said, I think this patch is legitimately "needs work" for a couple reasons:

  1. If people are this confused and uncertain about the patch, it suggests the tests need to be more clear and more comprehensive. Would tests based off the exact scenarios I described in #110 be a good idea?
  2. Looking at the documentation called out in #104:
    + * a single hook. Thus, to ensure that your implementation of
    + * hook_form_FORM_ID_alter() is called at the right time, you will have to
    + * have to change the order of hook_form_alter() implementation in
    + * hook_module_implements_alter().
    

    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.

webchick’s picture

Category: bug » task
Priority: Critical » Normal
Issue tags: -Release blocker

Ok, 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.

Jody Lynn’s picture

Comment removed because I was just behind the times and updating to 7.10 fixed my issue.

farald’s picture

I 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

<?php
/**
 * Adjust module weight so this module's hook_field_presave is
 * fired before geocoder.
 */
function custom_update_7100() {
 db_update('system')
    ->fields(array('weight' => -1))
    ->condition('name', 'custom', '=')
    ->execute();
  return t('Custom module weight set to -1');    
};?>
andypost’s picture

Issue summary: View changes

looks not fixed for d7

  • Dries committed 19a45ce on 8.3.x
    - Patch #765860 by effulgentsia, dww, dereine, mikey_p, sun: make...
  • Dries committed 835068d on 8.3.x
    - Patch #765860 by effulgentsia, dww, dereine, mikey_p, xjm, sun, sven....

  • Dries committed 19a45ce on 8.3.x
    - Patch #765860 by effulgentsia, dww, dereine, mikey_p, sun: make...
  • Dries committed 835068d on 8.3.x
    - Patch #765860 by effulgentsia, dww, dereine, mikey_p, xjm, sun, sven....

  • Dries committed 19a45ce on 8.4.x
    - Patch #765860 by effulgentsia, dww, dereine, mikey_p, sun: make...
  • Dries committed 835068d on 8.4.x
    - Patch #765860 by effulgentsia, dww, dereine, mikey_p, xjm, sun, sven....

  • Dries committed 19a45ce on 8.4.x
    - Patch #765860 by effulgentsia, dww, dereine, mikey_p, sun: make...
  • Dries committed 835068d on 8.4.x
    - Patch #765860 by effulgentsia, dww, dereine, mikey_p, xjm, sun, sven....

  • Dries committed 19a45ce on 9.1.x
    - Patch #765860 by effulgentsia, dww, dereine, mikey_p, sun: make...
  • Dries committed 835068d on 9.1.x
    - Patch #765860 by effulgentsia, dww, dereine, mikey_p, xjm, sun, sven....