Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
UniqueFieldValueValidator
assumes that the host entity is always defined with integer IDs. During the validation, it adds this condition to the entity query:
->condition($id_key, (int) $items->getEntity()->id(), '<>')
Note the entity ID typecasting.
Proposed resolution
Make it work also for content entities with string IDs.
Remaining tasks
- Review
User interface changes
None.
API changes
None.
Data model changes
None.
Release notes snippet
N/A
Comment | File | Size | Author |
---|---|---|---|
#16 | 3027745-16.patch | 6.16 KB | claudiu.cristea |
#9 | 3027745-9.test-only.patch | 3.54 KB | claudiu.cristea |
Comments
Comment #2
claudiu.cristeaAdded #2478663: UniqueFieldValueValidator works only with single value fields and #2973455: Unique field constraint does not work with any field main property name that is not "value" as related issues.
Comment #3
idimopoulos CreditAttribution: idimopoulos at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedI am attaching a patch for this one. The patch includes a test, however, the tests passes without the patch as well as by converting the id value to 0 still applies as a query.
Comment #5
wengerkAdded #2999226: Unique field constraint does not work with link module fields as related issue.
Comment #6
wengerkThanks to @idimopoulos for addressing this issue with a patch and tests !
I still think there is room for improvement.
UniqueFieldValueValidationTest
by fixing the$fieldname
&$value
instead of generating a random.@group Validation
instead of@field
core/tests/Drupal/KernelTests/Core/TypedData
instead ofcore/tests/Drupal/KernelTests/Core/Validation
I don't know which is betterUniqueFieldConstraintvalidatorTest
instead ofUniqueFieldValueValidationTest
.It would also be more consistent with Tests existing on
core/tests/Drupal/KernelTests/Core/TypedData
.$entityType
,$fieldName
,$value
) ofUniqueFieldValueValidationTest
.$this->fieldName = $field_storage = ...
is not necessary. Let's remove= $field_storage =
testUniqueFieldValueConstraint
'@field_name' seems to be the label & not the field name.It's mostly because the whole tests use random label/fieldname/value generation (which is not really necessary but why not).
You can fix this by
Comment #7
claudiu.cristeaIn unit tests we are not translating strings unless we test a specific translation thing. See https://www.drupal.org/docs/8/phpunit/phpunit-browser-test-tutorial#t.
I agree with @wengerk, this is a validator provided by core, as an effect the test should live in core. And the right location would be
core/tests/Drupal/KernelTests/Core/Validation
. Also, I see no reason to extendFieldKernelTestBase
. We are not using anything from the abstract class. We should start, clean, fromKernelTestBase
.Assigned but not used. Remove it.
It seems that we can limit the scope of this variable only to
testUniqueFieldValueConstraint()
. I don't see being used elsewhere.There's no need to store the field storage in
$storage
You can do this more compact:Not that I removed the 'field_storage' key and added the 'entity_type' as @wengerk already noted.
The
$id
parameter misses a@param
entry in the dockblock.The
$field_storage =
part is useless. Same the. '_field_name'
suffix.What's the reason of this line?
Use a more dedicated assertion:
$this->assertEmpty()
.Looks as they are passing the same value.
Same, this line seems useless.
Use
$this->assertCount()
instead.s/private/protected
s/public/static. Also, why 'node' module? It needs only 'field' and 'entity_test'.
Commenting also on #6:
#6.1: Yes, probably it would make it more readable but keep in mind that
$this->randomMachineName()
generates UNIQUE names. They cannot collide. So, using the random generator is correct as well for the scope of the test.#6.2, 3: See my comment above.
#6.4: Yes, I agree that
UniqueFieldConstraintvalidatorTest
would be better.#6.5: Should be added.
#6.8, 9, 10: No. I think is the other way around:
UniqueFieldValueValidator
is buggy and should be fixed. How it comes that the validator is lowercasing the field name? It's should not do that, it's a bug. There are languages where there is no lowercase version. For example, in German, the nouns are starting every time with uppercase letter. This piece of code is destroying that. We should repair the code inUniqueFieldValueValidator
. This doesn't apply to the line above'@entity_type' => $entity->getEntityType()->getLowercaseLabel(),
, as that line uses a dedicated method to get the lowercased version and the German translation could store there a capitalized version.Comment #8
claudiu.cristeaAssigning to fix the #6 and #7.
Comment #9
claudiu.cristeaHere we go...
First, I managed to prove the bug with the actual code, so I'm publishing also a "test only" patch.
#6.1: Partially I changed the IDs to hardcoded ones but only where I needed to refer later the ID.
#6.2: Done.
#6.3: Moved in
core/tests/Drupal/KernelTests/Core/Validation
.#6.4: Renamed simply as
UniqueFieldConstraintTest
. I reorganized the test inside so that there are no per-class setups that are referring specifically to entities with string IDs. Now, the test class can be enriched with other tests.#6.5: Those properties were removed.
#6.6, 7 and #7.5: Removed. In fact, for simplicity, I removed that field and I tested with base field of
entity_test_string_id
entity type.#6.8, 9, 10: I fixed the root cause in
UniqueFieldValueValidator::validate()
.#7.1: Now I understand why the message has been translated: because it contains markup. But decided to use
FormattableMarkup
instead of translating.#7.2, 3: Done.
#7.4, 13: The class properties were removed.
#7.6: Done.
#7.7, 8, 11: We're now adding the constraints via a testing module.
#7.9: My proposal didn't work. Replaced with
$this->assertCount(0, ...)
.#7.10: Removed the duplicate. Added more edge cases.
#7.12: Done.
#7.14: Done.
Comment #12
claudiu.cristeaYeah, I expected that will break other tests that are testing unique value validation.
Comment #13
wengerkThanks for your works !
Every points seems to be adresse - from my point of view.
Nice catch with the isset/empty edge case for 0 &"0" !
RTBC
Ps: maybe kind of out-of-scoop fix with the lowercase bugfix - let's what other think.
Comment #14
PanchoComment #15
alexpottThis is an unrelated change that deserves its own discussion. Don't use a random field name in the test.
Comment #16
claudiu.cristeaReverted changes to message lowercasing.
Comment #17
wengerkCreate an issue #3029782: UniqueFieldValueValidator lowercasing Label of field on violation message as suggested by #15 & link it here
Comment #18
wengerkThe revert requested by #15 has been applied and an issue (#3029782: UniqueFieldValueValidator lowercasing Label of field on violation message) has been open to address the lowercase discussion.
We still should apply the suggestion from #15:
Comment #19
claudiu.cristeaThat has no impact. He's referring to the 2nd test where we need a clear determined label.
Comment #20
wengerkSeems legit. Let's RTBC.
Comment #21
wengerkComment #22
alexpottHmmm.... looking at this again I'm odd to consider when $entity_id is not set. If I remove the condition and run the test it passes. Why is the if here?
Comment #23
claudiu.cristea@alexpott
Sure, the tests are passing because
NULL !== {any other stored ID}
. The reason of theif(...)
condition is not for the main logic of the patch, but to avoid an additional condition in the entity query when there's no need for that condition. Why making the entity query process a useless condition for that case? Setting back to RTBC to show on @alexpott's radar.Comment #25
claudiu.cristeaUnrelated test failure.
Comment #26
alexpottCommitted and pushed ee822bb01d to 8.7.x and b55d893b44 to 8.6.x. Thanks!
Okay #23 makes sense. I pondered asking for a code comment but it's not so necessary.
As a non-disruptive bugfix backported to 8.6.x