feature(doctrine events): allow factories to disable doctrine events.

[CharloMez]

feature(doctrine events): allow factories to disable doctrine events before creation. (issue #215)

[kbond]

Thanks for working on this. Don't worry about the CS issues - I can fix before merging. Let me know if you need help with the test suite - it's a bit... complicated.

[CharloMez]

@kbond actually I tried to build a docker environment to make tests works in local env, I partially succeed (I can't run functional tests).
So it's almost a blind devlopment.
Do you have something to help me on it ? Maybe a docker-compose with Dockerfile files ? Makefile ? .env ?

[CharloMez]

I set up a docker environment with php/pqsql/nginx, I configured it well, the DATABASE_URL is well set in the container, but I still don't succeed to run your test suite. I always have this error
Error running "doctrine:database:drop": in /var/www/src/Test/DatabaseResetter.php:123
I miss something but I don't understand what.

Except that, I can see that my functional tests doesn't work on the CI, my doctrine EventSubscriber fixture is not executed.

[kbond]

Unfortunately, I don't have docker experience. I run everything locally. I was going to suggest doing what you did but am unsure why you're getting that error. Any more context? That error should output the command output.

[kbond]

For the test event subscriber: try having it implement Doctrine\Bundle\DoctrineBundle\EventSubscriber\EventSubscriberInterface. It doesn't look like Doctrine\Common\EventSubscriber can be autowired.

[kbond]

Regarding running the tests locally, could https://github.com/nektos/act help? I haven't used it myself but see it referenced quite a bit.

[CharloMez]

ok finally I succeed to make it run locally (with my docker setup), and it works locally with a php8.

[kbond]

Great to hear. I'd like to make the test suite easier to run for contributors. Is there anything you can share that could help? Are there other/bundles libraries you know of that I can use as inspiration?

[CharloMez]

@kbond finally CI is ok (except scrutinizer, I don't know why, everything look green).
I will try to share my docker-compose environment after some cleaning

[kbond]

Just a few comments:

1. Relation factory's created during create() would currently have events called even if disabled by the parent. There are certain scenarios where they would be disabled though (ie if cascade: persist).
2. The afterPersist hook is called with disabled events re-enabled.

Are the above two things intentional or what are your thoughts on these?

If a user has chosen to disable events, I'm wondering if all of create should have events disabled. WDTY?

[kbond]

Just an idea but what do you think of this API:

Factory::withoutDoctrineEvents(function() {
    PostFactory::createOne(); // events disabled
    PostFactory::createOne(); // events disabled
});

PostFactory::createOne(); // events enabled

Can't disable on a factory-by-factory basis (ie you couldn't say "I always want this model factory created without events") but could solve some of the inconsistencies detailed above.

[CharloMez]

Just a few comments:

1. Relation factory's created during create() would currently have events called even if disabled by the parent. There are certain scenarios where they would be disabled though (ie if cascade: persist).
2. The afterPersist hook is called with disabled events re-enabled.

Are the above two things intentional or what are your thoughts on these?

If a user has chosen to disable events, I'm wondering if all of create should have events disabled. WDTY?

No it was not intentional, I didn't really thought about it. Actually I thought to do it like the withoutPersisting() functionality.

I think the functionality is good like that for some reasons:

• I feel like it is same as withoutPersisting functionality, and different way to use them will loose a little the user
• In this way, if the user want all the process to be without events he can do it (he explicite withoutDoctrineEvents on all factory). But if we force all the process to be without events, the user can do nothing if he just want the first entity without events.
• It's personal but I don't like much this kind of declaration
• And finally we can still correct anytime the feature if the community have troubles to use it like that

[kbond]

I feel like it is same as withoutPersisting functionality, and different way to use them will loose a little the user

FYI, withoutPersisting is "passed" down to sub-factory's.

It's personal but I don't like much this kind of declaration

Is this what you're referring to?

And finally we can still correct anytime the feature if the community have troubles to use it like that

If we merge as-is, then decide to disable events for the entire create, this would be a BC break. That being said, going from "all create", to just "save" would probably be more troublesome. There is currently an avenue to disable events on sub-factory's: set withoutDoctrineEvents() on your sub-factory('s).

[CharloMez]

FYI, withoutPersisting is "passed" down to sub-factory's

Ah ok I didn't notice, my bad. But when I said that, I thought more about different way to declare it in tests

Is this what you're referring to?

Yes

If we merge as-is, then decide to disable events for the entire create, this would be a BC break.

Yeah true...

In this way, if the user want all the process to be without events he can do it (he explicite withoutDoctrineEvents on all factory).
But if we force all the process to be without events, the user can do nothing if he just want the first entity without events.

And what do you think about that ?

You have a much better vision of this, and where does it go, so if you think it's better to follow the withoutDoctrineEvents to child creations, let's do it.

[kbond]

In this way, if the user want all the process to be without events he can do it (he explicite withoutDoctrineEvents on all factory). But if we force all the process to be without events, the user can do nothing if he just want the first entity without events.

I'm a bit torn to be honest, you're right in that we couldn't provide a way to disable just for the primary factory but:

PostFactory::new()->withoutDoctrineEvents()->create();

My initial instinct expects all doctrine events to be disabled during "create".

[CharloMez]

ok let's do it like that (follow the disable to child factories)

[kbond]

Let's sit on this for just a bit. I've asked some other foundry contributors to weigh in.

[kbond]

Ok, yeah, I'm thinking let's disable events for all of create. The solution for allowing sub-factory's to be created with events will be to create them first:

$categroy = CategoryFactory::createOne(); // created with events
PostFactory::new()->withoutDoctrineEvents()->create(['category' => $category]);

WDYT?

[CharloMez]

Ok, yeah, I'm thinking let's disable events for all of create. The solution for allowing sub-factory's to be created with events will be to create them first:

$categroy = CategoryFactory::createOne(); // created with events
PostFactory::new()->withoutDoctrineEvents()->create(['category' => $category]);

WDYT?

It's ok for me 👍
i will try to do it soon. thanks

[CharloMez]

@kbond sorry it took me times to do it...
I figured out that this dev didn't work on listener, so I handle it in the last commit, and I add specific tests for listeners.
Usually I don't like to comment my code, but I feel that this new specific part needed some.
And of course I also add what we said about sub-factories, included tests.

Don't hesitate to tell me if something is not good for you.

[CharloMez]

@kbond I have a repository for a docker-compose environnement on php8/pgsql/nginx, and i made a specific branch for foundry.
https://github.com/CharloMez/php8-nginx-pg-bootstrap_for_symfony/tree/feature/zenstruckFoundryMakefile

Tell me what do you think about it. Feel free to ask me some improvement on it.

[kbond]

Tell me what do you think about it. Feel free to ask me some improvement on it.

I'm not very familiar with docker but a few comments:

1. The test suite doesn't require a web server so probably shouldn't include php-fpm/nginx
2. Should include a mysql container to be able to run test suite on mysql

[kbond]

sorry it took me times to do it...

No worries and no pressure/rush - we're all doing this in our spare time!

The test failures must be related to this PR but I can't see how. I merged a PR a few hours ago that didn't have failures. Maybe a rebase would fix?

[CharloMez]

sorry it took me times to do it...

No worries and no pressure/rush - we're all doing this in our spare time!

The test failures must be related to this PR but I can't see how. I merged a PR a few hours ago that didn't have failures. Maybe a rebase would fix?

I rebased it, but still fail, I don't really understand why...

[CharloMez]

I'm not very familiar with docker but a few comments:

1. The test suite doesn't require a web server so probably shouldn't include php-fpm/nginx
2. Should include a mysql container to be able to run test suite on mysql

yeah true, I will remove nginx for this branch.

It is a very simple docker-compose conf, to be able to execute the tests suite.

You are right, a perfect configuration would be to have all different kind of database and all different php version handled by your project, but doing such environment will require to create a system to be able to switch between all of that.
It require much more reflexion and skills in dockerisation, I can think of it but I'm not sure I have the skill for that.

[kbond]

Small fix.

Since the failing test is happening when running the test suite with dama-doctrine-bundle, I wonder if messing with the events is breaking something. When I have a chance, I'll checkout the code and take a look.

Edit: I think this event is being removed and causing problems. We'll need to somehow only remove ORM events (this might be easier said than done).

[kbond]

Suggested change

	$proxy->save();
	$proxy->withoutAutoRefresh(function(Proxy $proxy) use ($attributes) {
	$proxy->save()->withoutAutoRefresh(function(Proxy $proxy) use ($attributes) {

[CharloMez]

This don't solve the problem

[kbond]

I didn't think it would, this was just a minor thing.

[CharloMez]

Small fix.

Since the failing test is happening when running the test suite with dama-doctrine-bundle, I wonder if messing with the events is breaking something. When I have a chance, I'll checkout the code and take a look.

Edit: I think this event is being removed and causing problems. We'll need to somehow only remove ORM events (this might be easier said than done).

hmm yeah I see. Maybe a more reasonable way to handle this functionality is to give explicitly the subscriber or listener to remove. like:
->withoutDoctrineEvent(CommentEventListener::class)
And allow the user to chain call if needed, like:
->withoutDoctrineEvent(CommentEventListener::class)->withoutDoctrineEvent(CommentEventSubscriber::class)->create()

[kbond]

Maybe a more reasonable way to handle this functionality is to give explicitly the subscriber or listener to remove. like:

I agree, while not ideal, I don't see an easy way to distinguish between ORM and DBAL events.

[kbond]

I'm closing this as stale but do think it's a valid feature. Feel free to reopen!