[DoctrineBridge][EventDispatcher] Add LazyEventManager and LazyEventDispatcher #20039
Conversation
3552657 to
73f204d
Compare
|
I've refactored it a little bit. The LazyEventManager/Dispatcher is no longer abstract with a method to be implemented. Instead it accepts a callable resolver. The ContainerAwareEventManager/Dispatcher simply wrap the container into a closure. It is cleaner this way in my opinion but I can revert it if desired. EDIT: For reference you can find the original implementation here: https://github.com/enumag/symfony/commits/lazy-3 |
c606588 to
490e8e0
Compare
There was a problem hiding this comment.
I like this version better too... I don't think anything in the docs need updating...
However, splitting the tests for ContainerAwareEventManageras you did for the code. And use them as a basis to write tests for the ContainerAware/LazyEventManager too?
Status: Needs work
91e0d0a to
7812a7b
Compare
|
@lemoinem Done, please review. Status: Needs Review |
|
@enumag Could you rename the Lazy* Tests to mention LazyListener instead of Services? Status: Needs work |
|
@lemoinem There is nothing lazy about the listeners, just the evm/evd is lazy. So I'll just rename |
|
Yes, but it is lazyily loaded. My point is, tests for the lazy evm/evd do not deal with services but lazily built/loaded listeners, so I think the test names should reflect that. |
|
@lemoinem Like this? EDIT: If your're talking about methods names they are named after the methods they are supposted to test. Status: Needs Review |
|
Yes, I noticed this afterward. Since the Lazy evm/evd only takes a closure to lazily load the listeners, I don't think it should refer to services at all... What do you think? |
|
@lemoinem I agree but since there is "service" in the method names I can't change it because of BC. |
|
Why not introduce new methods in the new class and keep (depreciate?) the current methods in the current class? The other way around would be to introduce an interface similar to PSR11's ContainerInterface within the component and make Symfony's ContainerInterface extend it... But I'm not sure it would still solve your problem... |
|
I don't think introducing new methods is a good idea. I mean even in my case I'll still use DI container with services, just a different implementation. PSR11 unfortunately isn't acccepted yet and creating something similar doesn't have a chance to be merged. |
|
OK, Then I guess keeping the current code will be good enough... Thanks. |
|
ping @fabpot |
|
We are in a code freeze, so any new feature will be consider after the release of 3.2. |
|
@fabpot There is a separate 3.2 branch now. I'm guessing that fulfills the requirement? |
|
I'm quite -1 for adding the integration between nette/di and doctrine/common in our doctrine bridge. Our bridges are meant to provide integration between Symfony and external libraries, not between 2 libraries outside Symfony. If you want to have a shared LazyEventManager somewhere, you should submit it to doctrine/common, not to Symfony. |
|
@stof That's a good point! Yeah I should try and push it to doctrine/common. Here are some questions for you:
|
|
For the LazyEventDispatcher, WDYT of #20875 instead? |
|
Closing as we are working on a more generic way to solve this problem as mentioned by @nicolas-grekas |
…t type (nicolas-grekas) This PR was merged into the 3.3-dev branch. Discussion ---------- [DI][EventDispatcher] Add & wire closure-proxy argument type | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #12521, #12007, #20039 | License | MIT | Doc PR | - By resolving event subscribers at compile time, then wrapping listeners in a closure, we get laziness almost for free. This should solve/replaced all the above linked issues. (WIP because tests are missing) Commits ------- ecdf857 [DI][EventDispatcher] Add & wire closure-proxy argument type
|
@enumag Make sure to keep the author's intact and you can propose this to the Doctrine project 👍 both are MIT licensed, so this shouldn't be a problem. Make sure to link this pull request so they can decide if this acceptable. |
|
@sstok Thanks, I'll look into it when I have some time. |
My application does not use the full symfony framework, just some parts. Among them the DoctrineBridge and EventDispatcher. I'd like to have the subscribers loaded lazily which symfony can do but the classes require a Container from Symfony\DependencyInjection - which I don't use. I'm using a different DI container, namely nette/di.
The logic of ContainerAwareEventManager/Dispatcher is not really dependent on symfony/dependency-injection. In fact the container is only used once in the whole class. Therefore I'd like to split the classes like this so that I don't have to copy the lazy-loading logic when using a different DI container.