Make Unique Field constraints generic

Having a generic implementation for this would be great.

Core has one more use case that is currently not implemented as a constraint at all.

See BlockContentForm::validateForm(). That's weird, custom, form-only validation code.

I'll go ahead and add a BlockContentDescriptionConstraint, seems like a good way to test if the patch works while the testbot isn't testing :)

Well adding a constraint on the block description worked quite nicely, so your patch makes sense :)

The generic test message doesn't have test coverage and the placeholders aren't easily override-able from the unique field constraints themselves but maybe that's for the better, as people only really need the field value when validating uniqueness.

And a test

Sounds like a good idea! I asked about this on IRC and apparently @Berdir had already tried to push for that in other issues but it didn't end up happening because people just wanted to get the issue done.

Nice work! Had a look at the interdiff and don't see any problem with this at first sight. If this passes tests, +1 :)

1. +++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/UniqueFieldConstraint.php
   @@ -0,0 +1,31 @@
   +  public $message = 'The %entity_type_id with the %field_name %value already exists.';
   

   Shouldn't we use the entity type label here, and not the id? There is a method to get the lowercase label I think. It's actually the same string for user in english, but not other languages, and for example it's block_content vs "custom block".

2. +++ b/core/modules/block_content/src/Entity/BlockContent.php
   @@ -187,7 +187,8 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   -      ->setDisplayConfigurable('form', TRUE);
   +      ->setDisplayConfigurable('form', TRUE)
   +      ->addConstraint('UniqueField', ['message' => 'A block with description %value already exists.']);
    
   

   Ah, you use custom messages then to replace the message completely. sneaky ;) Still, the default should be better I'd say :)

3. +++ b/core/modules/user/src/Tests/UserCreateTest.php
   @@ -115,4 +115,22 @@ public function testUserAdd() {
   +  /**
   +   * Tests the unique user email constraint.
   +   */
   +  public function testDuplicateUserEmail() {
   +    $user1 = $this->drupalCreateUser();
   

   We already have tests for this in UserValidationTest I think.

Tests are green, I think you can remove the custom messages now wherever it makes sense.

IMO we shouldn't remove all of them, but most can be removed.

I think we can lose the empty array in the second argument of addConstraint(). We also shouldn't be deleting that newline before the end of the closing brace of the class definition.

As to what will break, you could just do a grep for the error message and replace the verbiage in the relevant tests.

This will also need a beta evaluation.

1. +++ b/core/lib/Drupal/Core/Field/FieldConfigInterface.php
   @@ -178,7 +178,7 @@ public function addPropertyConstraints($name, array $constraints);
   -   *   $fields['field_username']->addConstraint('UserNameUnique', []);
   +   *   $fields['field_username']->addConstraint('UniqueField', []);
   

   This doesn't need a second argument either, or maybe use a custom message in the example?

2. +++ b/core/modules/block_content/src/Tests/BlockContentCreationTest.php
   @@ -66,8 +66,8 @@ public function testBlockContentCreation() {
   -    $this->assertRaw(format_string('A block with description %name already exists.', array(
   -      '%name' => $edit['info[0][value]']
   +    $this->assertRaw(format_string('The custom block with the block description %value already exists.', array(
   

   I wonder if the custom verbiage wasn't clearer than the automatic one. But maybe that just means we need to change the labels on the entity? (ie. s/custom block/block/)

3. +++ b/core/modules/user/src/Tests/UserRegistrationTest.php
   @@ -141,13 +141,13 @@ function testRegistrationEmailDuplicates() {
   -    $this->assertText(t('The email address @email is already taken.', array('@email' => $duplicate_user->getEmail())), 'Supplying an exact duplicate email address displays an error message');
   +    $this->assertText(t('The user with the email @email already exists.', array('@email' => $duplicate_user->getEmail())), 'Supplying an exact duplicate email address displays an error message');
   ...
   -    $this->assertText(t('The email address @email is already taken.', array('@email' => $duplicate_user->getEmail())), 'Supplying a duplicate email address with added whitespace displays an error message');
   +    $this->assertText(t('The user with the email @email already exists.', array('@email' => $duplicate_user->getEmail())), 'Supplying a duplicate email address with added whitespace displays an error message');
   

   In the examples I've quoted so far "A %entity_type" would've been more correct than "The %entity type". Maybe that would be better for a default message? Also does anyone know, have we standardized on "email" or "email address"? We shouldn't be mixing those up...

Hmm good point. "A/An" would look as odd as "The", and coding the grammar rules would be a bit of a convoluted way of avoiding custom messages.

Maybe we should stick with the custom messages then as they're more descriptive/end user-friendly, maybe @Berdir can chime in as he had suggested using the defaults.

I'm OK with keeping the custom messages. It's still a huge simplification compared to what we have now. Generic messages like that are always hard and they are very hard to translate as well, usually.

There is explicit support to translate the default string, but the custom strings need to be passed through t() explicitly, like the labels/descriptions on the field definitions.

Well @almaudoh if you have that doubt I'm sure others would have it as well, so wrapping custom messages in t() would remove that doubt :)

Also not sure it's worth making custom messages translated by default given unique constraints are a rarity and how little effort it is to just wrap a message in t(), which people are used to anyway.

+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/UniqueFieldValueValidator.php
@@ -37,7 +38,11 @@ public function validate($items, Constraint $constraint) {
+        '@entity_type' => $entity->getEntityType()->getLowercaseLabel(),
+        '@field_name' => Unicode::strtolower($items->getFieldDefinition()->getLabel()),

May also be worth checking if the labels are already translated (I guess so)?

Just looking at the interdiff and this looks great to me now.

Technically maybe the tests should say t() instead of format_string() but those tests are run in English anyway so I don't think that's worth changing.

Updating issue summary

@Berdir do you think any of this is Major? Or that "reduces fragility" might apply here?

1. +++ b/core/modules/block_content/src/Entity/BlockContent.php
   @@ -187,7 +187,8 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +      ->setDisplayConfigurable('form', TRUE)
   +      ->addConstraint('UniqueField', ['message' => t('A block with description %value already exists.')]);
   

   Hm, actually, this doesn't work.

   This will immediately translate without replacing the placeholder. So you will have a german string that still has the placeholder. Then we translate it again, using the german string as translation source.

   This is one of the cases where we, instead of actually translating, just need to mark it as a translatable string, which is unfortunately something that we still don't have.

2. +++ b/core/modules/block_content/src/Tests/BlockContentCreationTest.php
   @@ -66,8 +66,8 @@ public function testBlockContentCreation() {
   +    $this->assertRaw(format_string('A block with description %value already exists.', array(
   +      '%value' => $edit['info[0][value]']
   

   Can we switch this to SafeMarkup::format() when we touch it anyway? format_string() never got a @deprecated, but it really is..

@almaudoh yes that makes sense, updated issue summary.

As to the translation problem, indeed the translated strings would be retranslated in ExecutionContext::addViolation() so we could remove the t()'s but as @Berdir mentions then the potx extractor wouldn't find them, so probabably something like #1542144: Mark strings as localizable/translatable (new t()-alike string function that isn't t(), only for potx) / #2363099: Using translated field definition descriptions in entity schema results in different schema definitions, resulting in update.php changes would be the cleanest way to fix?

The BlockContent part of this is a critical bug - opened #2529892: BlockContentForm uses form validation instead of a validation Constraint after confirming with @catch

Attempt at a reroll

looks great

LGTM

Less boilerplate will help people make less mistakes. Committing under the committer discretion provision in the beta policy. Committed c73fdfc and pushed to 8.0.x. Thanks!