search

zenstruck / foundry

A model factory library for creating expressive, auto-completable, on-demand dev/test fixtures with Symfony and Doctrine.
https://symfony.com/bundles/ZenstruckFoundryBundle/current/index.html
MIT License
607 stars 62 forks source link

Configuration instantiates faker generator prematurely #604

Open lmanzke opened 1 month ago

lmanzke commented 1 month ago

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 commented 1 month ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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