Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
If a username is over 20 characters than the RDF on a node page for the author is wrong. Patch attached that changes the Drupal\rdf\Tests\UserAttributesTest
to use longer usernames to show we have an issue.
Comment | File | Size | Author |
---|---|---|---|
#35 | 1992954-35.patch | 7.41 KB | sphism |
#29 | 1992954-29.patch | 7.41 KB | sphism |
#24 | interdiff-22-24.txt | 785 bytes | lokapujya |
#24 | 1992954-24.patch | 6.92 KB | lokapujya |
#22 | interdiff-19-22.txt | 1.73 KB | lokapujya |
Comments
Comment #1
Gaelan CreditAttribution: Gaelan commentedComment #3
djevans CreditAttribution: djevans commentedUsernames 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).
Comment #4
star-szr@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.
Comment #5
djevans CreditAttribution: djevans commentedOk, 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?
Comment #6
Stefan Freudenberg CreditAttribution: Stefan Freudenberg commentedI 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.
Comment #7
bserem CreditAttribution: bserem commentedPatch on #6 works fine, It provides the correct RDF. Testing also works fine :)
Comment #8
Dries CreditAttribution: Dries commentedIs that change to the test enough to demonstrate the bug?
Comment #9
scor CreditAttribution: scor commentedWe should test both cases (when the truncation happens and when it doesn't).
Comment #10
chrisfromredfinAttached 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).
Comment #11
bserem CreditAttribution: bserem commented#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:
And this is D8 patched (with patch #10):
Comment #12
chrisfromredfinI 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. :)
Comment #13
bserem CreditAttribution: bserem commentedFrom 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.
Comment #14
chrisfromredfinTo 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.
Comment #15
scor CreditAttribution: scor commentedThis might be why @bserem was confused, but the comment here is inaccurate, since we do the parse() on node/NID.
Comment #16
chrisfromredfinpatch with clearer comments.
Comment #17
scor CreditAttribution: scor commentedWe shouldn't need that extra code in there.
Comment #18
lokapujyaWorking on this at the Boston code sprint.
Comment #19
lokapujyaMade the changes suggested in #17.
Comment #20
lokapujyaComment #21
scor CreditAttribution: scor commentedMissing end-of-sentence period.
Missing end period. (also make sure there is no trailing space at the end).
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".
Comment #22
lokapujyaComment #23
scor CreditAttribution: scor commentedLooks 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.
Comment #24
lokapujyaComment #26
scor CreditAttribution: scor commentedThe retest worked, and all the comments & typos have been addressed, so this is RTBC. thanks Jamie for your work on this during the weekend!
Comment #27
alexpottLooks good. We just need to update this documentation for the new variable:
Comment #28
sphism CreditAttribution: sphism commentedI'm fixing this now
Comment #29
sphism CreditAttribution: sphism commentedI've updated the doc block for theme_username
Comment #30
sphism CreditAttribution: sphism commentedSetting 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:
Comment #31
dman CreditAttribution: dman commentedYeah, 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.
Comment #32
scor CreditAttribution: scor commented@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).
Comment #33
dman CreditAttribution: dman commentedGood call.
Roll this needed fix now, thanks.
Add another trivial fix issue for the remaining missing docs, which are equally valid, but separate.
Comment #34
scor CreditAttribution: scor commentedMaybe this text would be better: A boolean indicating whether $variables['name'] has been shortened.
Comment #35
sphism CreditAttribution: sphism commentedSettled 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
Comment #36
scor CreditAttribution: scor commentedWorks 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.
Comment #37
alexpottCommitted 3001e93 and pushed to 8.x. Thanks!
Minor code comment fix on commit... We're not asserting here :)
Comment #38
dman CreditAttribution: dman commentedBravo!
Good to see work by @sphism started at the DrupalSouth code sprint getting in here.
Thanks all!