8000 [EventDispatcher] Remove callable parameter type by derrabus · Pull Request #42319 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[EventDispatcher] Remove callable parameter type #42319

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

derrabus
Copy link
Member
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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor
8000

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about compiling a compatibile closure instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 🙂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then im in favor of documenting it, so we dont make this mistake again 👍

@fabpot
Copy link
Member
fabpot commented Aug 6, 2021

@derrabus Can you add a test to be sure we don't have a regression?

@derrabus
Copy link
Member Author
derrabus commented Aug 6, 2021

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.

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Aug 6, 2021

I don't understand what this fixes: DI wiring (including decoration) always happens at the implementation level. Wiring doesn't happen against abstractions.

@derrabus
Copy link
Member Author
derrabus commented Aug 6, 2021

The problem is that the interface defines a contract that our own calling code violates.

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Aug 6, 2021

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.

@derrabus
Copy link
Member Author
derrabus commented Aug 6, 2021

Which violation? The implementation follows LSP rules, the contract is respected to me.

RegisterListenersPass registers calls to addListener() that are incompatible with the signature of the interface.

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.

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Aug 8, 2021

RegisterListenersPass registers calls to addListener() that are incompatible with the 8000 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, EventDispatcherInterface is not mentioned at all in RegisterListenersPass, and it doesn't have to.

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.

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.

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.

@derrabus
Copy link
Member Author
derrabus commented Aug 8, 2021

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. 🤔

@nicolas-grekas
Copy link
Member

Maybe we can find a way to get rid of those pseudo-callables instead. thinking

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.

@derrabus derrabus deleted the bugfix/revert-callable-type branch August 8, 2021 11:53
@derrabus
Copy link
Member Author
derrabus commented Aug 8, 2021

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. 😞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
0