Add validation constraints to field_ui.settings

🏓 I don't think this is quite right yet.

This looks good to me, i don't think we should decrease the size even further.

What would happen if you had an existing field prefix that's invalid according to the new validation?

Is this something that can be fixed though after the field is created?

I think that is already handled in core/modules/field/src/Entity/FieldStorageConfig::__construct

if (!preg_match('/^[_a-z]+[_a-z0-9]*$/', $values['field_name'])) {
throw new FieldException("Attempt to create a field storage {$values['field_name']} with invalid characters. Only lowercase alphanumeric characters and underscores are allowed, and only lowercase letters and underscore are allowed as the first character");
}

#11++ … and #13++ for answering it 😄

But … @narendraR, you literally quoted the code that proves that the validation constraints you added to the config schema are incorrect 😅

Adding _ in the end of field prefix failed core/modules/field_ui/tests/src/Functional/ManageFieldsFunctionalTest::testFieldPrefix. Either that test needs to be adjusted as per new validation rule or _ in the end of field prefix is not necessary. For now, I am removing the _ from field prefix end.

#15: that's not what I meant — I meant disallowing the field name (and hence also the prefix) to start with [0-9].

Sorry, but I don't get #17. As per #13, Only lowercase alphanumeric characters and underscores are allowed, and only lowercase letters and underscore are allowed as the first character.

Ah, I see — I had only seen the two underscore-related commits (eafb29c0 and 5b82dd9a), not the one that fixed the regex (f7ea3188) — sorry about that! 🙈

Indeed, I don't think a trailing _ is required for the prefix — it could also be just foo as a prefix for example.

I think this looks ready now, except for one nit (left a suggestion for it) and one question/bit of ambiguity.

Looks good now!

With #11 I actually meant what happens if the field prefix is over 30 characters already (although glad my vague question found a real bug).

Back to needs review for #22.

I'm guessing we need an update path to handle that case. :(

Sorry I'm not sure about the update path either - this is silently updating someone's config and field names aren't changeable once they make a new field, which they might do without realising.

I don't really believe anyone has a 40 (or even 31) character field prefix, but if their field prefix was iloveantidisestablishmentarianismverymuch would we really want to shorten that to <code>iloveantidisestablishmentarianismverymu.

If all that happens when the field prefix is too long, is they get a validation error if they try to save that form again, could we add a hook_requirements() warning, then they can fix it to a shorter string of their choosing? Or something like that seems OK as long as config import/export is unaffected.

#27: a 40-character long prefix is impossible already thanks to \Drupal\field\Entity\FieldStorageConfig::preSaveNew() doing:

    // Field name cannot be longer than FieldStorageConfig::NAME_MAX_LENGTH
    // characters. We use mb_strlen() because the DB layer assumes that column
    // widths are given in characters rather than bytes.
    if (mb_strlen($this->getName()) > static::NAME_MAX_LENGTH) {
      throw new FieldException('Attempt to create a field storage with an name longer than ' . static::NAME_MAX_LENGTH . ' characters: ' . $this->getName());
    }

I agree that should be better documented though.

Not sure what next should be done in this issue, hence marking it for another review.

Addressed @catch's review like I suggested in #28: with slightly expanded docs. I think this is ready now.

Discussed with @catch - I don't think we should have an update path here. Because as wim points out on the less you’d already have problems if it was over 31 characters… and practically you do with the new limit of 30… every field name would have to only be 2 letters… so I just so think the upgrade path is worth it.

Also see #27

Okay, I'm going to guess this is therefore RTBC once tests pass!

Committed and pushed 6c6c22c36a to 11.x and fb746dae75 to 10.3.x. Thanks!

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