Skip to content

[11.x] Chore: Decouple Str::random() from Validator #56852

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Sep 2, 2025
Merged

[11.x] Chore: Decouple Str::random() from Validator #56852

merged 3 commits into from
Sep 2, 2025

Conversation

michaeldyrynda
Copy link
Contributor

@michaeldyrynda michaeldyrynda commented Sep 1, 2025 •
edited
Loading

The use of Str::random() within the Validator can lead to non-deterministic test outcomes when using random string mocking such as Str::createRandomStringsUsing() or Str::createRandomStringsUsingSequence().

For example, with a test file that contains a set of tests as follows:

test('the thing is authorised', function () {
    postJson(...)->assertForbidden();
});

test('the thing was successful', function () {
    Str::createRandomStringsUsingSequence([
        'dot_placeholder', // Account for the form request being resolved via ValidatesWhenResolved
        'some_random_string',
        'another_random_string',
    ]);

    postJson(...)
        ->assertCreated()
        ->assertStreamedContent()
        ->assertJson(fn ($json) => $json
            ->where('data.0.id', 'some_random_string') // True in isolation, dot_placeholder otherwise
            ->where('data.1.id', 'another_random_string') // True in isolation, some_random_string otherwise
            ->etc()
        );
});

When accounting for the call to Str::random() within the Validator class' constructor, the test will pass in isolation i.e. filtering to only run the thing was successful.

If, however, the test file or the full test is run, the thing was successful fails making assertions against the matching random strings due to the Validator::$placeholderHash already being set in a preceding test.

As the $placeholderHash is a static property, the state persists through a single test process, which is why it impacts file, folder, and suite, but not a single test.

This PR adds a flushState method to the Validator class and calls it from the InteractsWithTestCaseLifecycpe trait, inline with similar behaviour elsewhere in the framework, per @timacdonald's suggestion.

@michaeldyrynda michaeldyrynda force-pushed the chore/decouple-str-random-from-validator branch from 3110ff3 to 86f2449 Compare September 1, 2025 02:38
@timacdonald
Copy link
Member

Instead of changing this behaviour, I wonder if we should follow the existing pattern of introducing a static flushState method that then clears the value. We would then update InteractsWithTestCaseLifecycle to call this alongside all the others?

framework/src/Illuminate/Foundation/Testing/Concerns/InteractsWithTestCaseLifecycle.php

Lines 169 to 190 in c1b1628

AboutCommand::flushState();
Artisan::forgetBootstrappers();
Component::flushCache();
Component::forgetComponentsResolver();
Component::forgetFactory();
ConvertEmptyStringsToNull::flushState();
Factory::flushState();
EncodedHtmlString::flushState();
EncryptCookies::flushState();
HandleExceptions::flushState($this);
Markdown::flushState();
Migrator::withoutMigrations([]);
Once::flush();
PreventRequestsDuringMaintenance::flushState();
Queue::createPayloadUsing(null);
RegisterProviders::flushState();
Sleep::fake(false);
TrimStrings::flushState();
TrustProxies::flushState();
TrustHosts::flushState();
ValidateCsrfToken::flushState();
WorkCommand::flushState();

@michaeldyrynda
Copy link
Contributor Author

Instead of changing this behaviour

I prefer your approach, but that (selfishly) won't save me for 11.x as it doesn't exist until 12.x. I suppose it's debatable whether #54845 is a bug to be fixed within the bug fix window or not.

It could be that the inline random is used and merged-forward to 12.x and the InteractsWithTestCaseLifecycle.php adds flushState later, because it probably should do that irrespective of how the $dotPlaceholder is generated.

Moot point but the str_shuffle is probably "faster" than using random_bytes in a while loop.

@timacdonald
Copy link
Member

11.x as it doesn't exist until 12.x

Do you meant the trait or something else? The trait has been there since 10x: https://github.com/laravel/framework/blob/10.x/src/Illuminate/Foundation/Testing/Concerns/InteractsWithTestCaseLifecycle.php#L157-L166

@michaeldyrynda
Copy link
Contributor Author

Oh, I am blind. I was looking in Illuminate\Testing not Illuminate\Foundation\Testing

@michaeldyrynda michaeldyrynda force-pushed the chore/decouple-str-random-from-validator branch from 86f2449 to 178ce52 Compare September 1, 2025 06:04
crynobone added a commit to orchestral/testbench-core that referenced this pull request Sep 1, 2025
@crynobone
[9.x] Supports flushing Validator state
f0fd30d 
PR: laravel/framework#56852

Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
@crynobone crynobone mentioned this pull request Sep 1, 2025
Merged
@taylorotwell taylorotwell merged commit 52a8e25 into laravel:11.x Sep 2, 2025
58 checks passed
@michaeldyrynda michaeldyrynda deleted the chore/decouple-str-random-from-validator branch September 2, 2025 20:29
crynobone added a commit to orchestral/testbench-core that referenced this pull request Sep 3, 2025
@crynobone
[9.x] Supports flushing Validator state (#356)
811497b 
* [9.x] Supports flushing Validator state

PR: laravel/framework#56852

Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>

* wip

Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>

* wip

---------

Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
@AhmedAlaa4611 AhmedAlaa4611 mentioned this pull request Sep 5, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@michaeldyrynda @timacdonald @taylorotwell