Rename format_username() now that it's in user.module

This is a task not a bug.

+++ b/includes/theme.inc
@@ -1931,40 +1931,6 @@ function theme_more_link($variables) {
- * @see template_preprocess_username()
- * @see template_process_username()

Looks like you forgot these?

1 days to next Drupal core point release.

eek, patch serials are a wonky to review... can you roll a single?

I'd be happy with this, but I'd like @catch to RTBC it.

I don't know why it doesn't apply, but it doesn't:

Hunk #1 succeeded at 2000 (offset 23 lines).
Hunk #2 succeeded at 6486 with fuzz 2 (offset 26 lines).
patching file includes/theme.inc
Hunk #1 succeeded at 1966 (offset 34 lines).
Hunk #2 succeeded at 2648 with fuzz 2 (offset 111 lines).
patching file modules/user/user.module
Reversed (or previously applied) patch detected!  Assume -R? [n] 

The patch is reversed - its moving code from user.module to includes/common.inc.

Shot in the dark with the new /core file structure?

The testing bot likes it, and it really helps with cleaning up common.inc and theme.inc. Applied, cleared cache... Usernames are still being displayed correctly. I'm marking this one RTBC.

+1 on the rtbc as Rob Loach shouldn't really be rbtc'ing his own patch!

> Can this be backported to 7.x? Is it considered an API break? The theme functions still exist...they're just moved around.

They're moved from essential include files to a required module... nobody's site will break and theme functions will continue to work. So doesn't sound like an API break to me.

Very nice. Committed/pushed to 8.x. Thanks!

Well I was a bit trigger happy.

Now we have format_username() in user.module, which completely violates coding standards for namespacing of functions.

This issue is short so let's do the follow-up here.

Yeah, let's move it to user.module too.

Whoops, missed one.

Sun also suggested user_format_name() to avoid the duplicate "user" :-) .

Wow, what is wrong with me? #PatchFail

What are your thoughts on somehow merging user_label() and user_format_name()? Maybe switching the "label callback" arguments around so that the $entity is first and $entity_type defaults to "user"?

Like this... It's still user_format_name(), but we save ourselves from having to define two functions.

Missed one test...

I reversed the argument order in the label callback function. So it becomes entity_label($entity, $entity_type). This means user_format_name($account) works as the $entity_type just defaults to "user".

Hmm... we specifically chose that parameter order in #1096446: entity_label() is not passing along the $entity_type parameter because "$entity_type, $entity" is the standard in D7/8. Sadly it's not 100% consistent over the place, but in general I think we should standardize on that. Also, the whole debate will become obsolete when #1184944: Make entities classed objects, introduce CRUD support lands, so I'm not sure it would make sense to switch things around just before that again.

Please leave that entity_label() discussion out of this issue. You may want to search the queue for entity system, $entity_type, and entity_label() related discussions.

Looks like #31 is RTBC. Can someone confirm?

Re-uploading that patch to prevent confusion wouldn't hurt either.

Hrm, now we have a mismatch with the alter hook. Before it was format_username() and hook_username_alter(). Now we would have user_format_name() and hook_username_alter(). I'm not really happy with the names of those two diverging as well.

@sun Please leave that entity_label() discussion out of this issue. You may want to search the queue for entity system, $entity_type, and entity_label() related discussions. Re-uploading that patch to prevent confusion wouldn't hurt either.

Deal!

@tstoeckler we specifically chose that parameter order in #1096446: entity_label() is not passing along the $entity_type parameter because "$entity_type, $entity" is the standard in D7/8. Sadly it's not 100% consistent over the place, but in general I think we should standardize on that.

Personally, I think the label callback arguments make more sense as entity_label($entity, $entity_type), but I suppose that's for another issue, like sun suggested.

#1184944: Make entities classed objects, introduce CRUD support looks like a pretty awesome issue though!!! I'll get involved over there. Thanks for the link.

@Dave Reid Hrm, now we have a mismatch with the alter hook. Before it was format_username() and hook_username_alter(). Now we would have user_format_name() and hook_username_alter(). I'm not really happy with the names of those two diverging as well.

Maybe hook_user_name_alter()?.... Or, user_format_name() might live nicely beside hook_user_format_name_alter()?

So, looks like:

• We are renaming format_username() to user_format_name().
• We need to rename the alter hook. Everyone OK with hook_user_format_name_alter()?

See also: #1361228: Make the user entity a classed object.

+++ b/core/modules/user/user.module
@@ -193,10 +193,10 @@ function user_uri($user) {
+ * @see user_format_name

Since we are already changing this, the function should be appended by "()" for it to be linked on api.drupal.org

I agree with #41 on the alter hook.

-11 days to next Drupal core point release.

We could get rid of the function entirely if we change the parameter order of entity_label(), but that's a discussion for another issue. I'll hopefully get around to putting together the follow up issue soon. Maybe in the one xjm mentioned #1361228: Make the user entity a classed object. Thanks!

@xjm We are renaming format_username() to user_format_name().

Yup!

@xjm We need to rename the alter hook. Everyone OK with hook_user_format_name_alter()?

Sounds good! Let's move that into user.api.php too.

@tstoeckler Since we are already changing this, the function should be appended by "()" for it to be linked on api.drupal.org

Good catch.

One occurence of format_username() in the documentation for t() was missed.
This one should fix that.

Applied the patch and checked:

[kepler:drupal | Thu 16:36:31]$ grep -r "format_username" *
[kepler:drupal | Thu 16:36:36]$ grep -r "username_alter" *
[kepler:drupal | Thu 16:37:40]$ grep -r "drupal_alter('username" *
[kepler:drupal | Thu 16:38:57]$ grep -r 'drupal_alter("username' *

No results, so looks like we got everything.

This is SO close to RTBC, except for one eensy tiny thing....

+++ b/core/modules/user/user.moduleundefined
@@ -1376,9 +1376,9 @@ function user_preprocess_block(&$variables) {
- * may override this by implementing hook_username_alter(&$name, $account).
+ * may override this by implementing hook_user_format_name_alter(&$name, $account).

This is over 80 chars and so we should wrap the function to the next line.

Aw heck I will just fix it and RTBC anyway.

* xjm begs forgiveness

Tagging.

Feel free to disagree and call me nuts, but shouldn't this be $account->label() now or very very soonish?

Re #50: Yes, that's correct.
Because of the parameter order thing, we don't use user_format_name() directly as label callback, but once we have $user = entity_create('user', array());> the whole $entity_type thing is gone, and we will call $user->label();. (Strictly speaking, we could already change all those calls to entity_label('user', $user); which would pretty much be the same thing.)

For now, though, Entity::save() simply calls the label callback, so we would still need this function (or move it directly to user_label()). We could also override Entity::save() with User::save(), which would then move the code directly into the class, but I don't think that's part of #1361228: Make the user entity a classed object initially.

I also don't know (but I don't think so) if that issue handles global $user;, for which we also need this function.

So either way, I think this is a step in the right direction, and it shouldn't conflict with making $user a classed object (I think :) ...).

gosh... apparently I refused to talk about entity labels in #38 of this issue ;)

However, time goes by, and now that there's some actual code behind the effort in #1361228: Make the user entity a classed object, I do wonder whether this change is still necessary and/or whether we're going to unnecessarily break other patches/changes down the line by committing it. As this only applies to D8, I'd actually be inclined to hold it off on aforementioned patch, and directly or indirectly move to $account->label().

In the entity conversion issues we've seen so far, we are retaining various functions as procedural wrappers for entity methods. So I think it's entirely reasonable, perhaps better, to commit this now. At most the user entity patch would need to reroll one or two hunks, I think? And that issue is still NW.

I've gone ahead and committed this, it's likely only minor conflicts at most with the entity conversion patch, and it's a better target for completely removing the function if we end up doing that if it's properly owned by user module.

This will need a change notification.

Thanks!... And agreed.

Added a change notification: http://drupal.org/node/1408514
Could maybe have a look-over before being marked fixed.

Looks good to me, thanks @tstoeckler! I polished the formatting a little and snuck in a semicolon. :)