Problem/Motivation

As discovered in #3027745: UniqueFieldConstraint doesn't work for entities with string IDs, the UniqueFieldValueValidator lowerify the label of field when generating the message of violation.

See https://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Core/Validation/...

$this->context->addViolation($constraint->message, [
  '%value' => $item->value,
  '@entity_type' => $entity->getEntityType()->getLowercaseLabel(),
  '@field_name' => mb_strtolower($items->getFieldDefinition()->getLabel()),
]);

This is unnecessary as the Label should be displayed as seen in the Drupal UI.

Proposed resolution

Remove the mb_strtolower.

Remaining tasks

  • Write a patch
  • Write test(s)
  • Review

User interface changes

  • On violation of UniqueFieldValueValidator, the field name will be displayed as seen on the Admin and will not anymore be altered.

API changes

None.

Data model changes

None.

Release notes snippet

N/A

CommentFileSizeAuthor
#34 3029782-34.patch2.51 KBkarishmaamin
#26 3029782-26.patch2.53 KByogeshmpawar
#12 3029782-12.patch2.49 KBwengerk
#12 interdiff-3029782-06-12.txt1.67 KBwengerk
#6 interdiff-3029782-03-06.txt735 byteswengerk
#6 3029782-06.patch1.72 KBwengerk
#3 3029782-3.patch847 bytesknyshuk.vova
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

wengerk created an issue. See original summary.

knyshuk.vova’s picture

knyshuk.vova CreditAttribution: knyshuk.vova at Internetdevels, Drupal Ukraine Community commented
Assigned: Unassigned » knyshuk.vova
Issue tags: +LutskGCW19
knyshuk.vova’s picture

knyshuk.vova CreditAttribution: knyshuk.vova at Internetdevels, Drupal Ukraine Community commented
Assigned: knyshuk.vova » Unassigned
Status: Needs work » Needs review
FileSize
3029782-3.patch847 bytes

Removed the mb_strtolower.

abramm’s picture

abramm
Primary language Ukrainian
Location Lutsk
CreditAttribution: abramm as a volunteer and at DevBranch, Drupal Ukraine Community commented
Related issues: +#2999225: Unique field constraint does not validated field values on delta greater than 0

Adding #2999225: Unique field constraint does not validated field values on delta greater than 0 as relation since the mentioned issue adds UniqueFileValueValidator test which may need to be updated as well.

Status: Needs review » Needs work

The last submitted patch, 3: 3029782-3.patch, failed testing. View results

wengerk’s picture

wengerk
he/him
Primary language French
Location Lausanne
CreditAttribution: wengerk at Antistatique commented
Status: Needs work » Needs review
Issue tags: -Need tests, -needs patch +beginner
FileSize
3029782-06.patch1.72 KB
interdiff-3029782-03-06.txt735 bytes

Here the fix for tests.

abramm’s picture

abramm
Primary language Ukrainian
Location Lutsk
CreditAttribution: abramm as a volunteer and at DevBranch, Drupal Ukraine Community commented
Status: Needs review » Reviewed & tested by the community

#6 looks good for me, as long as CI passes.

abramm’s picture

abramm
Primary language Ukrainian
Location Lutsk
CreditAttribution: abramm as a volunteer and at DevBranch, Drupal Ukraine Community commented

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: 3029782-06.patch, failed testing. View results

Pancho’s picture

Pancho
Location UTC+2 🇪🇺 EU
CreditAttribution: Pancho as a volunteer commented
Status: Needs work » Reviewed & tested by the community

Was green until yesterday. Testbot is heavily confused. Back to previous status.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: 3029782-06.patch, failed testing. View results

wengerk’s picture

wengerk
he/him
Primary language French
Location Lausanne
CreditAttribution: wengerk at Antistatique commented
Status: Needs work » Needs review
Issue tags: +drupalmountaincamp
FileSize
interdiff-3029782-06-12.txt1.67 KB
3029782-12.patch2.49 KB

Here a reroll, let's try it !

The last submitted patch, 6: 3029782-06.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 12: 3029782-12.patch, failed testing. View results

Pancho’s picture

Pancho
Location UTC+2 🇪🇺 EU
CreditAttribution: Pancho as a volunteer commented
Status: Needs work » Needs review

Testbot is green now.

wengerk’s picture

wengerk
he/him
Primary language French
Location Lausanne
CreditAttribution: wengerk at Antistatique commented
Issue summary: View changes

Update remaining tasks

wengerk’s picture

wengerk
he/him
Primary language French
Location Lausanne
CreditAttribution: wengerk at Antistatique commented
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon commented
Status: Needs review » Reviewed & tested by the community

This is legit. Labels should not be lowercased because they may look wrong in some languages, such as German. For example there's no lowercase for Apfel.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/UniqueFieldValueValidator.php
@@ -42,7 +42,7 @@ public function validate($items, Constraint $constraint) {
         '@entity_type' => $entity->getEntityType()->getLowercaseLabel(),
-        '@field_name' => mb_strtolower($items->getFieldDefinition()->getLabel()),
+        '@field_name' => $items->getFieldDefinition()->getLabel(),

I think we need to step back and have a more general re-think about labels and cases. If you look at the code we're fixing just above we call $entity->getEntityType()->getLowercaseLabel(), ... which does mb_strtolower($this->getLabel()); - also wrong in German and other languages. Fixing this piecemeal is going to end up with lots of inconsistencies. As in why is the resulting violation got an uppercase field name but a lowercase entity type name.

Yes we do need to fix the use of lowercasing in core and make it something that languages can override. So please don't take this comment as no we shouldn't do this.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Issue tags: +Bug Smash Initiative, +Needs reroll

@alexpott's concerns in #20 have been addressed in that getLowercaseLabel() has been removed in #2507235: EntityType::getLowercaseLabel() breaks translation - getSingularLabel() is used instead now in this class.

However, the mb_strtolower() still exists so as per the original report I think we can reroll the patch here to fix this, and then this should be acceptable to be committed to core.

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location 🇮🇳 Pune(MH)
CreditAttribution: yogeshmpawar as a volunteer and at QED42 commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
3029782-26.patch2.53 KB

A working patch against 9.3.x branch.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Reviewed & tested by the community

Thanks!

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 26: 3029782-26.patch, failed testing. View results

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon commented
Status: Needs work » Reviewed & tested by the community

Unrelated failure

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 26: 3029782-26.patch, failed testing. View results

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon commented
Status: Needs work » Reviewed & tested by the community
quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll.

karishmaamin’s picture

karishmaamin CreditAttribution: karishmaamin at Specbee commented
Status: Needs work » Needs review
FileSize
3029782-34.patch2.51 KB

Re-rolled against 9.4.x.Please review

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll

#26 and #34 look identical to me, not sure this needed a reroll - back to RTBC either way.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 34: 3029782-34.patch, failed testing. View results

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs work » Reviewed & tested by the community

Unrelated fail, back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 34: 3029782-34.patch, failed testing. View results

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Andrew Answer’s picture

Andrew Answer CreditAttribution: Andrew Answer as a volunteer commented
Status: Needs work » Reviewed & tested by the community

Strange reset, let's try again.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Version: 10.1.x-dev » 9.5.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed ac84424e52 to 10.1.x and 904319041f to 10.0.x and 403eb8dddc to 9.5.x. Thanks!

  • alexpott committed ac84424 on 10.1.x 
    Issue #3029782 by wengerk, knyshuk.vova, yogeshmpawar, karishmaamin,...

  • alexpott committed 9043190 on 10.0.x 
    Issue #3029782 by wengerk, knyshuk.vova, yogeshmpawar, karishmaamin,...

  • alexpott committed 403eb8d on 9.5.x 
    Issue #3029782 by wengerk, knyshuk.vova, yogeshmpawar, karishmaamin,...

Status: Fixed » Closed (fixed)

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