Problem/Motivation

realname_update() overwrites the $account->name property ($account->setUsername($realname)) for the remainder of the page request. This results in a different values for $account->getAccountName() before and after realname_user_update()

Proposed resolution

Remove the $account->setUsername($realname) from realname_update()

Remaining tasks

  1. Write a patch
  2. Review
  3. Commit

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#18 realname_remove_account_name_overwrite-2951535-18.patch2.36 KBidebr
#18 realname_remove_account_name_overwrite-2951535-18-test-only.patch863 bytesidebr
#17 realname_remove_account_name_overwrite-2951535-17.patch64 bytesidebr
#17 realname_remove_account_name_overwrite-2951535-17-test-only.patch863 bytesidebr
#15 realname_remove_account_name_overwrite-2951535-15.patch1.88 KBidebr
#15 interdiff-10-15.txt2.71 KBidebr
#10 realname_remove_account_name_overwrite-2951535-10.patch3.08 KBidebr
#10 interdiff-2-10.txt1.87 KBidebr
#2 realname_remove_account_name_overwrite-2951535-2.patch1003 bytesidebr

Comments

idebr created an issue. See original summary.

idebr’s picture

idebr commented
Status: Active » Needs review
StatusFileSize
new realname_remove_account_name_overwrite-2951535-2.patch1003 bytes

Attached patch remove the overwrite from realname_update().

Status: Needs review » Needs work

The last submitted patch, 2: realname_remove_account_name_overwrite-2951535-2.patch, failed testing. View results

vegantriathlete’s picture

vegantriathlete
he/him/his
Primary language English
Location Lakewood, CO
commented

Further arguments against updating the name column in the User entity:

  • What happens if you decide to uninstall the module? The database has already been updated and the module has no way to restore it.
  • What happens if you are using single sign on that expects the name to match against an attribute? We are encountering this issue right now when we implement hook_simplesamlphp_auth_user_attributes() so that we can populate a field in the User entity based on an attribute that is being passed in my SimpleSAML. When the entity is updated with our information it also gets the updated name value. Now when the user signs in the next time with SSO there is no match against the existing account. [Full disclosure: Real name is exhibiting a strange behavior that it has updated the name value upon our function being called for some users but has not for other users.]
vegantriathlete’s picture

vegantriathlete
he/him/his
Primary language English
Location Lakewood, CO
commented
Related issues: +#2849590: Real Name breaks LDAP authentication
vegantriathlete’s picture

vegantriathlete
he/him/his
Primary language English
Location Lakewood, CO
commented

It looks like D7 used its own table and added a separate field to the User entity.

vegantriathlete’s picture

vegantriathlete
he/him/his
Primary language English
Location Lakewood, CO
commented

This does look like an almost 1:1 port directly from D7. It still uses the .install and hook_schema, it still adds the extra field to the user entity, and it still uses views.inc to expose itself to Views. Additionally it performs almost all the same logic in the .module file. So, I would agree that it is needlessly including the $account->setUsername($realname); inside of the realname_update function.

vegantriathlete’s picture

vegantriathlete
he/him/his
Primary language English
Location Lakewood, CO
commented

Naturally this patch is failing the testRealnameUsernameAlter() test because that is looking specifically to see that the name column has been overwritten, which is exactly what this patch is trying to prevent.

vegantriathlete’s picture

vegantriathlete
he/him/his
Primary language English
Location Lakewood, CO
commented

simplesamlphp_auth explicitly uses getAccountName so that it can avoid any issues with getDisplayName. So, to beat this point, this module should not be updating the name column. It should only be using hook_user_format_name_alter.

idebr’s picture

idebr commented
StatusFileSize
new interdiff-2-10.txt1.87 KB
new realname_remove_account_name_overwrite-2951535-10.patch3.08 KB

@vegantriathlete The override for the username is not actually saved to the database, the override is only active until the end of the request.

I have added a title_callback for the user canonical page so it renders the displayName instead of the accountName.

idebr’s picture

idebr commented
Status: Needs work » Needs review
vegantriathlete’s picture

vegantriathlete
he/him/his
Primary language English
Location Lakewood, CO
commented

@idebr: But we DO encounter a situation in which it ends up getting written to the database. The details aren't important. The point is that the module shouldn't be doing the setUsername. Thanks for your work on this patch!

hass’s picture

hass commented

I have added a title_callback for the user canonical page so it renders the displayName instead of the accountName.

That is not needed. There is a case to fix all the bugs in core, see #2629286: Use getDisplayName() for user names consistently. You could help moving this case to a final patch!

hass’s picture

hass commented
Status: Needs review » Needs work

What a waste of time to write this patch... Have you seen #2881939: Roadmap D8 final ?

idebr’s picture

idebr commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-10-15.txt2.71 KB
new realname_remove_account_name_overwrite-2951535-15.patch1.88 KB

That is not needed. There is a case to fix all the bugs in core, see #2629286: Use getDisplayName() for user names consistently. You could help moving this case to a final patch!

I had a look at the related issue in Core. It only changes the displayed name but not the data model of the user object. This means this specific code in Realname has to be updated regardless.

What a waste of time to write this patch... Have you seen #2881939: Roadmap D8 final

I assume you are referring to the related Core issue that changes the display name of the user canonical page? The RealnameBasicTest currently asserts the account display name, but not actually change any logic regarding this page title. If this is to be fixed in Drupal core, then let's remove the assertion from Realname so this issue is not dependent on the fix in the related core issue.

hass’s picture

hass commented

No idea what you are changing there and why.

idebr’s picture

idebr commented
StatusFileSize
new realname_remove_account_name_overwrite-2951535-17-test-only.patch863 bytes
new realname_remove_account_name_overwrite-2951535-17.patch64 bytes

My apologies if the issue summary is unclear.

Assume the following account:
- Username: barack_obama
- First name (field_first_name): Barack
- Last name (field_last_name): Obama

and a Realname pattern: [user:field_first_name] [user:field_lastname]

The realname codes causes the following output:

print $account->getAccountName();
// Outputs: "barack_obama"
$account->save();
print $account->getAccountName();
// Outputs: "Barack Obama"

Since the Realname module is only supposed to change the $account->getDisplayName(), the expected output would be the same string "barack_obama" before and after saving.

I have added a test case to \Drupal\realname\Tests\RealnameBasicTest::testRealnameUsernameAlter() to showcase the failing behavior.

idebr’s picture

idebr commented
StatusFileSize
new realname_remove_account_name_overwrite-2951535-18-test-only.patch863 bytes
new realname_remove_account_name_overwrite-2951535-18.patch2.36 KB

realname_remove_account_name_overwrite-2951535-17.patch is formatted incorrectly. This patch is the correct one.

The last submitted patch, 17: realname_remove_account_name_overwrite-2951535-17.patch, failed testing. View results

The last submitted patch, 17: realname_remove_account_name_overwrite-2951535-17-test-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 18: realname_remove_account_name_overwrite-2951535-18-test-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

hass’s picture

hass commented

Oh... now I got it. That is true and the currently committed code is not correct than.

  • hass committed ccc950a on 8.x-1.x authored by idebr 
    Issue #2951535 by idebr: Real Name needlessly overwrites $account->name...
hass’s picture

hass commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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