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.
To remain consistent with the _invoke functions of other modules, user_module_invoke() should be renamed to user_invoke().
Also, user_login_block() should be renamed to something like user_login_block_form() or even _user_login_block_form(), as its name suggests it were an implementation of hook_block() though it is only an page handler.
A patch is enclosed.
Comment | File | Size | Author |
---|---|---|---|
#26 | drupal8.user-login.26.patch | 3.5 KB | sun |
#8 | user-remove_user_module_invoke-200344-7.patch | 3.53 KB | pdrake |
#5 | user_module-rename_user_module_invoke_and_user_login_block-200344-5.patch | 3.72 KB | pillarsdotnet |
#1 | user_module_cleanup.patch | 3.68 KB | Pancho |
user_module_cleanup.patch | 3.74 KB | Pancho | |
Comments
Comment #1
PanchoDidn't apply anymore. Rerolled against HEAD.
Comment #2
PanchoAs this is an API change, i bump it to D7.
Comment #3
gddNo longer applies to HEAD, needs a reroll
Comment #4
casey CreditAttribution: casey commentedD7 is in alpha fase already.
Comment #5
pillarsdotnet CreditAttribution: pillarsdotnet commentedUpdated patch.
Comment #6
pillarsdotnet CreditAttribution: pillarsdotnet commented#5: user_module-rename_user_module_invoke_and_user_login_block-200344-5.patch queued for re-testing.
Comment #7
marcingy CreditAttribution: marcingy commentedThis is really a task not a bug as nothing is actually broken.
Comment #8
pdrake CreditAttribution: pdrake commented$edit used to be modified in session_test_user_login() (see http://drupal.org/node/280934) but no longer modified by it, so $edit no longer needs to be passed by reference, and module_invoke_all should be a sufficient replacement for user_module_invoke().
Comment #10
pdrake CreditAttribution: pdrake commented#8: user-remove_user_module_invoke-200344-7.patch queued for re-testing.
Comment #12
pdrake CreditAttribution: pdrake commented#8: user-remove_user_module_invoke-200344-7.patch queued for re-testing.
Comment #13
christianchristensen CreditAttribution: christianchristensen commentedLooks good to me!
Comment #14
travist CreditAttribution: travist commentedJust pulled this in, and tested it. Looks good.
Comment #15
catchLooks great! Committed/pushed to 8.x. We should add a change notice for the removed function.
Comment #16
sun@catch meant to set this status.
Yes, we definitely need a change notice here, as I was desperately searching for what happened to
user_module_invoke()
— apparently,user_module_invoke('login')
(and the 'logout') counterpart is used by modules like Masquerade that are invoking discrete login/logout operations.And apparently, we no longer seem to have an API for this.
user_module_invoke()
, although part of a larger anti-API pattern, was at least sort-of-API-like — with this change here, it really feels a bit "random" to invokemodule_invoke_all('user_login')
...So in addition to the change notice, we should create the following follow-up issue(s):
hook_user_login()
.hook_user_logout()
works identically.Comment #17
sun@pdrake: Are you up for writing a change notice for this? :)
Comment #18
pdrake CreditAttribution: pdrake commented@sun, sure. I've never done that before and could use some real specific direction, but definitely willing to give it a shot.
Comment #19
YesCT CreditAttribution: YesCT commented@pdrake I'm about to do a change notice (also my first) for a different issue.
So I can at least point you to:
http://drupal.org/node/add/changenotice
and
http://drupal.org/node/1314040 doc which includes "How to create a Change record"
Comment #20
webchickThis has been holding up a spot in the criticals list for over 5 weeks now. Can we get a change notice added, please?
Comment #21
pdrake CreditAttribution: pdrake commentedOk, I gave it a shot: http://drupal.org/node/1879024
Comment #22
sunThanks!
1) We're missing the actual API change in the change notice; i.e., a clarification on how existing code needs to be updated from D7 to D8. (i.e., like the hook implementations in the committed patch).
2) The change notice should also include some guidance for the issues mentioned in #16 - in particular for modules that previously called
user_module_invoke()
in order to log in/out a user; i.e., what are such modules supposed to do.After clarifying those parts, the change notice should be done.
Does anyone want to take on the follow-up issue for the three points mentioned at #16? (I think they can be bundled into a single issue, and we probably want to update/enhance the change notice for this issue afterwards.)
Comment #23
andypostAs masquerade module maintainer I just need this
Comment #24
pdrake CreditAttribution: pdrake commented@sun, I made some changes to the change request, please review and let me know if anything is still missing. I wasn't entirely clear on how your points #1 and #2 differ, so I may not have addressed point #2 adequately.
Comment #25
BerdirHm.
I'm not sure how official that was, but $edit is actually the $form_state when logging in through the default user login block. Which meant that you could implement a redirect on login by setting $edit['redirect'] = 'something';
I guess that now means that you have to alter the form, add a submit callback , check if the user logged in successfully and then set $form_state['redirect'] there. Because drupal_goto() in hook_user_login() is bad :) Not sure if that should be documented as it was not very obvious before either.
Comment #26
sunI've updated and clarified the change notice: http://drupal.org/node/1879024
@Berdir is right in #25 - $edit is actually $form_state. That's ugly for two reasons:
1) I wrongly assumed that $edit would be the submitted form values only; i.e., $form_state['values'] instead of $form_state. If it's $form_state, then it should be called $form_state.
2) The entire general futzing with $edit dates back to the old D6 User API, which passed around $edit AND $account at the same time. However, that has been eliminated to >90% for D7 already, leaving only $account. The entire $edit parameter should actually be obsolete.
However, @Berdir is right that $edit ($form_state) also allowed hook implementations to manipulate the form redirection after logging in. But then again, that should be obsolete by now, since D8 only has one user login form, and thus, hook_form_alter-ing it to inject a form submission handler is straightforward.
Therefore, I think we want to eliminate the $edit argument entirely — which, apparently, makes
hook_user_login()
consistent with hook_user_logout(), because it doesn't have an $edit argument already.Attached patch does so.
Comment #28
Berdir#26: drupal8.user-login.26.patch queued for re-testing.
Comment #29
BerdirLooks good to me. Change notice looks great as well, needs to be changed a bit after the patch was commited, though.
Comment #30
catchMakes sense. Committed/pushed to 8.x, back to active for the change notice.
Comment #31
BerdirUpdated the change notice to remove $edit completely.
Comment #33
xjmUntagging. Please remove the "Needs change notification" tag when the change notice task is complete.