randomMachineName() should conform to processMachineName() pattern

This is not a duplicate of #2556711: Tired of Unicode::strtolower($this->randomMachineName()). This is a broader solution that would make #2556711: Tired of Unicode::strtolower($this->randomMachineName()) unnecessary and address problems potentially still present in that approach.

This came up again in #3174874: MediaTypeCreationTrait creates media type with invalid machine-readable name where @lendude said:

It feels very strange to have a method called randomMachineName that generates invalid machine names. Wouldn't the better fix be to make sure randomMachineName returns a valid machine name for Drupal that matches what \Drupal\Core\Render\Element\MachineName expects?

So instead of wrapping calls to this method with mb_strtolower all over core (which is what happens currently, 121 times according to a quick find) shouldn't we just wrap it once in the place where you'd expect this to happen and get rid of all the wrapping?

So I would think, something like this.

If this is ok then we can remove all the wrappers around the current calls to randomMachineName()

There is a dirtier but DRYer approach possible - we could get machine names and other names to share their lower case uniqueness:

public function name($length = 8, $unique = FALSE) {
  $values = array_merge(range(65, 90), range(97, 122), range(48, 57));
  $max = count($values) - 1;
  $counter = 0;
  do {
    if ($counter == static::MAXIMUM_TRIES) {
      throw new \RuntimeException('Unable to generate a unique random name');
    }
    $str = chr(mt_rand(97, 122));
    for ($i = 1; $i < $length; $i++) {
      $str .= chr($values[mt_rand(0, $max)]);
    }
    $counter++;
-  } while ($unique && isset($this->names[$str]));
-  if ($unique) {
-    $this->names[$str] = TRUE;
+  } while ($unique && isset($this->names[mb_strtolower($str)]));
+  if ($unique) {
+    $this->names[mb_strtolower($str)] = TRUE;
  }
  return $str;
}

+ public function machineName($length = 8, $unique = FALSE) {
+   return mb_strtolower($this->name($length, $unique));
+ }

This should fix the failing tests.

Not sure about #6, yeah we could reuse some code but I like that this is a little more explicit. ¯\_(ツ)_/¯

Updated Schematest and SqlContentEntityStorageSchemaTest to use machinename() instead of name()

Updated Schematest and SqlContentEntityStorageSchemaTest to use machinename() instead of name()

@anmolgoyal74 uhhh? Why? How is that in scope?

The machine name is not universal. Random text containing underscore (e.g. cy7q0twj5qpl98_c) is not valid for menu.

1) Drupal\Tests\menu_ui\Functional\MenuUiTest::testMenu
Behat\Mink\Exception\ExpectationException: The string "Menu cy7q0twj5qpl98_c has been added." was not found anywhere in the HTML response of the current page.

"The machine-readable name must contain only lowercase letters, numbers, and hyphens."

Can we add a pattern of allowed characters as a parameter of the randomMachineName method?

--- a/core/tests/Drupal/Tests/RandomGeneratorTrait.php
+++ b/core/tests/Drupal/Tests/RandomGeneratorTrait.php
@@ -84,14 +84,16 @@ public function randomStringValidate($string) {
    *
    * @param int $length
    *   Length of random string to generate.
+   * @param string $allowed_regex
+   *   Regex patern for allowed characters.
    *
    * @return string
    *   Randomly generated unique string.
    *
    * @see \Drupal\Component\Utility\Random::name()
    */
-  protected function randomMachineName($length = 8) {
-    return $this->getRandomGenerator()->name($length, TRUE);
+  protected function randomMachineName($length = 8, $allowed_regex = '[a-z0-9_]') {
+    return $this->getRandomGenerator()->machineName($length, TRUE, $allowed_regex);
   }

--- a/core/modules/menu_ui/tests/src/Functional/MenuUiTest.php
+++ b/core/modules/menu_ui/tests/src/Functional/MenuUiTest.php
@@ -205,7 +205,7 @@ public function addCustomMenu() {
     // Try adding a menu using a menu_name that is too long.
     $this->drupalGet('admin/structure/menu/add');
     $menu_name = strtolower($this->randomMachineName(MenuStorage::MAX_ID_LENGTH + 1));
-    $label = $this->randomMachineName(16);
+    $label = $this->randomMachineName(16, '[a-z0-9-]');
     $edit = [
       'id' => $menu_name,
       'description' => '',

...or something like $validator from the string($length, $unique, $validator) method.

  * @param callable $validator
   *   (optional) A callable to validate the string. Defaults to NULL.
   *
   * @return string
   *   Randomly generated string.
   *
   * @see \Drupal\Component\Utility\Random::name()
   */
  public function string($length = 8, $unique = FALSE, $validator = NULL) {

Can we add a pattern of allowed characters as a parameter of the randomMachineName method?

I suggest we should not do this.

The randomMachineName() method is basically a helper to simplify using randomName(). It seems to me that it should be really simple.

The idea of a pattern of allowed characters makes sense, but let's make that a separate issue for adding that option to randomName().

This also gives me an idea for an alternative way to DRY randomMachineName: we could add an optional $lower_case parameter to it, and make randomMachineName a wrapper around randomName..

1. Let's check where machine name cannot contain underscore.

2. See #12. Can we set any machine names instead of random where underscore is not allowed?

--- a/core/modules/menu_ui/tests/src/Functional/MenuUiTest.php
+++ b/core/modules/menu_ui/tests/src/Functional/MenuUiTest.php
@@ -205,7 +205,7 @@ public function addCustomMenu() {
     // Try adding a menu using a menu_name that is too long.
     $this->drupalGet('admin/structure/menu/add');
     $menu_name = strtolower($this->randomMachineName(MenuStorage::MAX_ID_LENGTH + 1));
-    $label = $this->randomMachineName(16);
+    $label = 'tested-menu-label';
     $edit = [
       'id' => $menu_name,
       'description' => '',

1. #7 is a good approach. Setting any machine names (like #15.2) will fix the tests.
2. Changing the behavior of randomMachineName requires change record.

#15.1 is awesome! It looks like you've identified 28 random fails (each a critical issue). There are a significant concern for core maintainers. But this seems to be a seperate issue to this one - why should we do this together in this issue?

Sorry, I'm wrong. #15 has nothing to do with random failures. It puts a character into random::name that is not normally there.

We commonly use the randomMachineName method because there is no other that generates a random string containing only letters and numbers. Unfortunately, it is also used in situations for which it was not intended, e.g. user roles, block or menu ids where underscore is not allowed.

2) Drupal\Tests\user\Functional\UserRolesAssignmentTest::testCreateUserWithRole
Behat\Mink\Exception\ElementNotFoundException: Form field with id|name|label|value "edit-roles-a_yy59h6" not found.

1) Drupal\Tests\menu_ui\Functional\MenuUiTest::testMenu
Behat\Mink\Exception\ExpectationException: The string "Menu cy7q0twj5qpl98_c has been added." was not found anywhere in the HTML response of the current page.

1) Drupal\Tests\block\Functional\Views\DisplayBlockTest::testDeleteBlockDisplay
The block e_hkq0dt appears on the page
Failed asserting that false is true.

How about this approach:

Utility/Random

  public function name($length = 8, $unique = FALSE, $exclude='') {
    $values = array_merge(range(65, 90), range(97, 122), range(48, 57));
    $values = array_values(array_diff($values, str_split($exclude)));
     ...
  }

RandomGeneratorTrait

  protected function randomMachineName($length = 8) {
    return $this
      ->getRandomGenerator()
      ->name($length, TRUE, 'ABCDEFGHIJKLMNOPQRSTUVXYZ);
  }

  protected function randomDomId($length = 8) {
    return $this
      ->getRandomGenerator()
      ->name($length, TRUE, '_');
  }

I added a separate issue #3176270: Add to Drupal\Component\Utility\Random::string an additional parameter of possible characters to select from. Then we will change the randomMachineName() easier. Any other method can be added in a similar way, e.g. randomDomId().

protected function randomMachineName($length = 8) {
  $characters = array_merge(range('a', 'z'), range(0, 9), ['_']);
  return $this->getRandomGenerator()->string($length, TRUE, function ($str) {
    return ctype_lower($str[0]);
  }, $characters);
}

Drupal\Tests\RandomGeneratorTrait::randomMachineName() now generates random strings of lowercase letters, numbers and underscore

Using #3176270: Add to Drupal\Component\Utility\Random::string an additional parameter of possible characters to select from in randomMachineName is not simple. #7 and Random::Name have a narrower criteria for the first character in the string than for the rest of the string.

      $str = chr(mt_rand(97, 122));
      for ($i = 1; $i < $length; $i++) {
        $str .= chr($values[mt_rand(0, $max)]);

Using #3176270 in randomMachineName is not simple. #7 and Random::Name have a narrower criteria for the first character in the string than for the rest of the string.

I agree that #7 is simpler. The disadvantage is that it requires a new method machineName. An additional parameter of possible characters is more universal.

Let's go for the simple approach first. Let's remove the underscore so it should be compatible with all machine names in use.

In#27 we duplicate many methods (name -> machineName, testRandomMachineNameException -> testRandomNameException, testRandomNamesUniqueness -> testRandomMachineNamesUniqueness, testRandomNameNonUnique -> testRandomMachineNameNonUnique).

With #3176270: Add to Drupal\Component\Utility\Random::string an additional parameter of possible characters to select from it is much simpler.

protected function randomMachineName($length = 8) {
  $characters = array_merge(range('a', 'z'), range(0, 9));
  return $this->getRandomGenerator()->string($length, TRUE, NULL, $characters);
}

Easier to understand.

-    $values = array_merge(range(97, 122), range(48, 57));
+    $values = array_merge(range('a', 'z'), range(0, 9));

...

-        $str .= chr($values[random_int(0, $max)]);
+        $str .= $values[random_int(0, $max)];

#28 is true, but doesn't need to be an argument for postponing this.

The test fails are because the first character needs different handling, as mentioned #23.

So, did I get it green this time?

1. +++ b/core/lib/Drupal/Component/Utility/Random.php
   @@ -130,6 +137,47 @@ public function name($length = 8, $unique = FALSE) {
   +   *   Length of random string to generate.
   

   Nit:(optional)

2. +++ b/core/lib/Drupal/Component/Utility/Random.php
   @@ -130,6 +137,47 @@ public function name($length = 8, $unique = FALSE) {
   +    if ($unique) {
   +      $this->machineNames[$str] = TRUE;
   +    }
   

   This is a mistake. $unique is supposed to guarantee the uniqueness of the returned string. But we should store the generated string even if this call does not require uniqueness, so that other calls to the method can avoid enforce uniqueness if they want. However, the other methods in this class have the same issue, so it makes sense to do it this way here for consistency.

3. +++ b/core/tests/Drupal/Tests/Component/Utility/RandomTest.php
   @@ -115,6 +115,52 @@ public function testRandomStringNonUnique() {
   +    for ($i = 0; $i <= 10; $i++) {
   

   There's a reasonable chance of this test passing even without enforcing uniqueness as there are 36 possible characters. I wonder if it should be $i <= 25.

Thanks for your suggestions @jonathanshaw
Addressed comment #34 point 1,3
Attached patch against Drupal 10.1.x
Attached reroll patch

1. +++ b/core/lib/Drupal/Component/Utility/Random.php
   @@ -130,6 +137,47 @@ public function name($length = 8, $unique = FALSE) {
   +   *   Randomly generated string.
   +   *
   +   * @see \Drupal\Component\Utility\Random::string()
   +   */
   +  public function machineName($length = 8, $unique = FALSE) {
   +    $values = array_merge(range('a', 'z'), range(0, 9));
   +    $start_characters = range('a', 'z');
   

   This ought to be able to have scalar type hints and a return type hint now.

   Also needs a @throws for the exception.

2. +++ b/core/tests/Drupal/Tests/Component/Utility/RandomTest.php
   @@ -115,6 +115,52 @@ public function testRandomStringNonUnique() {
   +    for ($i = 0; $i <= 25; $i++) {
   +      $str = $random->machineName(1, TRUE);
   +      $this->assertFalse(isset($names[$str]), 'Generated duplicate random name ' . $str);
   +      $names[$str] = TRUE;
   +    }
   +  }
   

   I think this could use assertArrayNotHasKey()

I uploaded the patch and addressed #41 both the point. Please review it

Thank for the patch @anil_gupta!

This ought to be able to have scalar type hints and a return type hint now

You missed this review point from #41.

Exception thrown when either $counteror are not generate a unique random machine name.

Let's improve the grammar/spelling:
Thrown if a unique machine name cannot be generated within the allowed number of random attempts.

Please set this issue to Need Review when uploading a patch in order to run the tests.

Addressed comment #43, please review.

I think this may be causing a random failure #3354138: [random test failure] MenuUiTest::testMenuAdministration can generate "escaped locator"

Created an MR, starting from #39 since #42 and #44 didn't actually address the feedback (and 44 changed code out of scope of this issue)

Hiding all old patches.

Related, #3353658: [PHPUnit 10] Provide a static alternative to randomMachineName() and implement in data providers.

Changes look good but could the issue summary be updated with the solution.

Example if this is just replacing $this->getRandomGenerator()->name($length

IS updated

Thanks that clears things up!

Needs rebase/merge following #3353658: [PHPUnit 10] Provide a static alternative to randomMachineName() and implement in data providers

Also added a couple of nitpicks to the test.

Added the @longwave's suggestions and moved the changes to the branch 11.x
Please review.

Reviewing 4193

Reroll seems good and points made on the tests have been addressed.

I don't think the reroll is correct, it is undoing some of the recent work in #3358416: Use RandomGeneratorTrait in UnitTestCase and remove direct implementations of randomMachineName and getRandomGenerator and #3353658: [PHPUnit 10] Provide a static alternative to randomMachineName() and implement in data providers

Sorry for messing it up, trying to fix it.

Please review.

Left a comment on the MR.

Thanks!

@smustgrave sorry, I don't see the comment, can you link it in a comment, please.

I ran into this very problem too over at #3361534-57: KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate.

In fixing it, I noticed that a bunch of places do

strtolower($this->randomMachineName(8))

Shouldn't this issue remove all those strtolower() (and mb_strtolower()) occurrences? 🤔

Because this issue only touches Random::machineName(), but \Drupal\Tests\RandomGeneratorTrait::randomMachineName() calls it:

  protected function randomMachineName($length = 8) {
    return Random::machineName($length);
  }

This now blocks #3361534 since per @longwave in #3361534-58: KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate, I don't think I can land that issue without this being fixed first.

The current implementation is on its own responsible for a huge number of invalid config entities, but for now we're all blissfully unaware 😅

This last commit introduced new test failures

Discussed with @longwave that the (new Random()) here that is wrapped in an anonymous instance is the problem which lead to the the test fail in the last patch. Reverted that change, back to rtbc.

Added typehint and default value to the new property.

Somewhere the code that actually uses this new method has been lost; we need to update \Drupal\TestTools\Random to use this new method instead of ->name()?

Back to needs review.

Remarking. #69 appeared to be addressed in https://git.drupalcode.org/project/drupal/-/merge_requests/4193/diffs?co...

Previous reviews still apply.

@Wim Leers makes a good point in #64 that we should no longer need to wrap random machine names in strtolower(), let's open a followup to clean that up.

Committed 9a1ce8c and pushed to 11.x. Thanks!

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

Looks like this caused a new random fail: #3376563: Random test fail in Drupal\Tests\Component\Utility\RandomTest::testRandomMachineNamesUniqueness. 😅