Consider replacing Prefix/suffix interceptor approach with middleware approach
ostrolucky opened this issue · 4 comments
Current approach of splitting interception to 2 callbacks turned out to be quite limiting. There are some use cases that really requires to wrap the whole call into callable, so that they can measure the time it took for the function to execute. In SncRedisBundle I did manage to workaround this limitation by using references https://github.com/snc/SncRedisBundle/blob/546e4d47f5307b20ed58683bd54eab93a476536a/src/Factory/PhpredisClientFactory.php#L244-L279, but now I have a use case with NewRelic extension newrelic_record_datastore_segment function. Here, I do not have access to modify how timing is measured. Perhaps I could call the wrapped method in prefixInterceptor manually and use earlyReturn? But that's still workaround around dubious design decision. Perhaps something that could be changed in 3.x? What are your thoughts on this topic? Is split needed for some use cases?
Perhaps I could call the wrapped method in prefixInterceptor manually and use earlyReturn?
If you are okay with the responsibility of forwarding all parameters yourself, then earlyReturn allows for exactly this kind of design.
That's how you would achieve an "around" call.
Yep, that's what I did now in https://github.com/snc/SncRedisBundle/pull/645/files#diff-feae5e09fe9b8b0c27dc9409163109aeb8f4a7510774e20240dad704a80c367dR241. But this is still non-obvious and strange API design. Why not using middleware-first approach? Is split needed for some use cases?
Is split needed for some use cases?
It was mostly done to simplify the design in a first implementation: probably outdated thinking by now (more than a decade has passed, and middleware were not as popular in PHP yet), but given that the results can be achieved with very minimal effort, I don't think a full redesign+BC break is required here.
Yes, usage of by-ref for this is potentially confusing, and could perhaps require better type definitions, but I look back at when I wanted to get rid of callable() for explicit object-typed API, and the stability really wins here.
Instead, I would suggest refining the existing callable type definitions with better psalm- or phpstan- types, and perhaps documenting the use-case.
Alright, thanks for answering. Then I think we don't need this issue open anymore.