Truncate username if it is greater than 60 characters

+++ src/OpenIDConnect.php
@@ -538,14 +538,16 @@
+      $name = mb_substr($original . '_' . $i, 0, UserInterface::USERNAME_MAX_LENGTH - 2);

Seems like a bug here. You need to shorten to max and 2/3 chars less and then append the _i to the username. Instead, you are effectively removing the _i suffix for the case where name length is max.

Also, don't just do "- 2". Instead, do -(number of digits + 1).

I think the bug is still there.. Imagine there's 1000 'o' in the example below

If original is "really_looooong_string", you're then making it "really_looooong_string_1", and truncating it to be "really_looooo". You first need to truncate it to max - strlen - 1, and only then can you add the suffix.

Also, can you move $original = $name to the line before the for cycle. It's functionally the same, but I don't really think it is readable to instanciate multiple variables in the for like that.

It's not only length, but also other characters.
Could we port this from 7.x and also check whether it's still the same in D9/10?
#2872912: Username too long during creation

After suggestions created a patch please review

I've ported the Drupal 7 version of this as suggested in #9. We should probably add a proper test coverage for this tho.

Wouldn't it be safer to refuse to log the user in rather than manipulate the username like this?

Wouldn't it be safer to refuse to log the user in rather than manipulate the username like this?

We did that. But I think it's a decision that has to be made individually for each project/website. This module must prevent Drupal errors, but should add a hook to change the behavior.

Patch from #14 looks ok for me. But it should add a hook to change the behavior.

Tried to take a step back here as it felt like we tried to invent something that might already be invented, and looked at what core, example: class UserNameValidator and its use of https://symfony.com/doc/current/components/validator.html - Would what be more standardized way to going. I think the migration modules use it as it generate user names during a user migration from example uknown systems that is not Drupal.

But it should add a hook to change the behavior.

What behaviour?

The current implementation attempts to use user's preferred_name, so you can already alter the username by modifying the $userinfo['preferred_username'] in hook_openid_connect_userinfo_alter.

Would what be more standardized way to going. I think the migration modules use it as it generate user names during a user migration from example uknown systems that is not Drupal

The UserNameValidator only validates the username, but doesn't allow us to generate a unique name.

I couldn't find any username generation logic or migration examples with unique usernames besides this one: https://git.drupalcode.org/project/drupal/-/blob/main/core/modules/user/..., used in https://git.drupalcode.org/project/drupal/-/blob/main/core/modules/user/..., which isn't very helpful for this case.

What behaviour?

The current implementation attempts to use user's preferred_name, so you can already alter the username by modifying the $userinfo['preferred_username'] in hook_openid_connect_userinfo_alter.

I meant that the username can be changed from ODIC to Drupal.
With hook_openid_connect_userinfo_alter, that's already done.

The entity validation that was introduced in alpha7 is now handling the username length. I added a kernel test in MR 195 validating that the expected exception is thrown when a username is too long.

@pfrilling:

I guess the original report is outdated as of 3.0.0-alpha7 (as you said in Comment #23). There is some discussion on this issue about what to do when the username (or email) does not validate, but the current behavior is to show an error message, log an error with more detail, and not log in the user.

I agree with that decision (also suggested in Comment #15).

The current MR does not change the behavior, but it adds a Kernel test for the validation added in 3.0.0-alpha7. I am updating the issue summary with a "Proposed resolution" section.

Instead of 6 separate test methods, I would prefer

1. One test for valid credentials, with a data provider
2. One test for invalid credentials, with a data provider
3. One test for duplicate credentials.

If you can think of a way to combine (2) and (3), then that would be even better.

If you do not want to rewrite the tests, then I will not insist.

It looks as though there is a merge conflict in .cspell-project-words.txt, so NW for that. If you always add new words to the end of the file, then you pretty much guarantee that there will be add/add conflicts. If you keep the file sorted, then you have a chance of avoiding that. But an even better strategy is to avoid cspell exceptions: I think you can keep cspell happy by using first_user or firstUser instead of firstuser, for example.

Thanks @benjifisher. I updated the test to utilize data providers instead of the original individual methods.

@pfrilling:

Thanks for the updates. LGTM

I rebased and added to the merge train.