Entity query in UniqueFieldValueValidator must not use access checking

we're likely to need some sort of integrity check / hook_requirements message here to warn that 'field x has integrity issue, required unique values, but has duplicates for {} in entities {}'

There was talk in an update critical about adding a data-integrity checker to core. I think the example was a comment with a missing comment-type field or similar that was causing updates to fail. I guess this would fit in that.

But you're right, too slow for hook_requirements

For critical upgrade path issues we've added those to hook_requirements('update') - but those would only run for schemas below a certain version, not every time you run updates.

#3056543: taxonomy_post_update_make_taxonomy_term_revisionable() and the menu link content equivalent fail when entities have no default translation and similar issues are where this sort of thing came up.

I could see putting this in hook_requirements('update'), showing a warning (not an error), and setting a state key so it doesn't run again, but to me feels like it could be a spin-off issue - existing sites will already have the problem, whereas the patch here fixes it for sites that don't have it yet too.

but to me feels like it could be a spin-off issue - existing sites will already have the problem, whereas the patch here fixes it for sites that don't have it yet too.

Then does it really make sense to add code only to warn existing sites? What about an update that will search for such cases and just write a log message?

This is getting fixed in #3202125: EntityQuery accessCheck: unique value validation should not be access sensitive. Let's repurpose this issue to be a follow-up discussing how to warn existing sites that suffered from the bug.

We decided in #3202125: EntityQuery accessCheck: unique value validation should not be access sensitive to make the bug explicit there in order to unblock that issue's parent, but not to actually fix it. And @catch asked for the fix - this issue - to be critical (as it's a data integrity issue).

I added the Portland2022 tag. I also added the Needs issue summary update to maybe bring in some of the info in the issue linked in comment #10 (if necessary).

Here's a patch that brings back the bugfix + test coverage from #3202125: EntityQuery accessCheck: unique value validation should not be access sensitive.

@Berdir:

Can we do that? By fixing the bug, we might break the ability to save these then on existing sites, and depending on how it is used, it might or might not be easy to fix it. Unique-ness might be validated on a field that can't be changed afterwards?

This feels like the same as any other validation tightening, we had it with email validator too, and I don't think it should stop us fixing the bug - although we should do so only a minor release for this reason.

@catch what's your position on warning existing sites as discussed #2 - #9 ?

@jonathanshaw as discussed it's going to be too expensive for hook_requirements(), and we don't really have anywhere else to put it, so I'm not sure it's worth the effort for what is quite a specific situation. It's not the update that will fail, but only validation in cases that should have failed validation anyway.

How about doing an iterative approach, like...

Iteration 1:
1. On newly created instances that are not installed from config, use a different validator behind UniqueFieldConstraint than on the others
2. Create a new _real_ \Drupal\Core\Validation\Plugin\Validation\Constraint\UniqueFieldValidator
3. Mark \Drupal\Core\Validation\Plugin\Validation\Constraint\UniqueFieldValueValidator as deprecated and remove it in Drupal 11

Iteration 2 (Drupal 11)
1. Provide a tool (script?) for analyzing the database before the Drupal 11 upgrade for non-unique values.
2. Suggest ways to fix none unique values before Drupal 11 upgrade. At worst suggest, suggest using the original "unique-ish" validator from Drupal 10 as custom code on a project.

Per #17 we have a release manager saying the fix in #17 here is good as is. I agree that it's better to fix this critical issue now than let it persist forever because a better but slightly complex solution is possible.

The patch is good, so RTBC.

+++ b/core/tests/Drupal/KernelTests/Core/Validation/UniqueFieldConstraintTest.php
@@ -112,4 +113,38 @@ public function providerTestEntityWithStringIdWithViolation() {
+      '@field_name' => 'name',

This needs to be 'Name' to fix the test failure.

Thanks @rpayanm.

Added change record, updated IS.

RTBC for reasons gived in #22.

Just confirmed that the MR opened in #26 fixes #24, thanks @rpayanm.

Hiding the failing patch.

Making a few changes to the release note:

1. Linking the change record.
2. Adding a sentence that explains how to fix "impossible to edit" entities, because otherwise the release note is terrifying. There was already a sentence about this in the CR that we can repurpose.
3. Adding a sentence before that about why we are making this change, because if you don't understand the data integrity aspect and all you know is that it might make some entities impossible to edit, that sounds like a really bad idea.

Adding credit for the helpful reviews and discussion on the issue. I read all of them to understand the implications of this change, why we weren't providing some sort of update hook, etc. Even #21, which turned out not to be the direction we took, presented a good alternate course of action to compare it to.

I read over #3200985: [meta] Fix undesirable access checking on entity query usages and #3202125: EntityQuery accessCheck: unique value validation should not be access sensitive as well as this issue, and agree with going forward with this solution.

There are a couple small fixes I think we should make for the test. I left suggestions on the MR.

Thanks Rolando for the fix, and @xjm for the careful review. The changes to the release note seems great.

Thanks everyone! I had to fix a couple of small whitespace errors (not sure why it didn't get retested when the suggestions were applied.) Other than that it was fine, though.

On account of the potential editing bug for users, we'll keep this bugfix limited to 10.1.x only. I considered backporting it because of the data integrity aspects, but the fact is that it's been broken for a long time whereas the corrupt entities not being editable would be a new problem, so let's keep this minor-only as a disruptive bugfix.

Tagged for the 10.1.0 release notes, and published the change record. Thanks!

This seems to have broken 10.1.x on both PHPStan and PHPCS - can’t understand how checks failed.

Crosspost.