Unhandled exception when trying to register a duplicate username with different case

There is no evident data corruption or other signifiers of critical priority so this is probably major. I am adjusting that.

The critical priority guideline means most or all of the site would be unusable (not the case) and have no workaround (another username is a workaround). Also, #2502021: Unhandled exception when trying to register a duplicate user was major priority and is essentially the same.

LOL, I just noticed I was the reporter of #2502021: Unhandled exception when trying to register a duplicate user and raised it from normal to major.

Database string type fields are usually case-insensitive for lookups, which is why.

I believe this is heavily related to the changes introduced by https://www.drupal.org/node/3372085. It seems like the UniqueFieldValueValidator is no longer case-insensitive.

Furthermore, this also introduces another problem: You can now create multiple user accounts with the same e-mail address if you use a different case since the validator does no longer detect that there is an account with the same e-mail address, for example you can create one account with user@example.com and one with User@example.com.

What may need to happen is that this condition get updated to build out a 'LIKE' condition for each item rather than simply an 'IN' condition.

Building a 'LIKE' condition in Drupal

Note that you can't combine 'LIKE' and 'IN', but a loop should make short work of it.

The tests added in #2478663: UniqueFieldValueValidator works only with single value fields should be easy to expand to check for case insensitivity.

ETA: The PHP logic will probably also need to be updated to account for the 'LIKE' condition, e.g. at $duplicate_values = array_intersect($item_values, $other_entity_values); -- I wouldn't expect array_intersect() to be case-insensitive.

@caesius I did some debugging on the code and, actually, it seems like the condition you mentioned already produces correct results. Only this statement seems to get rid of duplicates. For example, when already having an account "example" and then trying to create an account "Example", $other_entity_values contains "example", which is what we want, right? Only #L85 then gets rid of "example" and thus, the code ends up in the conclusion that there are no duplicate values.

A really straightforward fix that worked for me was converting all values to lowercase before making the intersection

$duplicate_values = array_intersect(array_map('strtolower', $item_values), array_map('strtolower', $other_entity_values));

Not sure if this might have any unintended side effects, though.

EDIT: Is it possible that the statement mentioned by @caesius behaves differently on different entities? As I stated before, just adjusting the line above was sufficient for the user registration, but in the test cases, it seems like that's not sufficient since $other_entity_values is not filled correctly, as also stated by @caesius.

I added a test case that outlines the issue. This is my first ever test contribution (or Core contribution in general), so I hope everything is fine.

I've also added a potential fix. As stated before, not sure about potential side effects though.

Left a comment on the MR, we can't assume all unique field constraints are case-insensitive.

#3419816: Add support for case sensitive validation to UniqueFieldValuesConstraint has a different approach.

I think we should combine the two issues.

Are there other instances where two values may be considered the same by a database query but not in a PHP function like array_intersect?

#2068655: Entity fields do not support case sensitive queries leads me to believe that entity queries/values are case sensitive at the DB layer but I'm happy to be wrong about that

https://git.drupalcode.org/project/drupal/-/blob/11.x/core/tests/Drupal/... seems it depends on the field

I was actually asking about any theoretical "similarities" that can occur in the data comparisons besides case sensitivity. For example, 0 == FALSE (a stretch, I know) or encoding issues.

It's probably fine to only check for case sensitivity for emails and usernames, and it's also probably unlikely that there's anything else to consider here, but still something that should be pondered for a minute before moving on IMO.

Yes, agree @caesius

I don't think we can assume all instances of this constraint want a case insensitive comparison.

In general, I agree with this statement.

However, there are still some things not clear to me: If I understand the code of the UniqueFieldValueValidator and the changes made correctly, wasn't the behavior exactly the same in the past since the case-sensitivity was solely depend on the DB layer (since there were no additional PHP checks)? Do the changes I suggested really change this behavior since if the DB layer is case-sensitive, the code I suggested doesn't change that because the PHP code doesn't even see the potential case-insensitive duplicate because it's not even returned by the DB layer as a potential candidate (=> the results are still case-sensitive)? On the other hand, if the DB layer is case-insensitive, we now preserve this behavior and don't do an additional check on the PHP-side to filter out case-insensitive duplicates.

In contrast, in the code changes you suggested in https://www.drupal.org/project/drupal/issues/3419816, I believe the behavior is now quite confusing:
- If shouldIgnoreCase == FALSE, case-sensitivity is enforced, so this is fine.
- However, if shouldIgnoreCase == TRUE, case-insensitivity is not enforced if the DB layer performs a case-sensitive comparison, right? Therefore, even if shouldIgnoreCase == TRUE, it can happen that the uniqueness is still checked in a case-sensitive manner.

This behavior doesn't align with the expectations I would have towards a shouldIgnoreCase toggle. There might be an oversight on my end, so obviously the above only applies if my understandings are correct.

I able to reproduce this issue on Drupal v10.2.1.

Appears to have open thread, also MR target branch needs to be updated

I have to agree with @nico.b. The MR gets the UniqueFieldValueValidator back to where it was before the breaking change.

Given that the breaking change was inadvertent (as it was not documented and it broke major core functionality), there's no reason to assume that any core and contrib modules that use the UniqueField constraint now expect there to be case sensitivity when comparing values. So there's no strong reason to separate out a properly working constraint only for username and email while leaving any other uses potentially broken.

If there's a need for case sensitivity in UniqueField, that should be handled as its own constraint & validator (CaseSensitiveUniqueField?) that contributors can use if needed.

For context, I have a client site that relies on dozens of user self-registrations per day. With 50K users in the system, we were seeing a lot of username clashes showing up as SQL integrity violations in the logs. This means lots of potential users were getting WSODs without explanation. We've deployed the MR to production due to urgently needing a fix, and it has been working without problems so far.

Thanks for the confirmation @aasarava. We've also already applied the patch to our production system due to the same reasons. For large pages with a high number of user registrations per day, this issue is really critical.

@smustgrave I've updated the MR target branch. For the open thread, there's not much I can do except for waiting for a response. I've also added a response directly in the MR again (not only here).

Does changing the target branch to 11.x mean there's no plan to try to get this included as a bug fix in a 10.2.5 or 6 release?

@nico.b Will let @larowlan respond to the thread

Yes, we'd want this in a 10.2 bugfix.
Replied to the comment, sorry this one dropped off my radar. Left a couple of minor nitpicks on the MR.

Thanks @larowlan!

Thanks a lot, @smustgrave and @larowlan! The suggestions in the MR seem perfectly valid and I've applied them.

Pipeline runs are still successful (and test-only changes still fail) and all threads are resolved, so I hope the MR is good to go now :)

@aasarava: Based on what I've also only learned just a few days ago (from here), patches usually go to the main dev branch (currently 11.x). The patch is then cherry-picked back to stable branches, so e.g. 10.2. So as @larowlan also said, the change of the target is rather a "technical detail"/"process adherence"-thing instead of a decision to not bring it to 10.2.

Feedback appears to be addressed

Thanks!

Committed to 11.x and backported to 10.2.x and 10.3.x

Thanks folks

I'll repurpose the other issue to add a case-insenstive option (which as you rightly point out, we never supported before)

Just found this. Thanks for the fix!
How should accounts that are created programatically handle this sort when there is an existing username? Like is there a way to put an _1 etc after the username?

It seems the fix that was merged caused an unexpected side-effect, referencing the new issue in case anybody from the old crew would like to jump on the new one as well: #3507777: User names uniqueness is no longer accent-insensitive.