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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

claudiu.cristea created an issue. See original summary.

idimopoulos’s picture

I 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.

Status: Needs review » Needs work

The last submitted patch, 3: uniquefieldconstraint_string_ids-3027745-D8-3.patch, failed testing. View results

wengerk’s picture

wengerk’s picture

Thanks to @idimopoulos for addressing this issue with a patch and tests !

I still think there is room for improvement.

  1. Globally, I think we could simplify UniqueFieldValueValidationTest by fixing the $fieldname & $value instead of generating a random.
  2. Test group could be @group Validation instead of @field
  3. Most of the related issue put their tests on core/tests/Drupal/KernelTests/Core/TypedData instead of core/tests/Drupal/KernelTests/Core/Validation I don't know which is better
  4. The class could be more "generic" to be the place for all UniqueFieldValidations we will need (entity, string ID, Delta > 0, ...) and so I would suggest the classname UniqueFieldConstraintvalidatorTest instead of UniqueFieldValueValidationTest.
    It would also be more consistent with Tests existing on core/tests/Drupal/KernelTests/Core/TypedData.
  5. Missing explanatory comment for properties ($entityType, $fieldName, $value) of UniqueFieldValueValidationTest.
  6. I'm not sure the description key is necessary for our tests
      'description' => $this->randomMachineName() . '_description',
    
  7. $this->fieldName = $field_storage = ... is not necessary. Let's remove = $field_storage =
  8. in testUniqueFieldValueConstraint '@field_name' seems to be the label & not the field name.
  9. You may lowercase label to make your tests pass. if you still want to have a random label, which is not necessary - I think - If you want to assert a message with a specific label name.
  10. Tests fail with

    --- Expected
    +++ Actual
    @@ @@
    -'A test entity with string_id with eyG59DLz_label !i=>>&y1 already exists.'
    +'A test entity with string_id with eyg59dlz_label !i=>>&y1 already exists.'

    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

    -     '@field_name' => $entity->{$this->fieldName}->getFieldDefinition()->label(),
    +     '@field_name' => mb_strtolower($entity->{$this->fieldName}->getFieldDefinition()->label()),
    
claudiu.cristea’s picture

  1. +++ b/core/tests/Drupal/KernelTests/Core/Validation/UniqueFieldValueValidationTest.php
    @@ -0,0 +1,125 @@
    +use Drupal\Core\StringTranslation\StringTranslationTrait;
    ...
    +  use StringTranslationTrait;
    ...
    +    $message = $this->t('A @entity_type with @field_name %value already exists.', [
    

    In 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.

  2. +++ b/core/tests/Drupal/KernelTests/Core/Validation/UniqueFieldValueValidationTest.php
    @@ -0,0 +1,125 @@
    + * @group field
    ...
    +class UniqueFieldValueValidationTest extends FieldKernelTestBase {
    

    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 extend FieldKernelTestBase. We are not using anything from the abstract class. We should start, clean, from KernelTestBase.

  3. +++ b/core/tests/Drupal/KernelTests/Core/Validation/UniqueFieldValueValidationTest.php
    @@ -0,0 +1,125 @@
    +  /**
    +   * @var string
    +   */
    +  private $entityType;
    ...
    +    $this->entityType = 'entity_test';
    

    Assigned but not used. Remove it.

  4. +++ b/core/tests/Drupal/KernelTests/Core/Validation/UniqueFieldValueValidationTest.php
    @@ -0,0 +1,125 @@
    +  /**
    +   * @var string
    +   */
    +  private $value;
    ...
    +    $this->value = $this->randomString();
    ...
    +      $this->fieldName => $this->value,
    ...
    +      '%value' => $this->value,
    

    It seems that we can limit the scope of this variable only to testUniqueFieldValueConstraint(). I don't see being used elsewhere.

  5. +++ b/core/tests/Drupal/KernelTests/Core/Validation/UniqueFieldValueValidationTest.php
    @@ -0,0 +1,125 @@
    +    $storage = FieldStorageConfig::create([
    ...
    +    ]);
    +    $storage->save();
    ...
    +      'field_storage' => $storage,
    ...
    +    FieldConfig::create($field_definition)->save();
    

    There's no need to store the field storage in $storageYou can do this more compact:

    FieldStorageConfig::create([
      'field_name' => $this->fieldName,
      'entity_type' => 'entity_test_string_id',
      'type' => 'string',
      'constraints' => [
        'UniqueField' => [],
      ],
    ])->save();
    FieldConfig::create([
      'entity_type' => 'entity_test_string_id',
      'bundle' => 'entity_test_string_id',
      'field_name' => $this->fieldName,
      'label' => $this->randomMachineName(),
    ]);
    

    Not that I removed the 'field_storage' key and added the 'entity_type' as @wengerk already noted.

  6. +++ b/core/tests/Drupal/KernelTests/Core/Validation/UniqueFieldValueValidationTest.php
    @@ -0,0 +1,125 @@
    +  public function testUniqueFieldValueConstraint($id) {
    

    The $id parameter misses a @param entry in the dockblock.

  7. +++ b/core/tests/Drupal/KernelTests/Core/Validation/UniqueFieldValueValidationTest.php
    @@ -0,0 +1,125 @@
    +    $entity->{$this->fieldName}->getFieldDefinition()->addConstraint('UniqueField');
    

    The $field_storage = part is useless. Same the . '_field_name' suffix.

  8. +++ b/core/tests/Drupal/KernelTests/Core/Validation/UniqueFieldValueValidationTest.php
    @@ -0,0 +1,125 @@
    +    $entity->{$this->fieldName}->getFieldDefinition()->addConstraint('UniqueField');
    

    What's the reason of this line?

  9. +++ b/core/tests/Drupal/KernelTests/Core/Validation/UniqueFieldValueValidationTest.php
    @@ -0,0 +1,125 @@
    +    $this->assertEquals(0, count($violations));
    

    Use a more dedicated assertion: $this->assertEmpty().

  10. +++ b/core/tests/Drupal/KernelTests/Core/Validation/UniqueFieldValueValidationTest.php
    @@ -0,0 +1,125 @@
    +      'numeric id entity without an id' => [NULL],
    ...
    +      'string id entity with empty id' => [NULL],
    

    Looks as they are passing the same value.

  11. +++ b/core/tests/Drupal/KernelTests/Core/Validation/UniqueFieldValueValidationTest.php
    @@ -0,0 +1,125 @@
    +    $entity->{$this->fieldName}->getFieldDefinition()->addConstraint('UniqueField');
    

    Same, this line seems useless.

  12. +++ b/core/tests/Drupal/KernelTests/Core/Validation/UniqueFieldValueValidationTest.php
    @@ -0,0 +1,125 @@
    +    $this->assertEquals(1, count($violations));
    

    Use $this->assertCount() instead.

  13. +++ b/core/tests/Drupal/KernelTests/Core/Validation/UniqueFieldValueValidationTest.php
    @@ -0,0 +1,125 @@
    +  private $fieldName;
    ...
    +  private $value;
    

    s/private/protected

  14. +++ b/core/tests/Drupal/KernelTests/Core/Validation/UniqueFieldValueValidationTest.php
    @@ -0,0 +1,125 @@
    +  public static $modules = ['field', 'node', 'user'];
    

    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 in UniqueFieldValueValidator. 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.

claudiu.cristea’s picture

Assigned: Unassigned » claudiu.cristea

Assigning to fix the #6 and #7.

claudiu.cristea’s picture

Assigned: claudiu.cristea » Unassigned
Status: Needs work » Needs review
FileSize
3.54 KB
10.23 KB
6.57 KB

Here 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.

The last submitted patch, 9: 3027745-9.test-only.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 9: 3027745-9.patch, failed testing. View results

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
7.46 KB
917 bytes

Yeah, I expected that will break other tests that are testing unique value validation.

wengerk’s picture

Status: Needs review » Reviewed & tested by the community

Thanks 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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/UniqueFieldValueValidator.php
@@ -35,7 +42,7 @@ public function validate($items, Constraint $constraint) {
-        '@field_name' => mb_strtolower($items->getFieldDefinition()->getLabel()),
+        '@field_name' => $items->getFieldDefinition()->getLabel(),

+++ b/core/modules/block_content/tests/src/Functional/BlockContentValidationTest.php
@@ -32,7 +32,7 @@ public function testValidation() {
-    $this->assertEqual($violations[0]->getMessage(), format_string('A custom block with block description %value already exists.', [
+    $this->assertEqual($violations[0]->getMessage(), format_string('A custom block with Block description %value already exists.', [

This is an unrelated change that deserves its own discussion. Don't use a random field name in the test.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
2.49 KB
6.16 KB

Reverted changes to message lowercasing.

wengerk’s picture

wengerk’s picture

The 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:

Don't use a random field name in the test.
claudiu.cristea’s picture

That has no impact. He's referring to the 2nd test where we need a clear determined label.

wengerk’s picture

Status: Needs review » Reviewed & tested by the community

Seems legit. Let's RTBC.

wengerk’s picture

Issue summary: View changes
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/UniqueFieldValueValidator.php
@@ -23,9 +23,16 @@ public function validate($items, Constraint $constraint) {
+    // Using isset() instead of !empty() as 0 and '0' are valid ID values for
+    // entity types using string IDs.
+    if (isset($entity_id)) {
+      $query->condition($id_key, $entity_id, '<>');
+    }

Hmmm.... 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?

claudiu.cristea’s picture

Status: Needs work » Reviewed & tested by the community

@alexpott

Hmmm.... 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?

Sure, the tests are passing because NULL !== {any other stored ID}. The reason of the if(...) 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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 16: 3027745-16.patch, failed testing. View results

claudiu.cristea’s picture

Status: Needs work » Reviewed & tested by the community

Unrelated test failure.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 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

  • alexpott committed ee822bb on 8.7.x
    Issue #3027745 by claudiu.cristea, idimopoulos, wengerk, alexpott:...

  • alexpott committed b55d893 on 8.6.x
    Issue #3027745 by claudiu.cristea, idimopoulos, wengerk, alexpott:...

Status: Fixed » Closed (fixed)

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