Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.54%. Comparing base (dadad05) to head (1d5855f).
Report is 1 commits behind head on 6.x.

@@            Coverage Diff            @@
##                6.x   #13842   +/-   ##
=========================================
  Coverage     64.54%   64.54%           
  Complexity    34613    34613           
=========================================
  Files          2271     2271           
  Lines        103440   103440           
=========================================
  Hits          66766    66766           
  Misses        36674    36674           

Unfortunately, changing method signature is considered a BC break so this would have to wait for Mautic 6.

I see the point you're making, but I don't really understand your conclusion for this particular case.

1. The docstring already hinted the return type.
2. There is no other way than using LeadEvent with Lead for $lead as you can only instantiate a new object with a Lead entity and use the setter with a Lead entity.

The way I see that is that all this does is making something explicit that was already there (and enforced in other parts). From my understanding nothing can break because of this change.

I don't want to argue—I'm genuinely interested.

But when we are in improving this class, is there any reason for &$lead? Can we drop it? […]

I have no idea. 😅

Edit: I guess, this was necessary as $lead might get changed outside the event and, thus, the event listeners? It probably is necessary to keep the object in sync?

Can the class be final?

Why would that be preferable? Couldn't have a plugin or custom integration have the need to extend LeadEvent to pass more/custom data to the lead events?

What's the benefit for the Mautic core if this gets restricted?

How about to use Constructor Property Promotion?

Wasn't sure whether to include this. Did M5 drop support for PHP<v7 completely?

The thing is that all classes should be final by default unless there is a reason not to. For DTO classes like events there is no good reason to extend them. Even in the tests it's easier to use the DTO class rather than mock it. If we'd keep this rule from the beginning, we wouldn't have this BC break to worry about.

Imagine that some plugin is extending this class because we allow it. Then can have their own implementation of the getLead() method in their class. If you add the return type, their plugin will be broken. It's a BC break.

It seems that in this case we cannot use Constructor Property Promotion because the properties are defined in the parent class as the failing tests suggest. Another reason why composition is better than inheritance.

I guess, this was necessary as $lead might get changed outside the event and, thus, the event listeners? It probably is necessary to keep the object in sync?

Objects are always passed as a reference. This would make sense if it would be an array for example, but I don't see any reason for it as an object. The setLead() method doesn't seem to be called anywhere so even overwriting one Lead object with another isn't a suitable use case here. But it's another possible BC break.

Let me know if you have more questions

Thanks for your explanations.

Imagine that some plugin is extending this class because we allow it. Then can have their own implementation of the getLead() method in their class. If you add the return type, their plugin will be broken. It's a BC break.

The code documents that the return type should be a Lead object. If someone deviates from the core I'd say it's their fault if they get hit by a change like this. For anyone using/extending the class like it is intended this isn't a BC¹.

Am I mistaken, here?

What I mean by extending the class is that a plugin might add another method to use this event to transport additional data. This wouldn't be possible if the class is final. This could make it harder to leverage the LeadEvents for a plugin.

So, by extending I don't mean to override existing methods in a way that one completely deviates from the core (even though this is possible).

The code documents that the return type should be a Lead object. If someone deviates from the core I'd say it's their fault if they get hit by a change like this. For anyone using/extending the class like it is intended this isn't a BC

Well, try to do that. If the method in the parent class has a return type and the method in the child class does not then you get an error. That's what this BC change is about. Not that some plugin could expect some other object than Lead.

What I mean by extending the class is that a plugin might add another method to use this event to transport additional data. This wouldn't be possible if the class is final. This could make it harder to leverage the LeadEvents for a plugin.

Why would anyone extend the event class? When the event is dispatched in the core then a new instance of the event class is created. If you extend it, how would you enforce your event class being dispatched instead of the original?

Ideally, the event class should be unique to each event being dispatched. This way we could throw away the event keys and dispatch just the event objects. That is how it's recommended in the newer Symfony versions.

So I still don't see any good use case why the event classes should not be final.

Hi folks, do you think we could wrap this up over the next weeks so we can merge it in 6.0 alpha?

Hi @putzwasser another one here which is currently tagged for 6.0 - if we can't get it ready by then it'll have to be pushed back to 7.0 (alpha this quarter, but the General Availability release isn't until Q4).

Bumping to Release Candidate due to need for rebase.