Trimming of long usernames results in incorrect RDF

Usernames of over 20 characters are truncated by template_preprocess_username() in user.module.

The patch attached checks for this condition and sets the 'content' attribute to the full (sanitized) value as necessary. Additionally, the 'datatype' attribute is removed - if it's present but empty, EasyRDF's RDFa parser ignores the 'content' attribute and uses the text content of the element (that is, the shortened username).

@djevans - could you upload a patch that combines your fix with the failing test from #1? If that passes then we know we're on the right track :)

Also there is a bit of trailing whitespace in the patch, please remove per http://drupal.org/coding-standards#indenting.

Ok, combined patch attached.
It seems a bit awkward to be checking the username length in two places - would it be an idea to set a variable like

$variables['truncated'] = TRUE;

in template_preprocess_username() so it can be checked in rdf_preprocess_username() or elsewhere, instead of duplicating the check?

I think it's a good idea to indicate whether the username has been truncated, otherwise the code breaks when the 20 character limit is changed. I have added the necessary code as proposed above.

Patch on #6 works fine, It provides the correct RDF. Testing also works fine :)

Is that change to the test enough to demonstrate the bug?

We should test both cases (when the truncation happens and when it doesn't).

Attached is an incorporation of #6 plus the additional test cases for a user with a 30-character user, someone right on the cusp (20 characters) and a short username (5 characters).

#10 also works (tested both automatically and manually).
I really don't understand why so much testing is required (3 different cases). But since it is good for Drupal I see no reason not to do so :)

I won't RTBC it just yet, but it is fine by me.

ps: Shouldn't we add to the test a step that displays a node authored by the user? Since those patches also affect these pages too.

This is D8 unpatched:

<span rel="schema:author">

    Submitted by 

    <a class="username" lang="" datatype="" property="schema:name" typeof="schema:Person" about="/drupal/user/2" title="View user profile." href="/drupal/user/2">

        averyveryveryve...

    </a>

     on Fri, 09/27/2013 - 17:36

</span>

And this is D8 patched (with patch #10):

<span rel="schema:author">

    Submitted by 

    <a class="username" lang="" content="averyveryveryveryveryverylooooooooooooooooooongusername" property="schema:name" typeof="schema:Person" about="/drupal/user/2" title="View user profile." href="/drupal/user/2">

        averyveryveryve...

    </a>

     on Fri, 09/27/2013 - 17:36

</span>

I discussed with scor yesterday at the sprint when I contributed it. My general motivation with unit testing--what I've always heard--is that it's good to test the edge cases. So I tested one right on the cusp, in case user.module changes from > 20 to >=20 or reverses logic to < 20 or something like that. Perhaps the three tests at 19,20,21 would be better?

I would feel better, actually, if the trim length were in a define, actually. Then we can just test two cases; one above the USERNAME_TRIM_LENGTH and one below... but I wasn't sure in Drupal-land if that were a good idea or not.

My understanding is that the test DOES look at the nodes that are created by the users, so I'm not following the additional case you're suggesting. But if I can understand it, I'm happy to continue to modify the test. :)

From what I saw on the test (the verbose output, because I still like to test manually), it only displays the user profile page. Not a node posted by the user.

As for the other things you are suggesting, I really don't have something to add :D
Maybe some gurus will come and explain if it is fine to define the trim length.

To clarify, the test definitely creates nodes as the users (articles, promoted to be exact) and then parses those nodes to look for the expected value.

+++ b/core/modules/rdf/lib/Drupal/rdf/Tests/UserAttributesTest.php
@@ -48,59 +48,69 @@ public function setUp() {
+      // Parses the user profile page where the default bundle mapping for user
+      // should be used.
+      $parser = new \EasyRdf_Parser_Rdfa();
+      $graph = new \EasyRdf_Graph();
+      $base_uri = url('<front>', array('absolute' => TRUE));
+      $parser->parse($graph, $this->drupalGet('node/' . $node->id()), 'rdfa', $base_uri);

This might be why @bserem was confused, but the comment here is inaccurate, since we do the parse() on node/NID.

patch with clearer comments.

+++ b/core/modules/rdf/rdf.module
@@ -429,6 +429,15 @@ function rdf_preprocess_username(&$variables) {
+    // If we don't unset the 'datatype' attribute, EasyRdf_Parser_Rdfa will
+    // ignore the 'content' attribute and still use the element's text content
+    // (that is, the shortened username).
+    unset($attributes['datatype']);

We shouldn't need that extra code in there.

Working on this at the Boston code sprint.

Made the changes suggested in #17.

1. +++ b/core/modules/rdf/lib/Drupal/rdf/Tests/UserAttributesTest.php
   @@ -48,59 +48,68 @@ public function setUp() {
   +    // Creates users that should and should not be truncated
   +    // by template_preprocess_username (20 characters)
   +    // one of these users tests right on the cusp (20)
   

   Missing end-of-sentence period.

2. +++ b/core/modules/rdf/lib/Drupal/rdf/Tests/UserAttributesTest.php
   @@ -48,59 +48,68 @@ public function setUp() {
   +      // Parses the node created by the user ¶
   

   Missing end period. (also make sure there is no trailing space at the end).

3. +++ b/core/modules/rdf/rdf.module
   @@ -409,6 +409,11 @@ function rdf_preprocess_username(&$variables) {
   +  // In these cases, store the full username in the 'content' attribute.
   

   we're not really storing the full name here, I think "assert" or "set" is better. What about something like "Assert the full name in the content attribute so it can be extracted in RDFa".

+++ b/core/modules/rdf/rdf.module
@@ -410,7 +410,7 @@ function rdf_preprocess_username(&$variables) {
+  // Assert the full name in the content attribute so it can be extracted in RDFa.

Looks good, expect that line is longer than 80 characters (check it out with your dreditor). All you need to do is to move the "Assert" word to the end of the line above, and the second line should fit in the 80 character limit I think.

The retest worked, and all the comments & typos have been addressed, so this is RTBC. thanks Jamie for your work on this during the weekend!

I'm fixing this now

I've updated the doc block for theme_username

/**
 * Returns HTML for a username, potentially linked to the user's page.
 *
 * @param $variables
 *   An associative array containing:
 *   - account: The user object to format.
 *   - name: The user's name, sanitized.
 *   - truncated: Boolean TRUE if $variables['name'] has been shortened.
 *   - extra: Additional text to append to the user's name, sanitized.
 *   - link_path: The path or URL of the user's profile page, home page, or
 *     other desired page to link to for more information about the user.
 *   - link_options: An array of options to pass to the l() function's $options
 *     parameter if linking the user's name to the user's page.
 *   - attributes: An array of attributes to instantiate the
 *     Drupal\Core\Template\Attribute class if not linking to the user's page.
 *
 * @see template_preprocess_username()
 */

Setting to needs review to test the patch.

The patch in #29 fixes the issue mentioned in #28, unless the wording needs changing.

There are however other keys in the variables array which are not mentioned in the doc block, wasn't sure whether to add them here or not.

I think the missing ones are:

• $variables['profile_access'] (TRUE if user has permission 'access user profiles')
• $variables['uid'] (User ID)
• $variables['homepage'] (which is a duplicate of $variables['link_path'])
• $variables['name_raw'] (which you might want to sanitize and/or shorten yourself in a theme override)

Yeah, if we are filling in undocumented parameters here at the tail end of the patch - these other unexplained ones should probably be added to the docblock also, it looks. Could be another issue if necessary.

@sphism reroll looks good. could you create a follow up issue to look into the other missing variables? I would hate to block this issue on unrelated fixes (unrelated as far as the title goes).

Good call.
Roll this needed fix now, thanks.

Add another trivial fix issue for the remaining missing docs, which are equally valid, but separate.

+++ b/core/modules/user/user.module
@@ -653,6 +657,7 @@ function template_preprocess_username(&$variables) {
+ *   - truncated: Boolean TRUE if $variables['name'] has been shortened.

Maybe this text would be better: A boolean indicating whether $variables['name'] has been shortened.

Settled for:

* - truncated: A boolean indicating if $variables['name'] has been shortened.

because it's 80 chars

I like to use 'if' in the description for booleans because it indicates what the true and false values equate to

Works for me, thanks @sphism! I've also created #2199829: Missing documentation for some variables in template_preprocess_username() to fix the other undocumented variables you found in #30. back to RTBC.

Bravo!
Good to see work by @sphism started at the DrupalSouth code sprint getting in here.
Thanks all!