Skip to content
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.

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

Configuration instantiates faker generator prematurely #604

Open
lmanzke opened this issue May 11, 2024 · 3 comments
Open

Configuration instantiates faker generator prematurely #604

lmanzke opened this issue May 11, 2024 · 3 comments

Comments

@lmanzke
Copy link

lmanzke commented May 11, 2024

I was recently debugging a problem we had in our code base. I unfortunately cannot share a repo here, but let me still break down what I found out:

We are using zenstruck foundry in conjunction with a faker seed configurable as shown here: https://symfony.com/bundles/ZenstruckFoundryBundle/current/index.html#faker

This correctly instantiates a faker generator instance and seeds it. So far, so good.

However, the class Zenstruck\Foundry\Configuration also instantiates a faker generator on line 55. This is unused by our codebase (the seeded faker is set via setFaker), but unfortunately has nasty side effects:

Throughout the test (in the middle of it, while entities are being persisted), PHP randomly decides to destroy that faker instance, deciding that is no longer needed (I guess it's true, since it has been overridden). This causes the generator's __destruct method to be called that undoes the seeding.

So, while the test is correctly seeded in the beginning, in the middle of it, we get unpredictable values again since an unused faker instance destroys the setup.

Furthermore, it is not easy to override this ourselves, since unfortunately, the Configuration class is marked as internal/final and cannot be extended.

This is a problem and I would like it to be fixed.

My suggestion:
Remove line 55 and only instantiate the generator on demand, for example in getFaker (if the instance is null, instantiate it there). As setFaker is always called it seems though (because of the service definition), I am not even sure this is needed.

Another idea could be not to create a new generator instance when the seed is provided, but getting the already existing generator instance via getFaker on the configuration and seeding that one. This would ensure that the actual generator is not garbage collected as it is still in use.

What do you think?

@kbond
Copy link
Member

kbond commented May 16, 2024

Thanks for the detailed report @lmanzke!

Indeed, the static nature of this code base (and needing to persist objects between kernel reboots) seems to be causing the issue here.

Remove line 55 and only instantiate the generator on demand, for example in getFaker (if the instance is null, instantiate it there). As setFaker is always called it seems though (because of the service definition), I am not even sure this is needed.

This feels like the best/easiest solution, have you confirmed this fixes the issue for you?

@lmanzke
Copy link
Author

lmanzke commented May 31, 2024

Hey @kbond,

Sorry for the late reply, I somehow missed the notification about the mention :).
Yes, for us this worked. What I went with to circumvent is prevent composer from autoloading the foundry Configuration, but instead created my own that does it exactly as described there. That works for us now and the seed values are not reset.
Should I create a PR to apply these changes?

@kbond
Copy link
Member

kbond commented Jun 1, 2024

@lmanzke, no problem Lucas. Yeah, if you could create a PR, that'd be great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants