Problem/Motivation

It's not possible to generate sample data with defined random seed for the "string" or "link" field type. For other field types that I have checked, it works as expected.

Proposed resolution

After some investigation, I have found that `Random::word` sets the seed with every call. We should remove this random seed

CommentFileSizeAuthor
#16 3109767-15.patch1.73 KBquietone
#16 3109767-15-fail.patch1.09 KBquietone
#16 interdiff-12-15.txt1.03 KBquietone
#12 3109767-12.patch1.79 KBquietone
#12 3109767-12-fail.patch1.15 KBquietone
#2 3109767_2.patch654 bytesmtodor

Comments

mtodor created an issue. See original summary.

mtodor’s picture

mtodor commented
Status: Active » Needs review
StatusFileSize
new 3109767_2.patch654 bytes

Here is the patch with the proposed solution.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented

@mtodor Would you provide manual test instructions to reproduce the issue? Thanks!

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Status: Needs review » Postponed (maintainer needs more info)

Moving to "Postponed" as it's hard to know how to review this without more information. Need to also make sure this is covered by existing tests or needs new one.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented

In an attempt to understand this better, I see the use of word() here for string and link. I still don't understand how to manually test this though.

StringItem.php

  public static function generateSampleValue(FieldDefinitionInterface $field_definition) {
    $random = new Random();
    $values['value'] = $random
      ->word(mt_rand(1, $field_definition
      ->getSetting('max_length')));
    return $values;
  }

LinkItem.php

public static function generateSampleValue(FieldDefinitionInterface $field_definition) {
...
      $values['uri'] = 'http://www.' . $random
        ->word($domain_length) . '.' . $tlds[mt_rand(0, sizeof($tlds) - 1)];
...
kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Issue tags: +Bug Smash Initiative

If this is technically not working as the issue summary explains, IMO this should be a bug rather than feature request.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Category: Feature request » Bug report

Whoops. Trying again.

mtodor’s picture

mtodor commented

Sorry @kristen-pol for the delayed response. I'm not working with Drupal actively anymore.

The problem here is the following. With every execution of Random::word random seed will be changed. And that's not something we should do inside a function that generates random values. The main reason for that is that seed is defined globally and changing it could affect the behavior of other functions.

The main purpose of using defined seeds is if you want to have defined order of random values. And place when you could need that are tests. We needed defined seeding for our performance testing in Thunder distribution. What we do, is that we define seed at the beginning of testing srand and then we are using Drupal random generator function. With defined srand we have always the same order of random values in our tests. All other random generating functions are not changing defined seed, except Random::word and that makes unpredictable test values.

If you execute following code, you will get better picture:

// You will get different values for every execution
echo rand() . PHP_EOL;

srand(0);
// You will get always same values for every execution
echo rand() . PHP_EOL;
echo rand() . PHP_EOL;
echo rand() . PHP_EOL;

So the only bad thing that Random::word is doing, that it's changing defined srand every time you call it.

If you want to test it, you can simply set srand and call Random::word and you should always get same order of random values. With the old version, you would always get an unpredictable order of random values.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Status: Postponed (maintainer needs more info) » Needs review

@mtodor Thanks for the info! Moving back to needs review.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

quietone’s picture

quietone commented
Issue summary: View changes
StatusFileSize
new 3109767-12-fail.patch1.15 KB
new 3109767-12.patch1.79 KB

the use of mt_srand was added early in the issue that added Random with no explanation for that particular line.

The PHP documentation for srand states "Note: There is no need to seed the random number generator with srand() or mt_srand() as this is done automatically." So, that line is unnecessary.

The code sample in #9 isn't quite right. 

php > // You will get different values for every execution
php > echo rand() . PHP_EOL;
118498407
php > 
php > srand(0);
php > // You will get always same values for every execution
php > echo rand() . PHP_EOL;
1178568022
php > echo rand() . PHP_EOL;
1273124119
php > echo rand() . PHP_EOL;
1535857466
php >

The only way to get the same random value is to set the seed before each rand(). That agrees with the doc.

php > srand(0);echo rand() . PHP_EOL;
1178568022
php > srand(0);echo rand() . PHP_EOL;
1178568022
php > srand(0);echo rand() . PHP_EOL;
1178568022

Anyway, the line isn't really needed. Here is a patch with a fail test.

The last submitted patch, 12: 3109767-12-fail.patch, failed testing. View results

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented

Thanks for the test. Some nitpicks and a question :)

  1. +++ b/core/tests/Drupal/Tests/Component/Utility/RandomTest.php
@@ -142,6 +142,28 @@ public function testRandomStringValidator() {
+    // Test that without a seed a different word is returned each time.

    Nitpick: Wording seems a bit awkward... maybe something like:

    Without a seed, test a different word is returned each time.

  2. +++ b/core/tests/Drupal/Tests/Component/Utility/RandomTest.php
@@ -142,6 +142,28 @@ public function testRandomStringValidator() {
+    $this->firstStringGenerated = '';

    I see this is the approach used in testRandomStringValidator but I'm unclear why this code isn't similar to the seeded section so that it's something like the following. i.e. why does there need to be a check between an empty string and a random word?

        $this->firstStringGenerated = $random->word(5);
    $next_str = $random->word(5);
    $this->assertNotEquals($this->firstStringGenerated, $next_str);

  3. +++ b/core/tests/Drupal/Tests/Component/Utility/RandomTest.php
@@ -142,6 +142,28 @@ public function testRandomStringValidator() {
+    // Test that with a seed the same word is returned each time.

    Nitpick: Wording seems a bit awkward... maybe something like:

    With a seed, test the same word is returned each time.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

quietone commented
StatusFileSize
new interdiff-12-15.txt1.03 KB
new 3109767-15-fail.patch1.09 KB
new 3109767-15.patch1.73 KB

@Kristen Pol, thanks for the review

1 and 3. Yes, your suggestion is better.
2. I have no idea why I did that. And again, your suggestion is better.

Since these are changes to the test I made a new fail patch.

The last submitted patch, 16: 3109767-15-fail.patch, failed testing. View results

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Status: Needs review » Reviewed & tested by the community

Thanks for the updates! This looks good to me and tests are passing, so marking RTBC.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

I did some further digging and found the code that added the use of srand was added to devel_generate in #160557: abstract taxonomy creation to devel_generate.inc which was then ported to core in #2320157: Generate placeholder content for Field types

There is no discussion of why srand was used there.

I agree with the reporter, the use of Random::word should not generate side-effects.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Crediting myself for going back into ancient git logs to find the origins of that code.

  • larowlan committed 672d5d8 on 9.2.x 
    Issue #3109767 by quietone, mtodor, Kristen Pol, larowlan: Unable...
  • larowlan committed 30e300f on 9.3.x
    Issue #3109767 by quietone, mtodor, Kristen Pol, larowlan: Unable...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Version: 9.3.x-dev » 9.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 30e300f and pushed to 9.3.x. Thanks!

Backported to 9.2.x

Status: Fixed » Closed (fixed)

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