#2106127: Set weight to 20 so reroute_email comes after other hook_mail_alter implementations for 6.x prompted an analogous commit to 7.x (ceed2c) to set the module weight, but in 7.x we can more sensibly use hook_module_implements_alter() to adjust the weight of reroute_email_mail_alter() without affecting other hooks, and without making assumptions that 99 is going to be higher than other modules.

I also see that this interacts with the tests/reroute_email_test.module which installs with weight 100 so that it runs next. Obviously this case can also be covered by the hook_module_implements_alter() solution.

CommentFileSizeAuthor
#2 reroute_email-7.x-1.x-revert-weight-implement-hook_module_implements_alter-2241857-2.patch3.99 KBdydave

Comments

jweowu’s picture

jweowu commented
Issue summary: View changes
dydave’s picture

dydave commented
Status: Active » Needs review
Related issues: +#2106127: Set weight to 20 so reroute_email comes after other hook_mail_alter implementations
StatusFileSize
new reroute_email-7.x-1.x-revert-weight-implement-hook_module_implements_alter-2241857-2.patch3.99 KB

Hi @jweowu,

Thank you very much for your follow up on the issue #2106127: Set weight to 20 so reroute_email comes after other hook_mail_alter implementations and posting back your feedback.
I'm very happy you brought up hook_module_implements_alter since I wasn't aware such a hook existed and never actually came accross it in contrib or core. I wish this could have been mentioned earlier, while we were discussing changing the weight of the module at #2106127: Set weight to 20 so reroute_email comes after other hook_mail_alter implementations, before it got committed (ceed2ce), more than 8 months ago.

In any case, I would be glad to discuss the possibility of changing the implementation with weights in favor of using hook_module_implements_alter and would have a couple of small concerns to express:

  1. Would there be any cases where a module would have to run after reroute_email? In which case the weight would perhaps be more practical.
  2. Overhead of hook_module_implements_alter against weight in DB (minor factor since reroute_email is supposed to be a devel module, so not intended for production sites). Could the performace differences be non negligeable?
  3. Potential conflicts with other modules implementing hook_module_implements_alter/mail_alter

Otherwise, if anybody would like to take a closer look at the code changes this approach would imply, please find attached to this comment an initial quick patch against the 7.x-1.x branch.
File attached as: reroute_email-7.x-1.x-revert-weight-implement-hook_module_implements_alter-2241857-2.patch.

Since I don't really have any experience using this hook before and always worked with module weights, I would certainly greatly appreciate anyone's feedback on this approach.

Feel free to let me know if you would have any questions, comments or suggestions about the code changes, the patch for 7.x-1.x, suggested approach or any other aspects discussed in this ticket, I would be glad to explain in more details.

Thanks in advance to everyone for your testing, reporting, reviews, comments and feedbacks.
Cheers!

jweowu’s picture

jweowu commented

I'll review the patch sometime later, but in answer to your numbered questions:

1. The hook is more practical than the weight, because (as you've done) you can simply move your implementation to the end of the list, no matter what the weight was. The weights affect the sequence prior to alteration, but this alter hook has the final say.

2. module_implements() writes the results of the alterations to persistent cache, so there are no performance concerns to speak of -- generally the list is being read from cache in a pre-altered form.

3. For sure, but that's Drupal for you. If multiple modules want to attain priority, you can't necessarily do much about it. I think module weight probably affects the sequence in which this hook's implementations are called, but I feel that trying to enforce that is a step too far. You'll still have no control over the weight that other modules are using, so you don't really know what the end result will be.

dydave’s picture

dydave commented

Hi @jweowu,

Once again, thank you very much for keeping a friendly eye on the module.

All of your previous answers sound great and thank you for taking the time to have them documented in this ticket, just in case anyone else could have any objections.

I'll review the patch sometime later

Were you able to take a little bit of time to review the patch by any chance?

Unfortunately there hasn't been any more feedback on this issue, so before being able to move forward, it would be great if somebody else could give this patch a round of testing and review.

Please let me know if you would have any questions, comments or suggestions for the proposed patch or any other aspects in this ticket, I would be glad to provide more information.
Thanks in advance to everyone for your testing, reporting, reviews, comments and feedbacks.
Cheers!

jweowu’s picture

jweowu commented

If I've not chased this up in a week's time, please bump it again.

dydave’s picture

dydave commented

Hi @jweowu,

Thanks again very much for your follow-up and your help is definitely greatly appreciated as I can imagine you must be very busy.

Quick friendly bump:

Unfortunately there hasn't been any more feedback on this issue, so before being able to move forward, it would be great if somebody else could give this patch (from #2) a round of testing and review.

Any of your comments, questions, feedback, concerns, objections or suggestions would certainly be greatly appreciated.

Thanks in advance to everyone for your testing, reporting, reviews, comments and feedbacks.
Cheers!

jweowu’s picture

jweowu commented

Sorry, the past week+ has indeed been non-stop. Thanks for the reminder, and I may still need another one later, but I'm hoping I'll have some free time by the end of the week.

dydave’s picture

dydave commented

Alright, back for more bumping :-)

Thanks again a lot @jweowu for your efforts following-up on this issue, as I can't possibly imagine how busy you must be.

I thought leaving a little bit more time before bumping the ticket could potentially allow others to step in and land a hand, but...

Unfortunately there hasn't been any more feedback on this issue, so before being able to move forward, it would be great if somebody else could give this patch (from #2) a round of testing and review.

It would be great if you could perhaps spare 20 to 30 minutes to quickly test the patch manually, go over the code changes and post/report back in this ticket.
This would really allow us to unblock this issue, get it committed and finally release a new stable version of the Reroute Email module.

Any of your comments, questions, feedback, concerns, objections or suggestions would certainly be greatly appreciated.

Thanks in advance to everyone for your testing, reporting, reviews, comments and feedbacks.
Cheers!

skwashd’s picture

skwashd commented
Status: Needs review » Reviewed & tested by the community

Patch #2 looks good to me. This quite a common pattern for more advanced modules. See the list of hook_module_implements_alter() implementations on drupalcontrib.org.

  • DYdave committed cd5dcbd on 7.x-1.x 
    Issue #2241857 by DYdave, jweowu, skwashd: Reverted 'module weight = 99...
dydave’s picture

dydave commented
Status: Reviewed & tested by the community » Fixed

Hi @skwashd,

Thank you very much for your feedback and review of the patch.
I'm very happy to hear the patch could be committed as this issue was preventing us from releasing a new stable version of the module and had been stalling for a while...

Anyhow, with you green light, I went ahead and committed the patch from #2 against reroute_email-7.x-1.x at cd5dcbd.

Just a quick note:
After reviewing some of the implementations of hook_module_implements_alter for some contrib modules (link provided in @skwashd's comment), in particular commerce_product_reference_module_implements_alter, entity_module_implements_alter or ds_module_implements_alter, among others, I noticed there was no isset in the if ($hook == [HOOK_NAME]).
So after checking the hook_module_implements_alter's API page again, I found the comment Altering the order for a specific form ID, in particular:

The extra isset on the $implementations array is needed in case you don't actually implement hook_form_alter() as well.

Since in our case, reroute_email and reroute_email_test both implement hook_mail_alter, the isset($implementations['reroute_email']) tests are not necessary.

Which I would assume should pretty much wrap up this issue, so I allowed myself to mark it as fixed for now, but feel free to re-open it, or post a new ticket, at any time if you have any further objections with the changes committed at cd5dcbd (we would surely be happy to hear your feedback).

Please let me know if you would have any further comments, feedback, questions, issues, objections, suggestions or concerns on any of the latest code changes or this ticket in general, I would be glad to provide more information or explain in more details.

Thanks in advance to everyone for your testing, reviews, reporting and feedback.
Cheers!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.