use dedicated event eventArgs

[yassinefikri]

Resolves #2648

[codecov-commenter]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (3b5b5cb) 79.27% compared to head (cb6be44) 79.40%.

Files	Patch %	Lines
src/Mapping/Event/Adapter/ORM.php	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2649      +/-   ##
==========================================
+ Coverage   79.27%   79.40%   +0.13%     
==========================================
  Files         162      162              
  Lines        8467     8494      +27     
==========================================
+ Hits         6712     6745      +33     
+ Misses       1755     1749       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[franmomu]

After this change, looks like EventInterface::createLifecycleEventArgsInstance is not used anymore (if so, we should deprecate it):

https://github.com/search?q=repo%3Adoctrine-extensions%2FDoctrineExtensions%20createLifecycleEventArgsInstance&type=code

The problem I see is that we would still triggering deprecations when creating the ORM event, but I don't know if that's avoidable (given that we should still extend from the ORM LifecycleEventArgs to keep BC).

[yassinefikri]

I moved the createPreSoftDeleteEventArgs() and createPostSoftDeleteEventArgs() to the ORM/ODM classes as they feel more appropriate to have them (alongside with the createLifecycleEventArgsInstance())

i also deprecated the function createLifecycleEventArgsInstance()

i'm not sure if the deprecations triggered at LifecycleEventArgs construction are avoidable

[franmomu]

I've rebased since there were some conflicts with the latest changes.

[franmomu]

I've left some comments, please add some tests.

Updated: I've made those changes in a new commit, tests still need to be added.

[yassinefikri]

i've updated tests, let me know if i'm missing something

[franmomu]

I wanted to check if we can somehow avoid the LifecycleEventArgs deprecation, I was thinking that we can do a couple of things.

First of all, check with EventManager::hasListeners() method if there are listeners before creating the events, this way we prevent triggering the deprecation for those who don't have listeners.

And to finally remove the deprecation... I was thinking about creating PreSoftDeleteEventArgs and PostSoftDeleteEventArgs for ORM extending from the LifecycleEventArgs class from persistence and adding the getEntity() and getEntityManager() methods (trigger deprecations inside these methods).

That would create a BC break for someone with a listener typehinted with LifecycleEventArgs from the ORM, in that case we can maybe with Reflection check the listener if it's typehinted and in that case trigger a deprecation telling to change it to the dedicated event and for creating the event we would use AdapterInterface::createLifecycleEventArgsInstance().

If the listener is not typehinted with LifecycleEventArgs from the ORM, then we create the new dedicated event. If the listener is calling getEntity() or getEntityManager() would work, the only problem is if it has instance check with LifecycleEventArgs from the ORM, but I guess that's unlikely to happen.

These are some thoughts that I haven't tried and I don't know if that would work.

[yassinefikri]

And to finally remove the deprecation... I was thinking about creating PreSoftDeleteEventArgs and PostSoftDeleteEventArgs for ORM extending from the LifecycleEventArgs class from persistence and adding the getEntity() and getEntityManager() methods (trigger deprecations inside these methods).

i don't get what deprecations should be triggered inside the methods, can you provide an example ?

That would create a BC break for someone with a listener typehinted with LifecycleEventArgs from the ORM, in that case we can maybe with Reflection check the listener if it's typehinted and in that case trigger a deprecation telling to change it to the dedicated event and for creating the event we would use AdapterInterface::createLifecycleEventArgsInstance().

i think it should work but it's gonna get a little more complex to handle cases where there are multiple listeners on the same event

[franmomu]

@mbabker what about this? this is similar to #2725 and maintain a BC layer. When not all the listeners have type-hinted the new event class, it dispatches the old event.

Given that this is Softdeleteable behavior I think it is important to try not break BC.

And to finally remove the deprecation... I was thinking about creating PreSoftDeleteEventArgs and PostSoftDeleteEventArgs for ORM extending from the LifecycleEventArgs class from persistence and adding the getEntity() and getEntityManager() methods (trigger deprecations inside these methods).

i don't get what deprecations should be triggered inside the methods, can you provide an example ?

Sorry, I took another approach (adding a new commit), simplifying events and trying to maintain BC.

[mbabker]

@mbabker what about this?

Looks fine to me.

[franmomu]

thanks @yassinefikri!