(This is a split issue from http://drupal.org/node/188734#comment-629401)

Now we handle username styling with URL linkage by theme_username(), on the other hand handle plain text username with no styling (e.g. use check_plain($user->name)), which may increase difficulty for maintenance.

This patch try to add $options for theme_username(), so we will able to replace check_plain($user->name) as theme('username', $user, array('plain' => TRUE)). It is backward compatible with existing code implementation; on the other hand, it will be very useful if we may able to add free form users.fullname + site-wide display name styling option, which centralize all username styling within theme_username() for simplify overall handling.

CommentFileSizeAuthor
#127 192056_format_username.patch22.31 KBandypost
#121 192056_format_username.patch22.48 KBandypost
#113 format_username-192056-113.patch22.48 KBeffulgentsia
#110 format_username-192056-110.patch22.1 KBeffulgentsia
#108 rname.tgz336 bytesandypost
#108 192056_username.png74.28 KBandypost
#102 theme_username-192056-102.patch22.86 KBeffulgentsia
#101 theme_username-192056-101.patch27.42 KBeffulgentsia
#98 theme_username-192056-98.patch36.24 KBeffulgentsia
#95 ab-theme_username-192056-head.txt1.32 KBeffulgentsia
#95 ab-theme_username-192056-94.txt1.32 KBeffulgentsia
#95 ab-theme_username-192056-drupal_alter.txt1.37 KBeffulgentsia
#94 theme_username-192056-94.patch36.28 KBeffulgentsia
#83 theme_username-192056-83.patch16.34 KBeffulgentsia
#80 theme_username-192056-80.patch13.52 KBeffulgentsia
#79 theme_username-192056-79.patch12.53 KBeffulgentsia
#77 theme_username-192056-75.patch19.65 KBeffulgentsia
#53 theme_username_192056.patch17.68 KBDave Cohen
#50 theme_username_192056.patch16.65 KBDave Cohen
#46 theme_username_192056.patch16.04 KBDave Cohen
#43 theme_username_192056.patch15.7 KBDave Cohen
#42 theme_username_192056.patch15.7 KBDave Cohen
#40 theme_username_192056.patch15.69 KBDave Cohen
#37 theme_username_192056.patch11.02 KBDave Cohen
#33 username-plain-192056-33.patch2.63 KBpwolanin
#11 realname_theme.txt3.14 KBNancyDru
#6 theme_username.patch14.5 KBgeodaniel
#1 drupal-6.x-dev-theme_username-0.2.patch14.88 KBhswong3i
drupal-6.x-dev-theme_username-0.1.patch13.13 KBhswong3i
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hswong3i’s picture

Title: centralize username styling in theme_username() » theme: centralize username styling in theme_username()
FileSize
14.88 KB

Minor update for miss handling and bug fix.

catch’s picture

Version: 6.x-dev » 7.x-dev
geodaniel’s picture

#156266: Option to show theme_username as a link or not to make it much more reusable looks like a simpler version of this. What else might be passed in as part of the array?

Arto’s picture

This would be a useful feature. I'd prefer the $options array over the solution in #156266. Subscribing.

hswong3i’s picture

The $options array is target for future feature expansion, so we will not need to change the function call API whenever new features are add to it.

geodaniel’s picture

FileSize
14.5 KB

Patch re-rolled against current HEAD. I agree that it's better to do it this way and keep it more flexible.

Dries’s picture

What other options would we possibly introduce? I'm not a big fan of premature API optimization -- not unless we can come up with extremely likely use cases.

Robin Monks’s picture

Status: Needs review » Needs work

No longer applies to HEAD

patching file includes/theme.inc
Hunk #1 FAILED at 1536.
Hunk #2 FAILED at 1570.
2 out of 2 hunks FAILED -- saving rejects to file includes/theme.inc.rej
patching file modules/blog/blog.module
Hunk #1 FAILED at 105.
Hunk #2 FAILED at 119.
2 out of 2 hunks FAILED -- saving rejects to file modules/blog/blog.module.rej
patching file modules/blog/blog.pages.inc
Hunk #2 FAILED at 88.
1 out of 2 hunks FAILED -- saving rejects to file modules/blog/blog.pages.inc.rej
patching file modules/comment/comment.module
Hunk #1 succeeded at 1442 with fuzz 1 (offset -8 lines).
patching file modules/contact/contact.pages.inc
Hunk #2 succeeded at 164 with fuzz 1.
Hunk #3 FAILED at 177.
1 out of 3 hunks FAILED -- saving rejects to file modules/contact/contact.pages.inc.rej
patching file modules/openid/openid.pages.inc
Hunk #1 FAILED at 28.
1 out of 1 hunk FAILED -- saving rejects to file modules/openid/openid.pages.inc.rej
patching file modules/statistics/statistics.pages.inc
patching file modules/tracker/tracker.pages.inc
patching file modules/user/user.module
Hunk #1 FAILED at 529.
Hunk #2 succeeded at 766 with fuzz 2 (offset 79 lines).
Hunk #3 succeeded at 2132 (offset -55 lines).
1 out of 3 hunks FAILED -- saving rejects to file modules/user/user.module.rej
patching file modules/user/user.pages.inc
Hunk #1 FAILED at 12.
1 out of 3 hunks FAILED -- saving rejects to file modules/user/user.pages.inc.rej

Robin

JohnAlbin’s picture

Title: theme: centralize username styling in theme_username() » theme_username() should be consistently used
Category: feature » bug

This definitely needs fixing.

NancyDru’s picture

@Dries: I understand, and even share, your concern over "premature API optimization." However, if one couples this with the possibility of a contributed module taking over theme_username (see #102679: Add a Display Name field to core in addition to Username), such as RealName does, there could be other options provided. For example, RealName also has the capability of linking to a user's homepage (provided through Profile); in this case, it might be good to add 'homepage' => FALSE to disable that feature in some cases (like replacing the user name on the Navigation menu block). Actually, there are two other options already in this code: 1) show "Not verified" and 2) maximum length to display.

@geodaniel: I don't see the need to 'plain' => TRUE when building the link on the Navigation menu. As it currently works, without this, one would get a link pointing to the user's page. That allows those of us with longer menus to disable the "My account" item because such a link does the same thing.

NancyDru’s picture

FileSize
3.14 KB

Here's a real example of how a module could take over and expand on this concept. I still need to code for the new 'levels' option, but you can get the idea.

You can now see this in action in the RealName module. And there are places where the options are used.

sun’s picture

This is too limited IMHO. See my reasoning and proposal in #394282-26: Add a standardized full name field to the users table

Dave Cohen’s picture

Dries asked

What other options would we possibly introduce?

We might take hints from facebook. Their fb:name tag has options like possesive (Joe's instead of Joe), useyou ("you" when it's your uid), reflexive (yourself instead of you) and more. While I don't expect Drupal to go in this direction anytime soon, I do think theme('username') is justified in taking an array of options.

Details about facebook's options: http://wiki.developers.facebook.com/index.php/Fb:name

sun’s picture

And again, a pretty simple User Display API decorator was able to mimic those fb:name tag options. Replace the default 'username' decorator with that decorator, and you're done.

We need a flexible system like that, because, to advance on the given example, one often needs to be able conditionally render a username depending on another field value in the user object or attached data. The actual invocation of theme('username'), however, stays the same.

NancyDru’s picture

@sun: I see no reason why one of the options that could be specified might be "display_class." In my implementation of this feature, I think that would be fairly easy to add.

sun’s picture

So why introduce further cruft, if a single display class solves any possible use-case?

Dave Cohen’s picture

I prefer an array of options. That is consistent with other APIs. Also allows for parameters which some implementations of theme_username understand, but others don't. For example, I could write a theme_username for facebook canvas pages which ends up printing the fb:name tag I described in comment 13. In my implementation I could honor 'useyou' or 'reflexive' in the options array, even though other themes would never support them.

Why tie down the API? Why presume we've thought every possible use case?

andypost’s picture

mlncn’s picture

Issue tags: +RDF

theme_username is proving one of the more problematic functions that the RDFa in core effort has to deal with. Tagging with RDF.

More options and having it render later would help.

(Note Make theme_username() able to return username without link as another related issue that could be addressed here.)

NancyDru’s picture

Indeed, one only need look at the issue queue for RealName and see how screwy it all is. BTW, for your other issue, the RealName module also has an implementation of that very feature based on a recommendation in another issue.

dahacouk’s picture

Display Name Code Sprint Saturday September 5th...

Dave Cohen and I just had a really good face to face discussion with Angie (webchick). In summary, we are going to use the Saturday code sprint to make sure that Drupal 7 will facilitate us building contributed modules to implement display names.

This may mean altering some parts of core in a very small way. If you are passionate about cleaning up "theme_username" throughout core and making it easier for contributed modules to implement display names in a clean and consistent way, then join us at DrupalCon Paris or over the wires.

Saturday September 5th 2009 10:00 to 17:00 CEST
Location: DrupalCon Paris and online elsewhere

If you use IRC then #kendra is ready and waiting for you during those times.

I'll make sure developers are kept well feed and watered. So, if you are at DrupalCon and passionate about making it easy to implement display names then get in touch.

This is pretty much our last chance for making Drupal 7 display name friendly! Let's make it happen!

Cheers Daniel

username: dahacouk on skype, jabber, irc...
Kendra Initiative - http://www.kendra.org.uk

NancyDru’s picture

I am definitely passionate about this, but cannot afford a trip to Paris. I also don't know IRC at all.

dahacouk’s picture

@NancyDru: Any other IM? Skype? Can you be online for some or all of those times? Worst case we'll just do email I guess! ;-)

NancyDru’s picture

I have Yahoo Messenger. I don't think I can be there until near the end since I am in US Eastern Time (3am - 10am?) and Saturday is the only day I have to catch up on my sleep from the week and I have a meeting later in the day so a nap is out.

Dave Cohen’s picture

Couple things, while at DrupalCon....

First, I'm going to try to update this patch to apply to the latest head.

Second, because of time differences and we won't be here as long as originally planned, it makes little sense to try to meet up on-line.

I'll post here if I can update the patch shortly...

dahacouk’s picture

To assist a little here is where I found non use of theme_username():

* User's picture "alt" property needs to use theme_username().
* Admin bar needs to use theme_username().
* Authoring information for edit/create page needs to use theme_username().

Also, for what it's worth, here are other related issues:

Postponed: #464888: Use theme('username') in user profile page titles
Duplicate: #465160: Make theme_username() able to return username without link
Duplicate: #156266: Option to show theme_username as a link or not to make it much more reusable

Cheers Daniel

NancyDru’s picture

If it is also used for the Navigation menu title for logged in users, it becomes equivalent to "My account".

pwolanin’s picture

Why not simple add a simple additional theme function for no-markup output? E.g theme('plain_username'); - any theme function would allow for the injection of a display name during pre-process.

Trying to overload theme('username') with this seems problematic to me per comments on #565792: Refactor theme_username so that RDF patch can be accepted.

effulgentsia’s picture

I agree with #28. Whoever is taking the lead on this, please roll a patch with a new theme hook (e.g., 'plain_username'), and if the implementation is sound, I'll be happy to +1 it. This decouples this issue from #565792: Refactor theme_username so that RDF patch can be accepted which is good, and allows both issues to proceed, each at their pace. Personally (though I have no authority to speak for webchick or dries), I think this issue qualifies as a bug-fix and has the code slush window in which to proceed.

NancyDru’s picture

I still prefer the options array technique because it gives the contrib more flexibility in allowing the admin to decide how he/she wants the output to be.

effulgentsia’s picture

Once #565792: Refactor theme_username so that RDF patch can be accepted lands, a contrib module would still have full flexibility, even if these are separate theme hooks. I can appreciate stylistic disagreements, however, as to whether two functions or one function with options is better, and while in this case I prefer the former, I'm not opposed to the latter. However, choosing the latter makes it hard for this patch to proceed until #565792: Refactor theme_username so that RDF patch can be accepted lands.

pwolanin’s picture

Title: theme_username() should be consistently used » the username should always be generated via a theme() function
Assigned: hswong3i » Unassigned
pwolanin’s picture

Title: the username should always be generated via a theme() function » a user name should always be generated via a theme() function
Status: Needs work » Needs review
FileSize
2.63 KB

Here's a quick patch with a couple example conversions.

As I understand the real motivation in the issue it's so that a contrib module can readily set (for example) values like a "real" name from the user profile in place of $user->name every place the user's name is printed.

NancyDru’s picture

e.g. for us in an attribute should be e.g. for use in an attribute

Okay, it just means two theme entries to modify in RealName.

pwolanin’s picture

Somone who cares about this issue should ping Dries/webchick and see if it's still possible for D7.

Status: Needs review » Needs work

The last submitted patch failed testing.

Dave Cohen’s picture

Status: Needs work » Needs review
FileSize
11.02 KB

It's back....

When I talked to webchick at DrupalCon, she considered this a bug and therefore possible to fix even during code slush.

Since the last patch, theme_username has changed a lot. This patch has changed, too. Theme_username now introduces markup (a <span> with attributes) even when the username is not a link. I think this is intended to let you capitalize names with CSS and things like that. Whatever it's for, I didn't want to break it so I introduced new options. Now the options that can be passed into theme_username include 'plain' for just the name, 'link' to control whether an anchor tag should be used and 'shorten' to control whether the theme should limit length to say 20 chars. So just to be clear, under this scheme pass 'plain' => TRUE if you're going to do something like drupal_set_title with the name, but pass 'link' => FALSE (and 'shorten' => FALSE) if your going to display the name in HTML but not as a link.

Again, I didn't add the <span> tag, so don't act like this added complexity was my idea. It wasn't.

Finally, I believe the patch from #33 by pwolanin changed a couple places to use theme_username that didn't necessarily need to. For example setting $comment->name is so that $comment can be passed as an object to theme_username(), so I think its OK to simply set to check_plain($account->name). In other words, this patch calls theme_username() in a fewer places than that earlier patch, but IMHO this is more correct. On the other hand there could still be places in core where the name is displayed without theme_username being called, but if so I don't know about them.

Dave Cohen’s picture

Forgot to mention... the earlier changes to theme_username introduced a preprocess function. Conceivably, third-party modules can add their own preprocess functions (supporting whatever $options they want, for example). So I moved code for check_plain and shortening to the process function so that it will happen after third-party modules make their changes.

P.S. off topic - the preview button is broken on d.o. Can't preview what I'm typing now.

Status: Needs review » Needs work

The last submitted patch failed testing.

Dave Cohen’s picture

Status: Needs work » Needs review
FileSize
15.69 KB

Try again.

Status: Needs review » Needs work

The last submitted patch failed testing.

Dave Cohen’s picture

FileSize
15.7 KB

The tests fail with errors about "Users blog displayed with no entries" and "Breadcrumbs were displayed". Why they fail I haven't a clue. If someone can enlighten me, great.

Dave Cohen’s picture

Status: Needs work » Needs review
FileSize
15.7 KB

Continues to fail.

Status: Needs review » Needs work

The last submitted patch failed testing.

dahacouk’s picture

Good work Dave! Is it worth tracking down who made the specific tests throwing up the errors of "Users blog displayed with no entries" and "Breadcrumbs were displayed"? So, that they can help debug this?

Cheers Daniel

Dave Cohen’s picture

Status: Needs work » Needs review
FileSize
16.04 KB

Man, it's way harder to fix broken test cases than it is to fix broken code. That took some time.

sun’s picture

well, this looks more and more like http://drupal.org/project/user_display... Instead of hard-coding all these options (and not allowing a themer to alter the username display for certain locations - without hacking core), we really should implement display classes as implemented in User Display instead. However, I fear it's too late for that to happen now.

So. Anyway. Review on this approach:

+++ includes/theme.inc	21 Sep 2009 13:54:07 -0000
@@ -1870,6 +1870,13 @@ function theme_more_link($url, $title) {
+    'plain' => FALSE,
+    'link' => TRUE,

These are mutually exclusive.

Just keep 'link' and set that to FALSE everywhere you set 'plain' => TRUE.

+++ includes/theme.inc	21 Sep 2009 13:54:07 -0000
@@ -1923,12 +1924,20 @@ function template_preprocess_username(&$
 function template_process_username(&$variables) {
+  // Make sure name is safe for use in the theme function.
+  $variables['object']->name = check_plain($variables['object']->name);
@@ -1946,8 +1965,11 @@ function template_process_username(&$var
+function theme_username($object, $options) {
+  if ($options['plain']) {
+    $output = $object->name;
+  }
+++ modules/contact/contact.pages.inc	21 Sep 2009 13:54:07 -0000
@@ -54,7 +54,7 @@ function contact_site_form() {
-    '#default_value' => $user->uid ? $user->name : '',
+    '#default_value' => $user->uid ? theme('username', $user, array('plain' => TRUE)) : '',

Wait. $user is passed by reference, $user->name is check_plain()'ed. What in here ensures that we're not altering the global $user object?

+++ includes/theme.inc	21 Sep 2009 13:54:07 -0000
@@ -1923,12 +1924,20 @@ function template_preprocess_username(&$
+  if ($variables['options']['shorten'] && !$variables['options']['plain'] && drupal_strlen($variables['object']->name) > 20) {

Neither 'plain' nor 'link' should trigger any magic. Always shorten when 'shorten' => TRUE (and name > 20, of course).

+++ includes/theme.inc	21 Sep 2009 13:54:07 -0000
@@ -1938,6 +1947,16 @@ function template_process_username(&$var
+ * @param $options
+ *   An associative array of additional options, with the following keys:
+ *     'plain' (default FALSE)
+ *       Whether to force the output as plain text format. Useful for
+ *       centralize username style handling.
+ *     'link' (default TRUE)
+ *       Whether to format name as hyperlink to user profile.
+ *     'shorten' (default TRUE)
+ *       Whether to limit the length of the name, to fit in user tables.
+ *   Modules may introduce additional options by implementing preprocess functions.

Improper PHPDoc formatting and array property list descriptions. See drupal_build_form() for a valid example. Also exceeds 80 chars.

+++ includes/theme.inc	21 Sep 2009 13:54:07 -0000
@@ -1946,8 +1965,11 @@ function template_process_username(&$var
+  else if (isset($object->link_path)) {

"elseif"

+++ modules/blog/blog.module	21 Sep 2009 13:54:07 -0000
@@ -27,7 +27,7 @@ function blog_user_view($account) {
-      '#markup' => l(t('View recent blog entries'), "blog/$account->uid", array('attributes' => array('title' => t("Read !username's latest blog entries.", array('!username' => $account->name))))),
+      '#markup' => l(t('View recent blog entries'), "blog/$account->uid", array('attributes' => array('title' => t("Read !username's latest blog entries.", array('!username' => theme('username', $account, array('link' => FALSE, 'shorten' => FALSE)))))), array('html' => TRUE)),

I don't understand html => TRUE here. l() strips all HTML in attributes anyway.

+++ modules/blog/blog.module	21 Sep 2009 13:54:07 -0000
@@ -60,7 +60,7 @@ function blog_form($node, $form_state) {
-    drupal_set_breadcrumb(array(l(t('Home'), NULL), l(t('Blogs'), 'blog'), l(t("!name's blog", array('!name' => $node->name)), 'blog/' . $node->uid)));
+    drupal_set_breadcrumb(array(l(t('Home'), NULL), l(t('Blogs'), 'blog'), l(t("!name's blog", array('!name' => theme('username', $node, array('shorten' => FALSE, 'link' => FALSE)))), 'blog/' . $node->uid, array('html' => TRUE))));
@@ -72,9 +72,10 @@ function blog_node_view($node, $build_mo
-        'title' => t("!username's blog", array('!username' => $node->name)),
+        'title' => t("!username's blog", array('!username' => theme('username', $node, array('link' => FALSE, 'shorten' => FALSE)))),
+++ modules/blog/blog.pages.inc	21 Sep 2009 13:54:07 -0000
@@ -12,7 +12,7 @@
-  drupal_set_title($title = t("@name's blog", array('@name' => $account->name)), PASS_THROUGH);
+  drupal_set_title($title = t("@name's blog", array('@name' => theme('username', $account, array('plain' => TRUE)))), PASS_THROUGH);
@@ -123,7 +123,7 @@ function blog_feed_user($account) {
-  $channel['title'] = t("!name's blog", array('!name' => $account->name));
+  $channel['title'] = t("!name's blog", array('!name' => theme('username', $account, array('plain' => TRUE))));
+++ modules/blog/blog.test	21 Sep 2009 13:54:07 -0000
@@ -38,7 +38,7 @@ class BlogTestCase extends DrupalWebTest
-    $this->assertTitle(t("@name's blog", array('@name' => $this->big_user->name)) . ' | Drupal', t('Blog title was displayed'));
+    $this->assertTitle(t("@name's blog", array('@name' => theme('username', $this->big_user, array('plain' => TRUE)))) . ' | Drupal', t('Blog title was displayed'));
@@ -50,8 +50,8 @@ class BlogTestCase extends DrupalWebTest
-    $this->assertTitle(t("@name's blog", array('@name' => $this->own_user->name)) . ' | Drupal', t('Blog title was displayed'));
...
+    $this->assertTitle(t("@name's blog", array('@name' => theme('username', $this->own_user, array('plain' => TRUE)))) . ' | Drupal', t('Blog title was displayed'));
@@ -139,7 +139,9 @@ class BlogTestCase extends DrupalWebTest
+    $bc = t('Home ' . $crumb . ' Blogs ' . $crumb . ' !name' . $quote . 's blog', array('!name' => theme('username', $node_user, array('link' => FALSE, 'shorten' => FALSE))));
+    $bc = filter_xss($bc, array());
@@ -180,7 +182,7 @@ class BlogTestCase extends DrupalWebTest
-    $this->assertTitle(t("@name's blog | Drupal", array('@name' => $user->name)), t('View recent blog entries link target was correct'));
+    $this->assertTitle(t("@name's blog | Drupal", array('@name' => theme('username', $user, array('plain' => TRUE)))), t('View recent blog entries link target was correct'));
@@ -191,7 +193,7 @@ class BlogTestCase extends DrupalWebTest
-    $this->assertTitle(t("@name's blog | Drupal", array('@name' => $user->name)), t('User blog node was displayed'));
+    $this->assertTitle(t("@name's blog | Drupal", array('@name' => theme('username', $user, array('plain' => TRUE)))), t('User blog node was displayed'));
@@ -199,6 +201,6 @@ class BlogTestCase extends DrupalWebTest
-    $this->assertTitle(t("@name's blog", array('@name' => $user->name)), t('User blog feed was displayed'));
+    $this->assertTitle(t("@name's blog", array('@name' => theme('username', $user, array('plain' => TRUE)))), t('User blog feed was displayed'));

Would be cool to unify those two t() strings while being there. The rest seems to use !username instead of !name.

I'm on crack. Are you, too?

sun’s picture

+++ modules/blog/blog.pages.inc	21 Sep 2009 13:54:07 -0000
@@ -12,7 +12,7 @@
-  drupal_set_title($title = t("@name's blog", array('@name' => $account->name)), PASS_THROUGH);
+  drupal_set_title($title = t("@name's blog", array('@name' => theme('username', $account, array('plain' => TRUE)))), PASS_THROUGH);
+++ modules/blog/blog.test	21 Sep 2009 13:54:07 -0000
@@ -38,7 +38,7 @@ class BlogTestCase extends DrupalWebTest
-    $this->assertTitle(t("@name's blog", array('@name' => $this->big_user->name)) . ' | Drupal', t('Blog title was displayed'));
+    $this->assertTitle(t("@name's blog", array('@name' => theme('username', $this->big_user, array('plain' => TRUE)))) . ' | Drupal', t('Blog title was displayed'));

Additionally: watch out, this is double-escaped. theme_username() escapes, and @-type placeholders in t() escape, too.

I really must wonder why the tests passed.

I'm on crack. Are you, too?

Dave Cohen’s picture

These are mutually exclusive.

Just keep 'link' and set that to FALSE everywhere you set 'plain' => TRUE.

That is not sufficient because of the earlier changes supporting RDF. They changed theme_username to include a <span> even when not providing a link. So setting link = FALSE is not the same as plain = TRUE.
However if plain = TRUE, it would not make sense to also have link = TRUE. In my patch above setting plain=TRUE overrides both link and shorten, which I think is useful but I'm not particularly attached to. I just don't want to be the ping-pong ball changing this patch one way then back again if concerned parties can't agree.

Wait. $user is passed by reference, $user->name is check_plain()'ed. What in here ensures that we're not altering the global $user object?

It's not the global user object, its the variables array that has been passed to theme preprocess function. This patch is not changing the way it's already done.

Neither 'plain' nor 'link' should trigger any magic. Always shorten when 'shorten' => TRUE (and name > 20, of course).

To me it feels like shorten=FALSE should be the default. But for historical reasons that's not the case. I think passing array('plain'=>TRUE, 'shorten'=>FALSE) is kind of a burden, but I'll make that change if its the difference between getting this accepted or not.

I don't understand html => TRUE here. l() strips all HTML in attributes anyway.

theme_username is returning a <span>. the html => TRUE tells l() not to escape it. Again, I did not change theme_username to output a <span>. That was an earlier change to D7.

Additionally: watch out, this is double-escaped. theme_username() escapes, and @-type placeholders in t() escape, too.

I really must wonder why the tests passed.

I think they passes because usernames in the tests don't use any weird characters. It's possible that theme_username will return chars that need escaping, except when array('plain' => TRUE) is passed as is the case here. Are you suggesting changing '@name' to '!name'???

Thanks for the review.

Dave Cohen’s picture

FileSize
16.65 KB

Updated with changes suggested by sun.

pwolanin’s picture

I still don't think this should be the same theme function as theme_username. Basically to me this patch is a "won't fix". Why do we need to overload the one function with additional options?

andypost’s picture

This issue mostly about unification of username display, there are a some places in core where username used without theme() function.

For example I wanna use email_registration + realname modules so I need ability to override all username output that is not possible without hacking core in d6. dont forget about #464888: Use theme('username') in user profile page titles

Dave Cohen’s picture

FileSize
17.68 KB

Including the changes from #464888: Use theme('username') in user profile page titles, at least the 2 out of 3 diffs that I could still find.

RTBC?

@pwolanin, if you have a better fix please submit a new issue and let webchick et al. decide which is best.

andypost’s picture

Strange function name

@@ -1923,12 +1933,20 @@ function template_preprocess_username(&$
  * @see theme_username().
  */
 function template_process_username(&$variables) {

Suppose this one should be preprocess not process

pwolanin’s picture

I had a patch in #33 illustrating an alternate approach of a simple additional theme function.

Dries’s picture

To me, this seems like a Drupal 8 feature ...

andypost’s picture

Anyway all occurrences of theme_username should be fixed before release.

This patch works fine for me. But I glad to hear sun and NancyDru opinions.

If there's no conclusion about options let's continue with pwolanin approach (add a new theme function)

Dave Cohen’s picture

I spoke with Angie (webchick) at DrupalCon. At the time she considered this a bug and was on-board with this fix. (Otherwise I would not have spent time on it).

True, other changes happened to theme_username in the meantime. Those made this patch more complicated that it otherwise may have been.

Still IMHO anywhere Drupal displays $account->name without it being passed through a theme function is a serious bug that should be addressed in D7. This fix works, and adds flexibility to theme('username', ...) which is A Good Thing. So I remain if favor of it.

pwolanin’s picture

@Dave Cohen - I agree that this is a bug to be fixed. I strongly disagree with overloading them('username') when you want plain text output instead of using a different theme function.

dahacouk’s picture

@Peter Wolanin (pwolanin):

I agree that this is a bug to be fixed.

Great, so I don't need to say that I was at the same discussion with Angie (webchick) at DrupalCon and concur that we thought we had a remit to fix this. Also, that for RDF to work nicely they need a consistent output of username.

I strongly disagree with overloading them('username') when you want plain text output instead of using a different theme function.

1. Please could you qualify "overloading"? Do you mean overload in a technical sense? Is this patch going to slow down D7 massively? Or do you mean overload in a code writing sense? Is it bloated in your opinion?

2. Just to really clarify because that sentence is a little ambiguous... Your not saying that you want to use a different theme function when you want plain text output are you?

3. What's a credible alternative to this patch? Where is it? And has it passed all the tests?

4. If everyone is saying that we need to solve this issue then we need to solve this somehow for D7. So, let's line up the solution candidates and discuss their respective merits. Where are the credible alternatives?

I look forward to it...

Cheers Daniel

mattyoung’s picture

.

pwolanin’s picture

Status: Needs review » Needs work

Yes, I believe this patch will slow drupal down, since theme_username has a preprocess step and various other overhead that could be avoided with a separate theme function. I am saying that we should always use a different theme function when we want plain text output - a fundamentally different output requirement should use a different theme function.

I also think that it's confusing and bad DX to add even more possible outputs to an already somewhat confusing single theme function.

The fact the a (imho) bad patch passes the regression tests does not make it logically correct.

andypost’s picture

Agree with @pwolanin - we should have 2 functions:
1) old theme_username, maybe extended with $options argument - mostly to identify "context" of username output, this brings ability to overload with _preprocess
2) theme_username_plain for page titles and other cases where extended markup is useless - this one should be quick and simple to bring not a big overhead to render by analyzing a lot of conditions.

Anyway both should be overridable to support realname and other contrib

Dave Cohen’s picture

a fundamentally different output requirement should use a different theme function

It could be argued that fundamentally the same output should use the same theme function.

... theme_username, maybe extended with $options argument - mostly to identify "context" of username output ... theme_username_plain for page titles and other cases where extended markup is useless

Let's talk about this "context" for a moment. Let's say my pet peeve is that the blog module spits out "Dries's blog" but I really want it to write "Dries' blog", because let's just say I appreciate proper English. So I contribute a new possessive.module, which implements possessive_username_preprocess() and takes 'possessive' => TRUE as an option. It has some logic to detect 's' at the end of username and do the right thing. Now blog.module could pass 'possessive' => TRUE instead of stupidly appending "'s" to a username.

Is there any consensus on this thread that such a module would be A Good Thing? Seriously, is this something we want Drupal to be capable of or not? I'm not sure what your idea of "context" is, but let's say just for now that we want to support this.

If you'll concede this feature is desirable, assume I want to write a module that sends emails when users update their blogs. My module wants to email, in plain text, a message like the following: "Here are the latest titles from Dries' blog." Since my module is sending a plain text email, this falls into andypost's category of "cases where extended markup is useless," and still its a case that could benefit from possessive.module, because it want's to render "Dries'" instead of "Dries's".

In short, I'm proposing that a username with markup and a username without markup are fundamentally the same thing, and therefore it is reasonable to use the same theme function for both.

Note that I am not proposing 'possessive' => TRUE as a core feature. I just want core to get out of the way of those of us who want to implement it.

Dave Reid’s picture

Um, in that case we should really have format_possessive() and not implement it in theme_username(). That way we can do possessives correctly throughout core.

Dave Cohen’s picture

My point is I want third-party modules to be able to do whatever they think of, whether the function is named format_... or not. I want core out of the way.

But the point of this thread is not new features and I don't mean to derail it. I think we all agree that usernames displayed in ways that can't be overridden is bad. My proposed solution is being shot down. Maybe someone will win consensus with something more elegant.

NancyDru’s picture

Perhaps there is a way to override "format_possessive()" but I don't know of one, and there have been times when I have wanted to override such functions and so had to create ugly code. Frankly, I'd like to see most such functions (format_date and format_plural spring to mind quickly) replaced with theme functions.

theme_xxx, on the other hand lends itself eminently to overriding. The number of users using RealName clearly demonstrates that there is widespread interest in such a feature.

Whether theme_username_plain comes with an overload right now does not imply that a developer won't create one (as a matter of fact, I can all but guarantee it). To argue that there is extra overhead is totally meaningless. Besides, if one wants to argue efficiency, why are we using an interpreted language rather than a compiled one? This is a case where effectiveness is more important than efficiency.

In the possibility mentioned, both theme_username and theme_username_plain would want to offer possessive => TRUE, so we have just introduced redundancy, or another level of function calling (i.e. overhead).

BTW, Dave, not to sidetrack this issue, but that's an excellent idea, considering that non-English may well render a possessive in some way other than appending 's.

I think the only resolution here is going to come from Dries, himself. We need that top level direction here.

dahacouk’s picture

@All:

NancyDru said:

Besides, if one wants to argue efficiency, why are we using an interpreted language rather than a compiled one? This is a case where effectiveness is more important than efficiency.

This, for me, is the crux between this two function vs one function debate. Are we producing a system that is elegant in its usability or just super speedy? Sure, I know there is always a compromise.

And I think it is important to look at what people are doing with their Drupal installation once they download it - and so the point here is to make it easier for them to do what they want (right? aren't we all meant to be getting "out of the way"?) - and hence easier for contrib modules to assist with that.

Where are the RDF implementers? I thought they needed this patch? At the code sprint Stephane (scor) thanked me for raising this issue with Angie (webchick) and promoting a mini code sprint to push it along...

Cheers Daniel

pwolanin’s picture

Issue tags: -RDF

Since the output of the theme_username_plain function is supposed to be free of markup it is not relevant for RDFa, since that wants to add attributes to tags.

sun’s picture

@dahacouk asked me whether the last statement of theme_username_plain() being irrelevant for RDFa is true. To be honest, I asked that myself when I read it, because I think I understood @scor differently, i.e. RDFa attributes could also applied to plain-text usernames (using SPANs), if enabled. We both shut up, because we are no RDFa experts.

So it would be good to have scor approve this.

On a related note, however, the same principle also applies to other resources, such as node titles and taxonomy terms, which as of now are equally output directly.

dahacouk’s picture

sun said:

RDFa attributes could also applied to plain-text usernames (using SPANs), if enabled.

Yes, that's my understanding too. Actually that's my desire. If I turn on the RDFa option for my Drupal site then I want as much information to me made semantic and machine readable as possible. That means whether it's plain text or has markup.

@pwolanin: So, then, the RDFa option should work on as much text as we can turn semantic. If you really must have your plain text be plain text regardless of if the site is RDFa enabled or not then perhaps we need options:
1. Keep totally plain at all times even if RDFa site option is set.
2. Keep plain but allow RDFa tags if RDFa site option is set.

Like I say my preference would be to enable "set site RDFa" to override the plain text rule. Seeing as it will not change the look to the user.

Note: that a human user will not see any difference in a normal browser view between a site that is RDFa enabled or not.

Cheers Daniel

NancyDru’s picture

@sun: Trying not to hijack this thread, and knowing nothing about RDF, I have already had one node title patch going into 7.x, and have run into some problems with it already because of inconsistencies. I, for one, am going to try to improve that consistency of usage and theming (yes, node titles do have some theming already, whether people know it or not) in 8.x. Taxonomy terms are usually output through theme_links, but, yes, they too need to be more consistently used. But both of these are a topic for D8, and probably a group.

andypost’s picture

The main idea to separate logic when theming username and using username for titles and so. As I said before lets proceed with 2 different theme functions, so old one theme_username maybe user for for RDF-related staff with hard logic but new one theme_username_plain should be simple.

Dave Cohen’s picture

The last patch in this thread supports options 'plain' => TRUE to have no markup at all (i.e. for setting a title or plain text email) and 'link' => FALSE to output a <span> but not a <a>. Whether that is an RDF style <span> or not I cannot say. An earlier patch to D7 added that <span> to theme_username.

I think it is a bad idea to call something "plain" while it still has markup in it. Because for example a function called check_plain() would remove that markup.

pwolanin’s picture

@Dave Cohen - in many places in core it is calling check_plain() and I assumed the goal is to use this for cases where the output should be pure, plain text. If we are replace check_plain($user->name) then the output shoudl by default be the same as what it was before imho.

NancyDru’s picture

The use of check_plain($user->name) is the bug we are trying to address. The problem is that now we are getting incorrect output, so why should we try to keep it wrong?

effulgentsia’s picture

Status: Needs work » Needs review
Issue tags: +API clean-up
FileSize
19.65 KB

With the exception of Dries, there appears to be broad support that outputting a plain $account->name with no support for modules/themes to override is a bug, or at least, a serious API deficiency. I'm gonna go out on a limb and add the "API clean-up" tag to this issue, since I do think it qualifies as that, though of course, someone might overrule me on that. For everyone who cares about this being solved for D7, let's try to get to where we have consensus on approach. Then, perhaps, Dries would be willing to reconsider his position from #56. I think there's a chance that he will, if we come up with something that's truly RTBC before 10/15. I think it'll be a much harder argument after 10/15.

Almost everywhere in core drupal, the output of theme() can be assumed:

  1. to be a string
  2. to contain html
  3. to be sanitized (safe for printing)

Unfortunately, there are some exceptions (theme_table_select_header_cell() returns an array, theme_syslog_format() returns non-html, theme_meta_generator_header() returns nothing but has a side effect of setting http headers, and there's a couple others) but I believe these to be bugs, or at the least implementation flaws, and will open issues for them. I would very much like the solution to the needs to this issue to not introduce a new exception to the above rules.

The places where drupal currently uses $account->name directly is assumed to be neither sanitized, nor containing html tags. You don't want html tags being added to a string that will be used for an attribute value. And you don't want check_plain() called on something that will be used for an attribute value, because drupal_attributes() will call check_plain() again which will result in double escaping.

To me, this means that we need to introduce a new function for retrieving a non-html, potentially unsanitized user name, but that this should not be a theme function. Instead, I propose format_username(). Now, I think that other format_* functions always return sanitized text, so I think format_username() should do so by default, but have an argument for overriding that, so that it can be called from places that will implement their own check_plain() (like when being used for an attribute value). This function should include drupal_alter() as part of its implementation, which would allow modules to step in and change what is used as the name. I can't think of a use-case for why a theme should need to override the implementation, since this is free of markup. So, I think drupal_alter() gives us what we need.

Here's a patch that implements my proposed approach. See code and code comments for details. This patch only modifies client code (e.g., blog.module) that was modified as part of patch 53. I did not sweep the rest of drupal core to see if there's other places that should be changed.

Regarding the question of supporting RDFa in places where we don't want a link, yes, it's theoretically desirable. In places where we need the username as part of an attribute value (e.g., title attributes for links used within the blog module), we can't have span tags, so we can't have RDFa. However, in other places, (for example part of the text of a link), it may be desirable to output something that contains a span tag with RDFa, but not a link (since you can't have a link inside another link). Therefore, this patch, adds a $link boolean argument to theme('username'). If you want non-html, call format_username($account). If you want html, call theme('username', $account). If you want that html to not have a link, call theme('username', $account, FALSE). In this patch, I assumed that all places where a bare $account->name was being output, that we don't want html. This makes this patch fully backwards compatible. I recommend that we continue with this, and if and when this lands, if the RDF team would like some of those places changed from format_username() to theme('username'), then they can open a separate issue for that.

Regarding improving possessive logic, I think it's out of scope for this issue and possibly for D7. Note, that those places are routed through t(), so the translation layer can be used to some extent. I don't think we should move possessive logic into format_username() without making sure we do so in a way that still supports translations. For example, the translation layer can output t("!name's blog") as "blog of !name" (or the equivalent in the language of choice). So any standardization of format_possessive() would need to not take away this feature.

pwolanin’s picture

Status: Needs review » Needs work

This looks like a reasonable approach, but I think the function signature is overly complex and looks like it's killing kittens for fun.

I think $options must be dropped totally. t() should not be used in this function - it's not used here: http://api.drupal.org/api/function/template_preprocess_username/7

drupal_set_title() already uses constants defined to say whether or not to apply check_plain() - those should be used instead of a boolean.

Why "$skip_alter" - I cannot think of anywhere else we do that? That should be dropped also.

The drupal_alter should come after the point where it's shortened, and the original unshortened value should be provided as a parameter.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
12.53 KB

Right you are.

I had $options in there for t(), because initially I tried implementing the possessive feature, before realizing it was a can of worms.

I had $skip_alter in there, because calling drupal_alter() has enough overhead, even when no modules implement an alter() hook, that when I updated template_preprocess_username() to use format_username(), it slowed down the invocation of theme('username'), and I thought that might be a problem.

This patch simplifies things a lot. I decided to not have template_preprocess_username() call format_username(). In fact, I took out all changes to theme('username') from this patch. Anything we want to do there can be made into a separate issue. This removes performance issues with theme('username') from being a blocker to this patch. I think it would be nice to have template_preprocess_username() leverage format_username(), but perhaps I can investigate that after seeing whether drupal_alter() can be optimized. I think a $link argument to theme('username') would be nice, but again separate issue, perhaps only to be considered if the RDF team decides that some calls to format_username() should be replaced with theme('username'), but only if overriding link generation is supported. Finally, our only current use-cases of wanting shortened usernames is in theme('username'), not in format_username(), so I left that logic in the former and not the latter.

I hope these simplifications make it easier to push this through and solve the biggest hole, and allow the API improvements discussed above to be debated separately.

effulgentsia’s picture

Even more simplification. I was wrong about format_* needing to return a sanitized string. format_plural() is a straight wrapper to t(), which does not guarantee sanitized output. Given that all our use-cases don't want sanitized output, I just made it clear in the PHPDoc that that's what's returned. If a use-case turns up where sanitized output is wanted, it's straightforward to just call check_plain(format_username($account)).

I also added a PHPDoc for hook_username_alter().

Dave Cohen’s picture

This is great. Huge improvement over D6.

Would it makes sense to have theme_username() call format_username()?

If not, consider adding to the documentation of hook_username_alter that any module wishing to implement that function will probably want to override theme_username as well. You don't want people thinking that implementing just hook_username_alter() is sufficient "to ensure user privacy in situations..."

andypost’s picture

Status: Needs review » Needs work

Very promising approach! Agree with Dave - theme_username should use format_username to alter.
By the way logic is separated format for data layer and theme for presentation!

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
16.34 KB

Definitely makes sense to have template_preprocess_username use format_username(). I was concerned about the performance of doing so, because drupal_alter() is a little slow. But I solved that by changing format_username() to use a faster emulation of drupal_alter(). This patch fully separates logic from presentation. hook_username_alter() is the single place that changes WHAT is used as the name. hook_preprocess_username() can be used to adjust shortening logic or add markup. And, of course, theme_username() can be overridden by the theme to have full control over presentation.

sun’s picture

Status: Needs review » Needs work
+++ includes/common.inc	30 Sep 2009 16:53:00 -0000
@@ -2318,6 +2318,33 @@ function _format_date_callback(array $ma
+function format_username($account) {

This function has to use the "user_" prefix.

It makes no sense to put this into common.inc.

I'm on crack. Are you, too?

effulgentsia’s picture

Status: Needs work » Needs review

@sun: are you sure? theme_username() is registered in common.inc and implemented in theme.inc.

Dave Cohen’s picture

Status: Needs review » Needs work

I've changed my thinking and/or realized something.

I'm implementing a module which will can override theme_username() to display a user's Facebook name, rather than a Drupal user name. There are two ways to accomplish this. The first is via (X)FBML tags, which like <fb:name uid=123456789>. The second is via a call to facebook's servers to learn the user's name, given their facebook uid (not drupal uid). The first option is efficient, I can alter database queries to learn the facebook uid every time an account or node is loaded (because I store this when the user registers), then simply override theme_username() to display the <fb:name ...> tag instead of $object->name. However, in cases where the username cannot use markup, i.e. page titles and plain text email... in these cases I have to learn the username from facebook and render it, requiring the second and extremely inefficient option (because I can't use XFBML in these cases).

The problem with the current patch is that hook_username_alter is being called every time theme_username() is called. This could be many many times on a single page. And my module cannot afford to call out to facebook each of those times, and it can't tell when the call is truly necessary.

I realize it was kind-of my suggestion to begin with, but now I don't want theme_username() to call format_username(). Or, I want hook_username_alter to somehow know whether markup is allowed. I can't think of the best solution right now; will sleep on it. But I do know that the patch as it stands creates a problem for my work.

effulgentsia’s picture

@Dave: Interesting use case. My first impression is that this is a special situation, where you're implementing a slow alter() hook in a context that is normally assumed to be fast, and then to compensate, want to be able to have places that really do need to be fast to be able to circumvent calling something that is otherwise logical to call. A way you can accomplish that, even with patch 83, is to have your module implement hook_theme_registry_alter() to take template_preprocess_username() out of the pipeline, and replace it with your own preprocess function. This isn't super clean, but I think is appropriate in a situation where you need to optimize. If you agree, please set back to "needs review". If you have an alternate proposal, I'd love to read it.

effulgentsia’s picture

Also, if you don't want to take template_preprocess_username() out of the pipeline entirely, since it does a lot of useful stuff you might not want to duplicate, you can implement hook_theme_registry_alter() to add a special function to run before template_preprocess_username(). For example:

function MODULE_theme_registry_alter(&$theme_registry) {
  array_unshift($theme_registry['username']['preprocess functions'], 'MODULE_earlypreprocess_username');
}

function MODULE_earlypreprocess_username(&$variables) {
  // Runs before template_preprocess_username(), so original account object is
  // $variables['object'] rather than $variables['object']->account. We want to
  // add a property to it, so that MODULE_username_alter() knows that this
  // username will get fully themed, but we don't want to modify the passed in
  // account object, since that might have unwanted side effects.
  $variables['object'] = clone($variables['object']);
  $variables['object']->will_be_themed = TRUE;
}

Alternatively, you can implement some kind of caching mechanism of Facebook data in your local db to keep your hook_username_alter() reasonably fast in all situations.

andypost’s picture

Status: Needs work » Needs review

@Dave Cohen you should choose a right cache-strategy for your use-case, no sense about current patch.

@sun format_username() is common use function so why you propose to put it in user.module?

@effulgentsia your implementation is little bit faster then drupal_alter which works by the same way but more readable because of #557542: Cache module_implements()
Suppose benchmarks is a good argument about implementation

NancyDru’s picture

@Dave: In RealName, we use a static array to hold usernames we have already resolved on the page.

@effulgentsia: Yes, template_preprocess_username is in common.inc, but theme_username is in user.module, which I agree is where this should be. It is interesting that the user module is one of the worst culprits for not using theme_username.

andypost’s picture

@NancyDru take a closer look http://api.drupal.org/api/function/theme_username/7 this one lives in theme.inc

UPD link to D7 api

Dave Cohen’s picture

I think the suggestions #87 and #88 will work for me.

Status: Needs review » Needs work

The last submitted patch failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
36.28 KB

Re-roll, plus I found additional places where I think replacement is needed. My reasoning was that it's needed on any html output excluding user account management screens, on emails that get sent excluding ones related to account management, and on user related token replacements. I do not think it's appropriate to call format_username() on calls to watchdog(), so I left those alone. For any prior reviewers, please feel free to double-check where I decided format_username() was needed, and correct me.

@andypost: benchmarks coming once testbot oks this patch.

@sun: please confirm #85, or explain what I'm not seeing.

@pwolanin: please give more feedback or comment that it meets with your approval.

effulgentsia’s picture

Here's some benchmarks using Apache Bench. Fresh install of head, using "Devel generate" to create 300 users, giving anonymous role permission to "Administer users" and "Access user profiles" (ab accesses pages as anonymous), and then benchmarking the /admin/people page, which calls theme('username') 50 times.

HEAD: 164.4ms
Patch 94: 165.0ms
Patch 94 plus replacing format_username()'s foreach block with drupal_alter('username', $name, $account);: 165.9ms

Conclusion
There is some overhead in having template_preprocess_username() call format_username() which in turn calls module_implements(). But in my opinion, small enough to be worth doing. The additional overhead of using drupal_alter() instead of foreach(module_implements(... is, in my opinion, not worth it, which is why patch 94 is the way it is. drupal_alter() is a nice utility function that is mostly used in places where the extra overhead is not a problem. But all it really does is the exact same foreach(module_implements(... block, but with a bunch of cruft to be flexible with respect to how many arguments are passed in. Like I said, a nice utility for replacing 4 lines of code with 1 line of code in most places, but not where performance matters.

effulgentsia’s picture

removing "needs benchmark" tag.

effulgentsia’s picture

Status: Needs review » Needs work
effulgentsia’s picture

Status: Needs work » Needs review
FileSize
36.24 KB

Re-rolled. Just a note, I'm not an expert on where the raw login name ($account->name) needs to be used and where the formatted name (format_username($account)) needs to be used. Someone who is: please review this patch for where the former was converted to the latter, and comment if it's too aggressive (or, on the flip side, if you find a place that isn't converted by this patch and should be). API freeze is only 1 week away, so this patch needs to make it to RTBC status soon, or it will likely get deferred to D8.

effulgentsia’s picture

Title: a user name should always be generated via a theme() function » User's raw login name should not be output directly to the page in most places

Changed title. I'm open to it being changed to something clearer by someone who can come up with a way to phrase it succinctly.

Once again, summary of the issue trying to be solved:

1) It should be possible for a contrib module to implement user login name privacy and/or display a user's real name (determined however the contrib module wishes to determine that) instead of the login name in most places where the "username" is output to the screen.

2) At first glance, this sounds simple: make sure theme('username') is called whenever a username needs to be rendered. However, in most places where that's not being done already, it's because a full html version of the username (with link to profile page and/or rdf attributes) isn't desired (for example, it's being rendered to a plain-text email or an html attribute value like image alt text or link title).

3) Therefore, this patch introduces a function, format_username(), which gives modules a chance to decide how a username should be formatted, while still keeping it in plain text (similar in principle to what format_date() does, except in the case of date, there are well standardized formats, but in the case of username, it's delegated to modules to decide what's wanted).

4) Finally, this patch converts code that currently outputs raw login name in places where a formatted name makes more sense to use the new function. However, see #98: the decision of where this is being done needs another pair of eyes on it.

Status: Needs review » Needs work

The last submitted patch failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
27.42 KB

Re-rolled and removed unnecessary hunks that were just trailing whitespace removers in unrelated code.

effulgentsia’s picture

Given the lack of response to #98, I'm losing confidence that this will make it into D7. Therefore, I decided to go a little more conservative. I removed some of the places I converted $X->name to format_username($X) in #94. Specifically, I removed it from the return of user_search_execute(), because I think this might be a place where login name is needed. And, I removed it from all hook_tokens() implementations (because changing what HEAD currently does with respect to tokens might have implications I'm not in a position to evaluate).

I believe what's left in this patch is completely safe and proper and wouldn't make HEAD worse by landing. It's quite likely additional places exist where $X->name needs to be converted to format_username($X), but I think that makes sense to resolve during the bug-fixing phase between 10/15 and whenever D7 is released. In other words, better for this patch to be conservative rather than aggressive.

Anyway, leaving it up to someone to RTBC. I don't think there's anything more I can do on it without feedback.

NancyDru’s picture

I haven't tested this version yet because I have my D7 messed up. But there are 7,000 users of RealName who think there is NO place to show the login name.

Dave Cohen’s picture

regarding user search... I agree it makes sense to spit out the $account->name directly on this page because that is what the search algorithm looks at. If my site has a user named "X" and my custom module displays the name as "Y", clearly when someone types "X" in the search box they should not be shown the name "Y" in the results. In this case its up to my module to provide a better alternative to the core user search.

I'm travelling on vacation. Otherwise I'd take some time to test and hopefully RTBC this issue. Right now I can't, but it would be a shame to not get this in.

moshe weitzman’s picture

Edit: I meant a new build mode

I have not looked at the patch. It does occur to me thogh that this might be a use case for a new build mode during user view. Users only have 'full' right now but maybe we should add 'name' or something. Just a thought. One could imagine putting an online/offline indicator and a user badge into that build mode.

NancyDru’s picture

@Moshe: you may be right. Right now, RealName can intercept user searches and "fix" the name. Th ebuild mode might make this a bit more efficient.

dahacouk’s picture

How can I help RTBC this issue? I'm not technical enough though. Would be a real shame if this didn't get into D7 after so much good work.

andypost’s picture

FileSize
74.28 KB
336 bytes

@effulgentsia this patch is great, double + to RTBC this before feature-freeze!!!

@moshe weitzman Idea of build modes for user (like node) looks like sun's userDisplayAPI but right now no time to investigate deeper. This hook is quick and minimum what drupal requires to overide page titles at first.

I've tested this patch by writing small module (attached rname.tgz) this one implements a hook_username_alter() which doubles a username before output.

Tested: blog's tittle, username at toolbar, username on profile and picture, contact form username

@dahacouk You can install my module and check that username is doubled at all pages is in format "username-username" take a look at attached screenshot

Status: Needs review » Needs work

The last submitted patch failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
22.1 KB

re-rolled.

andypost’s picture

Contact form changed at #601250: Allow anonymous users to use personal contact forms
some comments about username #15

Status: Needs review » Needs work

The last submitted patch failed testing.

effulgentsia’s picture

Re-rolled.

Re: #111 (actually, the #19 of that issue: http://drupal.org/node/601250#comment-2139064), first of all, nice job on the patch for that issue!. Secondly, this patch does call format_username() on the default value of the "Your name" textfield in contact.pages.inc (both personal and site forms). My rationale is that we established that format_username() must return plain text only, not html, and so I see no reason not to consider the default value of a textfield to not be "display", since it is visible on the page. Please correct me if I'm missing something in that regard.

effulgentsia’s picture

Status: Needs work » Needs review

with the status change, this time.

dahacouk’s picture

Let's get this in before someone else adds another patch somewhere else that breaks it! ;-/

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Tested again and it works, because it's API change it should be in before 15/10 or webchick promise to proceed with filter_xss(theme('username', $account), array()); approach

I found no places that not covered by this patch, so about search and autocomplete widgets they should still use username, suppose this can be tuned in follow-up patches.

moshe weitzman’s picture

Looks solid to me. Nice work.

dahacouk’s picture

Do we just sit tight and wait for a committer? That's only webchick and Dries, right? They've got quite a queue!

effulgentsia’s picture

Yep. That's how it works.

juan_g’s picture

Looking at the title of this issue in the queue (User's raw login name should not be output directly to the page in most places), it might sound a bit limiting/partial/controversial, when in fact it opens more possibilities and respects all options, including the old behavior by default.

A more accurate issue title would be for example:

Let modules decide whether user's raw login name should/shouldn't be output directly to the page in most places

andypost’s picture

FileSize
22.48 KB

Just reroll for current HEAD

dahacouk’s picture

@juan_g: Regarding your issue with the title... I've been looking at the "reviewed & tested by the community" and "fixed" issue queues and they all sound pretty positive. So, I'm tending to agree with you that this issue's title sounds pretty negative and limiting and doesn't state what this issue really does and the advantages it brings.

When Dave and I spoke with webchick in Paris, she described the issue as a bug that needed to be fixed. It was very much that we needed to have consistent output of the username that would enable contrib modules to more easily override the output without producing loads of messy, inconsistent and sometime incompatible code - which is bad the community, right?

So, correct me if I'm wrong...

This is all about making output of the username consistent and clean.

And simplifying the use of theme_username().

And enabling options for contrib modules to more cleanly and consistently override the output of the username.

So...

"Clean up username output and enable contrib modules to easily and consistently override the output"

However, there's a fine line here as I know that webchick is adamant that the modules that actually override username output should definitely not be in core. However, making it easy and consistent for contrib modules to override username is a good thing and will help the community. However, Dries has come down a bit on the negative side of this issue. Not to sure exactly why but it feels like it's making things too fixed - which I can't get my head around - and I've got the feeling that he, like webchick, doesn't like the idea of overriding username output from within core modules. But with contrib modules it must be OK, right? Because they are doing it right now, right? But it's messy and inconsistent. So, all we're trying to do here is clean things up. So, perhaps less is more in this case. And perhaps we could say...

"Clean up username output"

It's short and catchy and does what it says on the tin...

Have a good weekend.

Cheers Daniel

juan_g’s picture

Yes, I think any of those two "Clean up..." titles would be more accurate than the current one. Other things such as a possible, optional and perhaps unique (IMDb style) field for full names -in addition to the current, also unique and usually shorter, login names- would be a matter to study on another issue, probably a D8 issue. However, I think the fix made by the patch in this issue would be excellent for D7 if it's not too late.

dahacouk’s picture

Title: User's raw login name should not be output directly to the page in most places » Clean up username output

As of today Dries is still committing to CVS HEAD. I'm not sure when he (or webchick) will stop. So, it's perhaps not too late. In the hope that webchick or he will notice this issue I'm changing the title to "Clean up username output" in the hope that it'll be more accurate (and positive) to actually what is going on here and talk about the benefits of this issue. Also, hope that webchick notices as we have been acting on her acknowledgement that there were bugs in the system - and we've been correcting them. I hope that effulgentsia and all the other developers that have worked on this issue agree with the sentiment of what I am doing. Fingers crossed!

Cheers Daniel

dahacouk’s picture

Title: Clean up username output » Cleanup username output
sun’s picture

Title: Cleanup username output » User's raw login name should not be output directly

There's no reason to be afraid. The less you derail this issue, the more likely it is that a core maintainer will be able to read and understand the purpose of this patch. Thanks.

+++ includes/common.inc	16 Oct 2009 06:58:38 -0000
@@ -2308,6 +2308,33 @@ function _format_date_callback(array $ma
+function format_username($account) {
+  $name = !empty($account->name) ? $account->name : variable_get('anonymous', t('Anonymous'));
+  // Faster than drupal_alter(), and format_username() gets called a lot.
+  foreach (module_implements('username_alter') as $module) {
+    $function = $module . '_username_alter';
+    $function($name, $account);
+  }
+  return $name;

This can be removed now - drupal_alter() is very fast now.

+++ includes/theme.inc	16 Oct 2009 06:58:38 -0000
@@ -1912,12 +1912,17 @@ function template_preprocess_username(&$
+  $name = $variables['name_unsafe'] = format_username($account);

I don't see why we're invoking format_username() here. This is a preprocess function anyway, so you can override the generated variables already.

That makes no sense to me.

This review is powered by Dreditor.

andypost’s picture

FileSize
22.31 KB

+ $name = $variables['name_unsafe'] = format_username($account);

This helps to override username providing only a one hook, without it contrib need hook and proprocess

Attached patch to sync with HEAD and changing back to drupal_alter()

juan_g’s picture

sun wrote:
>There's no reason to be afraid. The less you derail this issue, the more likely it is that a core maintainer will be able to read and understand the purpose of this patch. Thanks.

Sorry. I think we didn't realize the issue title had changed so many times:

2007-11-14 - centralize username styling in theme_username()
2007-11-14 - theme: centralize username styling in theme_username()
2009-03-02 - theme_username() should be consistently used
2009-09-07 - the username should always be generated via a theme() function
2009-09-07 - a user name should always be generated via a theme() function
2009-10-09 - User's raw login name should not be output directly to the page in most places
2009-10-17 - Clean up username output
2009-10-17 - Cleanup username output
2009-10-17 - User's raw login name should not be output directly

effulgentsia’s picture

Because I know quite a few people are anxiously awaiting this patch landing, I just wanted to point out Dries' comment from http://drupal.org/node/619258 :

Other than changes necessary for the overlay, and a few left-over patches that were ready by the 10/15 deadline, we have now entered the next phase of the code freeze: no more API changes and no additional features.

This one was ready by the 10/15 deadline, so as far as I know, is still in the running. Of course, webchick and Dries make the final call on the merits of the issue.

I'm posting this in an effort to help calm people rather than start a process of re-hashing the reasons for why this patch is important and should be part of D7. My recommendation is to patiently wait until webchick or Dries weighs in with an opinion or questions. Unless, of course, you have new information to share.

My sympathies to those of you for whom this is crucial in making your D7 plans. Dries and webchick are doing their best in keeping things moving. One could easily argue (and many already have) that Drupal has grown too big for just two core committers, and that's a discussion well worth having for the D8 cycle. But not here.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs documentation

One could also argue that two committers would be just fine if every single core developer hadn't left all of their major API changes 'til the last possible bloody second of code freeze and crammed their stuff into the RTBC queue all at once at the end. ;)

So my first gut impression of this is "Why are we outputting something that doesn't run through the theme system?" But then I see mostly we're replacing code like this:

-        'title' => t('Hello <strong>@username</strong>', array('@username' => $user->name)),
+        'title' => t('Hello <strong>@username</strong>', array('@username' => format_username($user))),

...which wasn't going through the theme system to begin with. Reading up into the issue (thanks for the nice summary @ #99), it appears that this is because theme_username() makes a whole lot of assumptions about how it ought to be rendered, and we don't want HTML with spans and links in places where a straight-up string is required (such as anchor title attributes). A theme_username_plain() option was also discussed, I see, but effulgenstia and others rightly pointed out that what's returned from here isn't an HTML string as we have in other standard theme functions, so this would be confusing.

My second gut reaction is that a hook doesn't really make sense here as the alter mechanism, since in 99% of cases it seems like there's only going to be one module that "wins" here (presumably, whichever one is weighted last). But Dave pointed out that his module might wrap the username in FBML, while another module might be doing weird formatting to possessive strings (incidentally, "Dries's" is proper English). We also have been trying to get away from one-off "magic" function names for this kind of thing, so the hook makes sense. (Though if you talk to Larry/Crell he'll say it deserves its own handler).

I got a bit turned around in the discussion in the last few replies, and it sounded like there would be two functions that people needed to override to get the right name showing up everywhere, which would totally suck. But I downloaded the rname module from #108 to try it out, and confirmed that the behaviour looks right (well, wrong ;) but as-designed) on a few spot-checks: the "Hello" text in the upper-right corner, the name of my blog, the link to all of my blog posts, and the hover-title for my blog posts.

This is overall a much better situation than we had in D6 (and I do consider it a bug that you can't have consistent username presentation everywhere), and doesn't go the weird "magical $options" parameter approach of previous patches which Dries didn't like (and with good reason), so I'm comfortable committing this. I have a feeling there will be follow-up work though, particularly around things like token support and e-mails which I didn't get the chance to test, and don't see anything in the replies leading me to believe it's been tested, either.

The only weird thing I found was:

+  $name = $variables['name_unsafe'] = format_username($account);

We don't use "unsafe" anywhere else in variable names, it should be 'raw' instead.

So I fixed that and committed to HEAD. Thanks for the rally around this, and the really nice solution that ensued. May Drupal 7 be an even more kick-ass framework for building social networking apps. :)

Marking needs work for documentation. I imagine modules such as RealNames need information on how to port their former workarounds in the module upgrade guide. We also need to make sure that module developers are format_username() instead of $user->name.

andypost’s picture

Great! Docs should point a difference between theme_username() and format_username(). Suppose, last one should be used mostly for places like page titles and markup without links (button's names, select options).

Dave Cohen’s picture

Status: Needs work » Fixed

W00t!

Webchick, you're drink of choice is on me, next time we meet. You too, Alex B.

Updating to fixed. It is fixed, right?

juan_g’s picture

Status: Fixed » Needs work

Well, she wrote:

> committed to HEAD

but also:

> Marking needs work for documentation. I imagine modules such as RealNames need information on how to port their former workarounds in the module upgrade guide. We also need to make sure that module developers are format_username() instead of $user->name.

Or maybe opening another issue for documentation?

juan_g’s picture

It seems updated documentation is needed at least here:

and possibly here:

See also:

andypost’s picture

It needs a docs so not fixed

webchick’s picture

Yeah, the main thing we need is an item on http://drupal.org/update/modules/6/7 that documents the 'before' and 'after' code so that module developers know what to change. It should also mention the addition of hook_username_alter().

Once this has been posted, feel free to mark this issue back to 'fixed'.

Dave: I enjoy Cherry Coke and V8 juice. ;)

NancyDru’s picture

Angie, Dave, and effulgentsia: I will buy you all a drink. I'm now right down the road from Acquia now, so if you're in the neighborhood, give me a shout.

effulgentsia’s picture

Status: Fixed » Closed (fixed)
Issue tags: -Needs documentation, -API clean-up

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