Let the user build its own randomizer engine to fix the seed and get reproductible results.#83
Let the user build its own randomizer engine to fix the seed and get reproductible results.#83sukei wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis PR introduces an optional ChangesEngine injection and propagation
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant Faker
participant Container
participant Provider
participant HasExtensions
participant Extension
Faker->>Container: new Container(engine)
Container->>Container: store engine, getEngine()
Faker->>Container: __call(method, args)
Container->>Provider: extensions()
Provider->>Container: resolveExtensions(extensions, getEngine())
Container->>HasExtensions: resolve(extension, engine)
HasExtensions->>Extension: new Extension(new Randomizer(engine))
Extension-->>Container: extension instance ready
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…reproductible results.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Container/Container.php (1)
61-74: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftReset the static extension cache when the engine changes
Container::__construct()only boots and resolves extensions whilestatic::$extensionsis empty, so the firstContainercreated in a process fixes theRandomizerused by every later extension instance. That makesFaker::__call()reuse the first engine’s extensions even when a different$engineis passed, which breaks concurrent seededFakerinstances. Consider invalidating the static cache on engine changes or avoiding the static extension state here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Container/Container.php` around lines 61 - 74, The Container constructor is reusing a static extension cache across different Engine instances, so later Faker calls can keep the first engine’s Randomizer and extensions. Update Container::__construct(), areExtensionsInitialized(), and the static::$extensions handling so the cache is invalidated or rebuilt whenever a different $engine is passed, or remove the shared static state from extension bootstrapping entirely. Make sure Faker::__call() and extension initialization always reflect the current engine instead of the first one created in the process.
🧹 Nitpick comments (3)
tests/Unit/Extensions/HashExtensionTest.php (1)
14-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSeedability tests only prove same-seed reproducibility, not seed sensitivity.
Each
*IsSeedabletest compares twocreateFresh()calls, which both hardcodeMt19937(19937)(perTestCase::createFresh()), so equality is guaranteed regardless of whether the engine is actually wired through correctly to the hash extension's randomizer. A complementary assertion that two different seeds produce different output would more directly validate the new seeding feature end-to-end.Also applies to: 29-35, 44-50, 59-65
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/Extensions/HashExtensionTest.php` around lines 14 - 20, The seedability tests in HashExtensionTest only verify that two identical createFresh() calls match, so they do not prove the seed is actually affecting the hash extension. Update the *IsSeedable tests that call createFresh()->sha1(), ->sha256(), ->sha384(), and ->sha512() to also compare outputs from two different seeds and assert they differ, using TestCase::createFresh() and the relevant hash methods to confirm the seed is wired through end-to-end.tests/Unit/Extensions/TestCase.php (1)
19-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFixed-seed
$this->fakerweakens fuzz-style assertions across the suite.
$this->faker(the shared instance used across allExtensiontest subclasses) is now always constructed with the same hardcodedMt19937(19937)seed, both insetUp()andcreateFresh(). Any test that still exercises$this->fakerin a loop to fuzz-check output formats (e.g.InternetExtensionTest::testUnsecureUrl/testSecureUrl, which weren't migrated to a local unseededContainer) will now generate the exact same sequence of values on every run, forever. This removes the randomized-input coverage those loops were designed to provide — a latent bug only triggered by a value outside seed 19937's sequence would never be caught by CI.Consider either:
- Using an unseeded engine (
null) for the shared$this->fakerby default, and only seeding explicitly in tests that need reproducibility (as done for mostInternetExtensionTestmethods), or- Varying the seed per test run (e.g., from a random value logged on failure) to retain both reproducibility-on-failure and broad fuzz coverage over time.
Illustrative fix: keep shared faker unseeded, opt-in to seeding where needed
protected function setUp(): void { parent::setUp(); (new FakerServiceProvider())->boot(); - $this->faker = new Container(new Mt19937(19937), false); + $this->faker = new Container(null, false); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/Extensions/TestCase.php` around lines 19 - 31, The shared faker in TestCase is being created with a fixed Mt19937(19937) seed in both setUp() and createFresh(), which removes fuzz coverage for subclasses like InternetExtensionTest that still rely on $this->faker. Update TestCase so the default shared Container is unseeded or otherwise varies per run, and keep explicit seeding only in tests that need deterministic output. Make sure createFresh() and the setUp() initialization stay aligned so subclasses continue to get the intended faker behavior.src/Container/Traits/HasModifiers.php (1)
28-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer the public
getEngine()accessor over the raw property.
HasModifiersimplicitly relies on the host class declaring a$engineproperty with that exact name/visibility. SinceContainer::getEngine()was just added for this purpose, use it here instead of reaching directly into$this->engine, decoupling the trait from the host's internal property layout.♻️ Proposed fix
- $this->modifiers[] = new NullableModifier(new Randomizer($this->engine), $weight); + $this->modifiers[] = new NullableModifier(new Randomizer($this->getEngine()), $weight);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Container/Traits/HasModifiers.php` around lines 28 - 33, The HasModifiers::nullable method is reaching directly into the host’s $engine property instead of using the public accessor. Update nullable() to obtain the engine via getEngine() when creating the Randomizer, so the trait depends on Container::getEngine() rather than a specific property layout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Container/Container.php`:
- Around line 61-64: The Container constructor parameter order was changed in a
way that breaks positional callers, so restore the public API by keeping the
boolean parameter first and the Engine parameter last in Container::__construct.
Update the constructor signature and any internal usage so
$shouldBuildContainerMixin remains the first argument and $engine is appended
afterward, preserving existing calls like new Container(false).
In `@src/Extensions/Extension.php`:
- Around line 12-15: The final constructor on Extension blocks third-party
subclasses that override __construct(), so update Extension::__construct to
allow inheritance while preserving the Randomizer injection contract used by
resolve(). If subclassing is intended as public API, also add the appropriate
semver-major/changelog note and ensure any extension instantiation path remains
compatible with existing subclass constructors.
---
Outside diff comments:
In `@src/Container/Container.php`:
- Around line 61-74: The Container constructor is reusing a static extension
cache across different Engine instances, so later Faker calls can keep the first
engine’s Randomizer and extensions. Update Container::__construct(),
areExtensionsInitialized(), and the static::$extensions handling so the cache is
invalidated or rebuilt whenever a different $engine is passed, or remove the
shared static state from extension bootstrapping entirely. Make sure
Faker::__call() and extension initialization always reflect the current engine
instead of the first one created in the process.
---
Nitpick comments:
In `@src/Container/Traits/HasModifiers.php`:
- Around line 28-33: The HasModifiers::nullable method is reaching directly into
the host’s $engine property instead of using the public accessor. Update
nullable() to obtain the engine via getEngine() when creating the Randomizer, so
the trait depends on Container::getEngine() rather than a specific property
layout.
In `@tests/Unit/Extensions/HashExtensionTest.php`:
- Around line 14-20: The seedability tests in HashExtensionTest only verify that
two identical createFresh() calls match, so they do not prove the seed is
actually affecting the hash extension. Update the *IsSeedable tests that call
createFresh()->sha1(), ->sha256(), ->sha384(), and ->sha512() to also compare
outputs from two different seeds and assert they differ, using
TestCase::createFresh() and the relevant hash methods to confirm the seed is
wired through end-to-end.
In `@tests/Unit/Extensions/TestCase.php`:
- Around line 19-31: The shared faker in TestCase is being created with a fixed
Mt19937(19937) seed in both setUp() and createFresh(), which removes fuzz
coverage for subclasses like InternetExtensionTest that still rely on
$this->faker. Update TestCase so the default shared Container is unseeded or
otherwise varies per run, and keep explicit seeding only in tests that need
deterministic output. Make sure createFresh() and the setUp() initialization
stay aligned so subclasses continue to get the intended faker behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b6a98295-6a3f-441c-8065-61cb43f831a1
📒 Files selected for processing (10)
src/Container/Container.phpsrc/Container/Traits/HasExtensions.phpsrc/Container/Traits/HasModifiers.phpsrc/Extensions/Extension.phpsrc/Faker.phpsrc/Providers/Provider.phptests/Support/Extensions/NumberTestExtension.phptests/Unit/Extensions/HashExtensionTest.phptests/Unit/Extensions/InternetExtensionTest.phptests/Unit/Extensions/TestCase.php
nikophil
left a comment
nikophil
left a comment
There was a problem hiding this comment.
Hi!
I was currently opening an issue in this way! (see zenstruck/foundry#1128)
Don't you think this should also be useful to reset the engine? maybe in order to reset the seed?
maybe instead of giving an instance of Random\Engine to the Container, let's pass a callable(): Random\Engine?
so we can easily reset it?
| protected Randomizer $randomizer; | ||
|
|
||
| public function __construct(Randomizer $randomizer) | ||
| final public function __construct(Randomizer $randomizer) |
There was a problem hiding this comment.
this creates a BC break, given that HasExtenisons trait allows to pass extensions instances, this seems needed, but that would prevent any service injection. What if we want an extension that uses a shared clock for example?
There was a problem hiding this comment.
I discussed this point with coderabbitai. The current implementation is doing new $extension(new Randomizer()) calls. So the contract have to be fixed in order to work reliably. That being said, with a fresh redesign of that part we could move toward custom constructors.
There was a problem hiding this comment.
I understand why you did this, but it can breaks some extensions if someone modified the constructor. I need to talk about this with @GautierDele too
I would prefer having isolated faker instances with their own settings instead of resetting existing ones. Thats why I propose to refactor the static nature of the library. |
yeah I understand, and totally agree, but it seems odd to re-instantiate all the extensions when we only need to reset the randomizer |
If we are looking for performance optimization, I think we could instantiate them lazily at call time. There is already an algorithm that collect method names to build the mixin. We could generate a map of |
|
Actually, we're trying to find a way to allow to use this package in Foundry (see zenstruck/foundry#1128) and being able to call |
I understand your use-case, you try to align with FakerPHP. Using a proper DI, we could have a Note that I'm nobody here, I just want to replace my own usage of FakerPHP for something more actual. I'm just trying to improve the current implementation to fit my needs, but it seems to be a lot of work. |
|
Hi, I will take some time during this week for this subject :) |
|
Well, I also would prefer the ->resetSeed() function because it seems easier to use with frameworks. But this can also return a new Faker instance. We need to do more tests with the Laravel and Symfony integrations, and keep in memory the changes it can cause to Foundry |
|
I frankly think that a shared |
|
I think that we should create our own Randomizer class (that may wrap the native one) but that can expose more convenient methods such as the |
3 participants
Sometimes, its useful to fix a random seed to get reproductible results. However, its not possible yet. This PR tries to address this issue.
I would also love to discuss the static nature of things because it is harder to have two Faker instances at the same time. Its not that much desirable, but force the consumer to do weird reset things to fix the seed. This can be seen on the extension TestCase I changed for this purpose.
If this PR goes further, I will gladly make a PR to the documentation project too.
Summary by CodeRabbit
New Features
Bug Fixes