[EventDispatcher] Remove callable parameter type #42319
Conversation
| Q | A |
|---|---|
| Branch? | 5.3 |
| Bug fix? | yes |
| New feature? | no |
| Deprecations? | no |
| Tickets | Fix #42283 |
| License | MIT |
| Doc PR | N/A |
Signed-off-by: Alexander M. Turek <me@derrabus.de>
| @@ -25,10 +25,11 @@ interface EventDispatcherInterface extends ContractsEventDispatcherInterface | |||
| /** | |||
| * Adds an event listener that listens on the specified events. | |||
| * | |||
| * @param int $priority The higher this value, the earlier an event | |||
| * listener will be triggered in the chain (defaults to 0) | |||
| * @param callable $listener The listener | |||
There was a problem hiding this comment.
The type actually accepted by our implementations is callable|array{\Closure, string}. Since the array with the service closure is a somewhat internal functionality, I'm unsure if we want to document it.
There was a problem hiding this comment.
what about compiling a compatibile closure instead?
There was a problem hiding this comment.
I don't think the DI component can do that. Apart from that: sure, that would be the nicer solution, but that wouldn't qualify as a bugfix, I guess. 🙂
There was a problem hiding this comment.
then im in favor of documenting it, so we dont make this mistake again 👍
|
@derrabus Can you add a test to be sure we don't have a regression? |
I don't think I can. This affects the interface only. All of our own implementations don't have the parameter type and they should already have tests that cover the use-case. |
|
I don't understand what this fixes: DI wiring (including decoration) always happens at the implementation level. Wiring doesn't happen against abstractions. |
|
The problem is that the interface defines a contract that our own calling code violates. |
|
Which violation? The implementation follows LSP rules, the contract is respected to me. This reminds me about #42013. There is nothing to change on our side here. At least we should not change an abstraction like it's a minor thing. |
But we kind-of did that already with that types backport mentioned in the linked ticket. All I'm suggesting is to take that change back for this interface. |
|
RegisterListenersPass registers calls to addListener() that are incompatible with the signature of the interface True, but that's an issue with the decorator, not with the pass. All the DI wiring layer works only by convention. No compiler passes work by contract. None of them are bound by any contracts. In this example, You can maybe understand why by thinking about what happens the day we add more pseudo-callable variants to one of our implementations (as we did before btw). Quite obviously, that hypothetical change shouldn't trigger a change in the interface, since that'd be a BC break.
I'm not sure we did: the pseudo-callable we use aren't part of the abstraction to me. They're part of the implementation, which we added because we needed a more capable type of pseudo-callables that brings better bootstrap performance. That's not something for the interface/abstraction to me. The linked issue describes exactly what we encountered in #42013: decorators must use wider types as argument. The reason is that LSP allows any implementation to accept wider types. If a decorator wants to be able to decorate any implementation, it has to also accept wider types. I'm 👎 with this PR. At least I'm 👎 with the reasoning that currently justifies it. |
|
I understand why you oppose this PR and I'm okay with scrapping it. Maybe we can find a way to get rid of those pseudo-callables instead. 🤔 |
It's quite possible that recent PHP versions have performance characteristics (thanks to eg preloading) that would allow us to use closures instead. Could be worth some benchmarks since that could allow simplifying our codebase. I'm closing here meanwhile. |
|
I have hacked DI and ED to produce a real closure instead of that pseudo-callable: function (...$args) {
static $closure;
$closure ?? $closure = \Closure::fromCallable([
/* service initialization code */,
/* method name */
]);
return $closure(...$args);
}For a very simple listener, that code is a lot slower than our current pseudo-callable way. 😞 |