Problem/Motivation

Taxonomy's test suite is littered with this sort of thing when creating vocabularies:

$vid = mb_strtolower($this->randomMachineName());

This isn't necessary. The calls to mb_strtolower() aren't really needed, because even though $this->randomMachineName() can return upper-case letters, those are allowed in machine names.

Steps to reproduce

N/A

Proposed resolution

When creating vocabularies in tests, don't call mb_strtolower($this->randomMachineName()) to generate vocabulary IDs. Either rely directly on $this->randomMachineName(), or use TaxonomyTestTrait::createVocabulary().

Remaining tasks

Implement the proposed resolution across Taxonomy's test suite, then commit the changes. No additional test coverage should be necessary.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

CommentFileSizeAuthor
#13 interdiff_4-13.txt6.26 KBsahil rohilla01
#13 3221140-13.patch6.26 KBsahil rohilla01
#4 3221140-4.patch7.26 KBguilhermevp

Issue fork drupal-3221140

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:

About issue forks

Comments

phenaproxima created an issue. See original summary.

xjm’s picture

xjm
she/her
Primary language English
commented

Using static vocabulary names would also be acceptable, unless the test is specifically testing conditions of variation in a name, in which case a data provider would be the solution. Probably the former. :)

guilhermevp’s picture

guilhermevp commented
Assigned: Unassigned » guilhermevp
guilhermevp’s picture

guilhermevp commented
Assigned: guilhermevp » Unassigned
Status: Active » Needs review
StatusFileSize
new 3221140-4.patch7.26 KB

Created a first patch for review, started by just removing mb_strtolower(), but if statics are preferred I can make the changes.

Please, review.

Status: Needs review » Needs work

The last submitted patch, 4: 3221140-4.patch, failed testing. View results

matroskeen’s picture

matroskeen
Primary language Ukrainian
Location 🇺🇦 Ukraine, Lutsk
commented

Isn't the machine name have the following requirements? "only lowercase letters, numbers, and underscores"
See related issues: #3174874: MediaTypeCreationTrait creates media type with invalid machine-readable name and #2556711: Tired of Unicode::strtolower($this->randomMachineName()).

And here is an active task to teach randomMachineName generate a valid machine name: #2972573: randomMachineName() should conform to processMachineName() pattern

guilhermevp’s picture

guilhermevp commented

Yes, that seems the case. Maybe I can use static machine names for that or use TaxonomyTestTrait::createVocabulary() if that change will happen at all.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

I think, for now, it's okay to change to static names where possible.

guilhermevp’s picture

guilhermevp commented
Assigned: Unassigned » guilhermevp

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.

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.

sahil rohilla01’s picture

sahil rohilla01 commented
StatusFileSize
new 3221140-13.patch6.26 KB
new interdiff_4-13.txt6.26 KB

Reroll patch-4

ameymudras’s picture

ameymudras commented

Going by the problem statement upper case letters are allowed in machine names but there is a constraint which exists that doesn't allow this

core/modules/field/src/Entity/FieldStorageConfig.php

 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");
    }

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

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

quietone’s picture

quietone commented
Assigned: guilhermevp » Unassigned
Status: Needs work » Closed (outdated)

This was fixed in #3376281: Random machine names no longer need to be wrapped in strtolower()

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.