Introduce client specific events #284
Conversation
rpkamp
commented
Dec 7, 2019
rpkamp
commented
| Q | A |
|---|---|
| Bug fix | no |
| New feature | yes |
| BC breaks | no |
| Deprecations | yes |
| Tests pass | yes |
| Fixed tickets | #283 |
| License | MIT |
There will be a lot of tests added to this class over the following commits, and it would benefit the readability of the class as a whole if it is cleaned up a bit.
Instead of listening to all pre transaction events and filtering for a desired service name, clients can not also listen for events from specific clients.
Instead of listening to all post transaction events and filtering for a desired service name, clients can not also listen for events from specific clients.
That we way we get BC support in the EventDisptachMiddleware for free
|
Hello @rpkamp First, thanks for PR :) but, let's do some small changes. You mixed here deprecations and new way of events, but in master we accumulate changes just for v8. These changes will not enter in v7. I propose to create 2 PRs:
Is it ok for you? Thanks, Vlad. |
|
I think Symfony normally already adds the new way of doing something, while deprecating the old thing, and then remove the old thing in the new major. The nice thing about this is that you can already update the new thing in the current major and then for the new major you don't have to do anything anymore. For this example this would mean that this PR goes against v7, and then in another PR the deprecations will be removed for v8. The advantage is that there is a clean upgrade path from v7 to v8. If we only deprecate in v7, then once you upgrade to v8 your code is temporarily broken until you fix it. If v7 already supports the new way you can get rid of deprecations there and know it will work in v8. Up to you of course, just offering an alternative 🙂 |
|
@rpkamp I got your idea. If you want this PR to appear in v7, then change PR to be for v7 branch and not for master branch. All what we have on master will be released as v8. |
|
@gregurco I've completely removed the CompilerPass from this PR, so it ready to be merged into |
|
All done @gregurco |
