Problem/Motivation

user_validate_name() uses the type data system in order to validate a simple name. This introduces complexity as we discovered in #3431200: FieldItemList::getConstraints() incorrectly sets $maxMessage as TranslatableMarkup where FieldItemList is adding a Count field constraint that is unrelated to this UserName validation.

Steps to reproduce

Proposed resolution

Deprecate user_validate_name() and use the basic validator that does not require the typed data system.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3431203

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

kim.pepper created an issue. See original summary.

kim.pepper’s picture

Status: Active » Needs review
longwave’s picture

UserPasswordForm has a copy-paste of user_validate_name() that could also be replaced here:

    // First, see if the input is possibly valid as a username.
    $name = trim($form_state->getValue('name'));
    $definition = BaseFieldDefinition::create('string')
      ->addConstraint('UserName', []);
    $data = $this->typedDataManager->create($definition);
    $data->setValue($name);
    $violations = $data->validate();
    // Usernames have a maximum length shorter than email addresses. Only print
    // this error if the input is not valid as a username or email address.
    if ($violations->count() > 0 && !$this->emailValidator->isValid($name)) {
smustgrave’s picture

Status: Needs review » Needs work

Have not reviewed

But appears to have several test failures.

kim.pepper’s picture

Status: Needs work » Needs review

Thanks for the reviews. I feel like this could be simpler if we didn't need to use typed data as we are not validating an entity, but \Drupal\user\Plugin\Validation\Constraint\UserNameConstraintValidator assumes it is. 🤷

Also, since we are only checking the name, I think we can rename it to UserNameValidator.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Searched for user_validate_name() and appears all instances have been replaced.

Based on the small number in core assume that D11 should be fine for removal vs D12.

Test coverage can be seen here https://git.drupalcode.org/issue/drupal-3431203/-/jobs/1099143

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I've added some review comments to address - thanks!

kim.pepper’s picture

Status: Needs work » Needs review

Addressed feedback.

kim.pepper’s picture

Had a look for docs that say we require a @return annotation and couldn't find one. Only docs for describing the format of the @return annotation. https://www.drupal.org/docs/develop/standards/php/api-documentation-and-...

smustgrave’s picture

Status: Needs review » Needs work

For the @return statement. If phpcs doesn't complain about it being included think it can't hurt.

kim.pepper’s picture

Status: Needs work » Needs review

Addressed feedback.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Feedback appears to be addressed.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Given where we are in the release cycle and the number of usages I think we should push the deprecation of user_validate_name out to 12. See https://git.drupalcode.org/search?group_id=2&repository_ref=&scope=blobs...

Also I think we should update the code to call the new service for consistency.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community

Pushed the minor tweaks for #14

alexpott’s picture

Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 8e7d3e0 and pushed to 11.x. Thanks!
Committed 3194f76 and pushed to 10.3.x. Thanks!

  • alexpott committed 3194f768 on 10.3.x
    Issue #3431203 by kim.pepper, alexpott, smustgrave, longwave: Deprecate...

  • alexpott committed 8e7d3e0f on 11.x
    Issue #3431203 by kim.pepper, alexpott, smustgrave, longwave: Deprecate...

Status: Fixed » Closed (fixed)

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