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

[2.x] ArgumentCountError thrown when passing $attributes array to closure in event #628

Closed NeuralClone closed 2 weeks ago

NeuralClone commented 2 weeks ago
Q A
Symfony Version 6.4.8
Foundry Version 2.0.1
PHP Version 8.3.8
PHPUnit Version 9.6.19

I've been upgrading my project to use Foundry v2.0. Everything has gone relatively smoothly except updating an event in one of my factory classes. I suspect the problem is on my end and not a bug, but I'm not sure what the problem is.

I'm using Foundry's default bundle configuration.

In a test I create an object as follows:

$object = SomeEntityFactory::createOne([
    // standard entity properties

    // extra property
    'account' => [
        'name' => 'Account name',
        'uid' => 'UID',
    ],
]);

In 1.x, I did this:

final class SomeEntityFactory extends ModelFactory
{
    // ...

    protected function initialize(): self
    {
        return $this
            ->instantiateWith((new Instantiator())->allowExtraAttributes(['account']))
            ->afterPersist(function (SomeEntity $object, array $attributes) {
                // do some stuff with $attributes['account']
            });
    }

    // ...
}

And for 2.x, I currently have this:

final class SomeEntityFactory extends PersistentProxyObjectFactory
{
    // ...

    protected function initialize(): static
    {
        return $this
            ->instantiateWith(Instantiator::withConstructor()->allowExtra('account'))
            ->afterPersist(function (SomeEntity $object, array $attributes) {
                // do some stuff with $attributes['account']
            });
    }

    // ...
}

The latter causes an error in 2.x:

ArgumentCountError : Too few arguments to function App\Tests\Factory\SomeEntity\SomeEntityFactory::App\Tests\Factory\SomeEntity{closure}(), 1 passed in /path-to-project/vendor/zenstruck/foundry/src/Persistence/PersistentObjectFactory.php on line 230 and exactly 2 expected

PHPStan gives the following error:

phpstan: Parameter # 1 $callback of method Zenstruck\Foundry\Persistence\PersistentObjectFactory<App\Entity\SomeEntity&Zenstruck\Foundry\Persistence\Proxy<App\Entity\SomeEntityt>>::afterPersist() expects callable(App\Entity\SomeEntity&Zenstruck\Foundry\Persistence\Proxy<App\Entity\SomeEntity>): void, Closure(App\Entity\SomeEntity, array): void given

Parameter # 2 $attributes of passed callable is required but accepting callable does not have that parameter. It will be called without it.

I've gone over the documentation and upgrade guide to see what I might be doing wrong. I've attempted to debug the issue and that's only muddied things further.

I feel like I'm missing something obvious. What would be the correct way to achieve the above in 2.x?

Thank you!

NeuralClone commented 2 weeks ago

I took another look at the afterPersist function signature and it appears the callable no longer accepts an array of attributes:

https://github.com/zenstruck/foundry/blob/17e5e4f0660e59e281b069593aa3b3578c8e056a/src/Persistence/PersistentObjectFactory.php#L256-L259

That seems to contradict the documentation:

https://github.com/zenstruck/foundry/blob/2.x/docs/index.rst#events--hooks

Is this no longer possible?

nikophil commented 2 weeks ago

Hi @NeuralClone

thanks for reporting this. This is a bug in 2.x, let me fix this

NeuralClone commented 2 weeks ago

Thank you for such a prompt response! It's a bit of a relief that this is a bug and not an intended change.

I figured out a workaround to get everything functional again in case it was intended. But I'm glad I won't have to use it long-term.

nikophil commented 2 weeks ago

curiosity question: using afterInstantiate() insteadof afterPersist() is not a choice for you?

NeuralClone commented 2 weeks ago

It's a good question. Doing it that way is my preference and I've actually been using that approach with other entities.

I ran into a cascade on persist issue with a third party entity that's already configured. It ended up being easier to persist the main object, and then conditionally create and add a different entity to its collection using the extra attribute.

My workaround for this bug was to move the afterPersist() code directly to the tests that need it and therefore not use events at all in the factory in question.

There's probably a better way to do this, but I haven't been able to find the time to properly dig into Foundry's capabilities to address it.

nikophil commented 2 weeks ago

maybe you could give a try in foundry 2, handling of the cascading stuff has been revamped. And if some problems still occurs in that area, we'll try to fix them

NeuralClone commented 2 weeks ago

Upgrading to v2.0 is definitely as good a time as any for me to re-examine this. My current approach has felt kind of like a hack since I implemented it. So I'll definitely be giving it another look now that so much has been revamped.

I really appreciate all the work that's gone into Foundry. Aside from this one issue when upgrading, using it has been incredibly smooth.

nikophil commented 2 weeks ago

thanks for the kind words!

don't hesitate to post a new issue (with a reproducer :pray: ) if you see other problems with cascading stuff

kbond commented 2 weeks ago

Yes, cascade is complex and since I never use it in my projects, I admit, I'm flying dark when working on it in foundry.