[Serializer] Revert deprecation of ContextAwareEncoderInterface and ContextAwareDecoderInterface
#47150
Conversation
… `ContextAwareDecoderInterface`
afca338 to
9ac7fb1
Compare
I'm not sure about this given these interfaces were just about surcharging the method signatures with an extra |
|
If we pass a context to supports method, we have to consider that context can be taken into account, aka we have to remove the cache. Adding yet another interface for cacheable supports feels like renaming interface for the sake of it. Until 6.1, supports method were expected to receive only one argument, the format. That made them cacheable. ContextAware interfaces were here to tell that the context could be used, and we forgot to make those instances uncacheable. I feel like we already have everything in place to properly deal with the issue. |
|
(Note that I would go one step further in 6.2 and deprecate passing the |
|
That would be a big code smell to me, really.
Caching support is a different feature, making it explicit would not be a bad thing imho.
But we did that change for a reason, mainly usability. Pretty much all implementations want the context (not always for caching, I'd even say it's the minority), and they end up implementing those interfaces for that just because we weren't able to add a new method parameter to an existing interface at that time, which added some extra complexity for the developer. |
On the contrary: the possibilty of caching is a consequence of the design. A design should not be tailored to any specific use case (eg caching). Instead, proper design should lead to abstractions that empower end users to do what they need. Eg caching.
See my previous note.
None of the core encoders use the context to decide in |
|
I'm sorry but I disagree. These interfaces were not meant to add any behavior but to allow some various metadata to transit, which is something that almost any encoder/normalizer need hence it is not worth such a dedicated abstraction which appears to be annoying to most. Applications using the serializer usually have custom encoders and normalizers so it's not only impacting the built-in ones. |
|
Relying on Undeprecating in 6.1 is a must to me. If we like to re-deprecate in 6.2, we must do so while providing a viable alternative. |
And deprecating is a must to me... which I'd prefer not doing twice for obvious reasons. Linking to #43982 btw to know about these interfaces purpose and the motivation behind removing them. |
|
I also prefer introducing a new dedicated interface instead of re-introducing this hacky interface that I introduced initially to fix an older bad design choice that was annoying for many use cases. |
|
Thanks for your patience and your consideration on the fact that deprecating these interfaces comes with relevant motivations. |
|
Thank you @nicolas-grekas. |
…n ChainEncoder/ChainDecoder (Guite) This PR was merged into the 4.4 branch. Discussion ---------- [Serializer] Fix caching context-aware encoders/decoders in ChainEncoder/ChainDecoder | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #38270 | License | MIT | Doc PR | - As proposed in #43231 (comment) Will require #47150 on 6.1 until a proper solution is implemented, hopefully in 6.2. Commits ------- 5902380 [Serializer] Fix caching context-aware encoders/decoders in ChainEncoder/ChainDecoder
This PR was merged into the 6.1 branch. Discussion ---------- remove upgrade instructions leftovers of #47150 | Q | A | ------------- | --- | Branch? | 6.1 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | | License | MIT | Doc PR | This PR corrects that #47150 has not been reflected in the UPGRADE doc. Commits ------- dc07bdf remove wrong entries for #47150
* 6.1: remove wrong entries for #47150 Fix LocaleListenerTest Accept-Language headers
While reviewing #43231, I figured out that the correct fix for #38270 was to make
ChainEncoderproperly considerContextAwareEncoderInterfacewhen deciding to cache or not.Since this interface is useful to discriminate the cache/no-cache situations, we have to undeprecate it (from #43982.)
/cc @Guite and @mtarld FYI