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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Pancho’s picture

FileSize
3.68 KB

Didn't apply anymore. Rerolled against HEAD.

Pancho’s picture

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

As this is an API change, i bump it to D7.

gdd’s picture

Status: Needs review » Needs work

No longer applies to HEAD, needs a reroll

casey’s picture

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

D7 is in alpha fase already.

pillarsdotnet’s picture

Title: Clean up user.module » Rename user_module_invoke() to user_invoke() and user_login_block() to user_login_block_form()
Status: Needs work » Needs review
FileSize
3.72 KB

Updated patch.

pillarsdotnet’s picture

marcingy’s picture

Category: bug » task

This is really a task not a bug as nothing is actually broken.

pdrake’s picture

Title: Rename user_module_invoke() to user_invoke() and user_login_block() to user_login_block_form() » Remove user_module_invoke() and rename user_login_block() to user_login_block_form()
FileSize
3.53 KB

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

Status: Needs review » Needs work

The last submitted patch, user-remove_user_module_invoke-200344-7.patch, failed testing.

pdrake’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, user-remove_user_module_invoke-200344-7.patch, failed testing.

pdrake’s picture

Status: Needs work » Needs review
christianchristensen’s picture

Looks good to me!

travist’s picture

Status: Needs review » Reviewed & tested by the community

Just pulled this in, and tested it. Looks good.

catch’s picture

Title: Remove user_module_invoke() and rename user_login_block() to user_login_block_form() » Change notice: Remove user_module_invoke() and rename user_login_block() to user_login_block_form()
Priority: Normal » Critical
Status: Reviewed & tested by the community » Fixed

Looks great! Committed/pushed to 8.x. We should add a change notice for the removed function.

sun’s picture

Status: Fixed » Active
Issue tags: +API clean-up, +Needs change record

@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 invoke module_invoke_all('user_login')...

So in addition to the change notice, we should create the following follow-up issue(s):

  1. Remove the $edit argument from hook_user_login().
  2. Make sure that hook_user_logout() works identically.
  3. Re-instantiate API functions for logging in/out a user.
sun’s picture

@pdrake: Are you up for writing a change notice for this? :)

pdrake’s picture

@sun, sure. I've never done that before and could use some real specific direction, but definitely willing to give it a shot.

YesCT’s picture

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

webchick’s picture

This has been holding up a spot in the criticals list for over 5 weeks now. Can we get a change notice added, please?

pdrake’s picture

Status: Active » Needs review

Ok, I gave it a shot: http://drupal.org/node/1879024

sun’s picture

Thanks!

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

andypost’s picture

As masquerade module maintainer I just need this

+++ b/core/modules/user/user.module
@@ -1731,7 +1708,7 @@ function user_login_finalize(&$edit = array()) {
-  user_module_invoke('login', $edit, $user);
+  module_invoke_all('user_login', $edit, $user);
 }
//D7 code.
$edit = array();
user_module_invoke('login', $edit, $user);
// D8 code.
$edit = array();
module_invoke_all('user_login', $edit, $user);
pdrake’s picture

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

Berdir’s picture

Hm.

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.

sun’s picture

Title: Change notice: Remove user_module_invoke() and rename user_login_block() to user_login_block_form() » Remove user_module_invoke() and rename user_login_block() to user_login_block_form()
Priority: Critical » Major
FileSize
3.5 KB

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

Status: Needs review » Needs work
Issue tags: -API clean-up, -Needs change record

The last submitted patch, drupal8.user-login.26.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +API clean-up, +Needs change record

#26: drupal8.user-login.26.patch queued for re-testing.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me. Change notice looks great as well, needs to be changed a bit after the patch was commited, though.

catch’s picture

Title: Remove user_module_invoke() and rename user_login_block() to user_login_block_form() » Change notice: Remove user_module_invoke() and rename user_login_block() to user_login_block_form()
Priority: Major » Critical
Status: Reviewed & tested by the community » Active

Makes sense. Committed/pushed to 8.x, back to active for the change notice.

Berdir’s picture

Title: Change notice: Remove user_module_invoke() and rename user_login_block() to user_login_block_form() » Remove user_module_invoke() and rename user_login_block() to user_login_block_form()
Priority: Critical » Normal
Status: Active » Fixed

Updated the change notice to remove $edit completely.

Status: Fixed » Closed (fixed)

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

xjm’s picture

Issue tags: -Needs change record

Untagging. Please remove the "Needs change notification" tag when the change notice task is complete.