Updated: Comment #14

Problem/Motivation

StringItem's have a 'maxlength'. This maxlength is not enforced by typed data validation constraint, however.

Proposed resolution

Automatically add a validation constraint based on the 'maxlength' setting.

Remaining tasks

Make tests pass.

User interface changes

None.

API changes

An additional validation constraint for StringItem's with a maxlength.

CommentFileSizeAuthor
#14 2181461-14-string-maxlength-validation.patch1.47 KBtstoeckler
#7 2181461-7-uuid-maxlength-schema.patch1.8 KBtstoeckler
#7 2181461-3-7-interdiff.txt1.97 KBtstoeckler
#3 2181461-2-uuid-schema.patch893 byteststoeckler
#3 2181461-1-2-interdiff.txt484 byteststoeckler
#1 2181461-1-uuid-schema.patch891 byteststoeckler
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Active » Needs review
FileSize
2181461-1-uuid-schema.patch891 bytes

Here we go.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Issue tags: +D8MA, +SprintWeekend2014
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
FileSize
2181461-1-2-interdiff.txt484 bytes
2181461-2-uuid-schema.patch893 bytes

Per @dawehner adding the missing newline at the end of the class.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Perfect, now we are in sync with the Constraint in the annotation.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Issue tags: +Entity Field API
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work

This change looks like this could be done by adding the following to the annotation:

 *   settings = {
 *     "max_length" = "128"
 *   },

See \Drupal\Core\Entity\Plugin\Field\FieldType\StringItem

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Needs work » Needs review
FileSize
2181461-3-7-interdiff.txt1.97 KB
2181461-7-uuid-maxlength-schema.patch1.8 KB

That's true, but then let's also automate the maxlength constraint.

For the extra nitpicky: I removed an unused use statement in UuidItem after generating the interdiff, sorry for that.

Status: Needs review » Needs work

The last submitted patch, 7: 2181461-7-uuid-maxlength-schema.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review

7: 2181461-7-uuid-maxlength-schema.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 7: 2181461-7-uuid-maxlength-schema.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

Faced with the same trouble in #2150511: [meta] Deduplicate the set of available field types
Can't find the place where this kind of annotation is parsed

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

Mostly all test failures caused by that all testing code checks explicit error message that vary depending on the constraint definition

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

The solution in #2002168: Convert form validation of terms to entity validation
Suppose better to limit scope by #3

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Title: UUID field items should enforce the maxlength in the schema » StringItem's should add a validation constraint according to their maxlenght setting
Issue summary: View changes
Status: Needs work » Needs review
FileSize
2181461-14-string-maxlength-validation.patch1.47 KB

This needed a re-roll. So here it is. The UuidItem annotation got updated in the meantime, so this just adds the validation constraint based on maxlength.

I also think it makes sense to split this from #2150511: [meta] Deduplicate the set of available field types. That issue has a much broader scope while it seems making the tests pass here will be non-trivial already.

I'm updating the title and issue summary for that.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Title: StringItem's should add a validation constraint according to their maxlenght setting » StringItem's should add a validation constraint according to their maxlength setting

Status: Needs review » Needs work

The last submitted patch, 14: 2181461-14-string-maxlength-validation.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Needs work » Closed (duplicate)

Huh, that's awesome. The exact same code was added already in #2002168: Convert form validation of terms to entity validation. So this is already fixed in HEAD. Thanks @klausi and @effulgentsia!!!