10000 [Serializer] ChainEncoder does not honor context support check if called repeatedly · Issue #38270 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Serializer] ChainEncoder does not honor context support check if called repeatedly #38270

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.

Sign up for GitHub

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

Jump to bottom
Closed
Guite opened this issue Sep 22, 2020 · 6 comments
Closed

[Serializer] ChainEncoder does not honor context support check if called repeatedly #38270

Guite opened this issue Sep 22, 2020 · 6 comments
Labels
Bug Serializer Status: Needs Review

Comments

@Guite
Copy link
Contributor
Guite commented Sep 22, 2020 •
edited
Loading

ChainEncoder#getEncoder method reads:

        if (isset($this->encoderByFormat[$format])
            && isset($this->encoders[$this->encoderByFormat[$format]])
        ) {
            return $this->encoders[$this->encoderByFormat[$format]];
        }

        foreach ($this->encoders as $i => $encoder) {
            if ($encoder->supportsEncoding($format, $context)) {
                $this->encoderByFormat[$format] = $i;

                return $encoder;
            }
        }

If that method is called more than once with different $context arguments the wrong encoder could be returned because $encoder->supportsEncoding($format, $context) is not called again.

Hence, I disabled the first check and return statement and things worked like a charm.
@carsonbot
Copy link

Hey, thanks for your report!
There has not been a lot of activity here for a while. Is this bug still relevant? Have you managed to find a workaround?

@carsonbot carsonbot added Stalled and removed Stalled labels Mar 24, 2021
@carsonbot
Copy link

Hey, thanks for your report!
There has not been a lot of activity here for a while. Is this bug still relevant? Have you managed to find a workaround?

@carsonbot carsonbot added Stalled and removed Stalled labels Sep 25, 2021
@xabbuh
Copy link
Member
xabbuh commented Sep 27, 2021

Can you create a small example application that allows to reproduce your issue?

@Guite
Copy link
Contributor Author
Guite commented Sep 27, 2021

I can do a PR with the proposed changes including a test case that checks exactly this issue. How about that?

@Guite Guite mentioned this issue Sep 28, 2021
Closed
@Guite Guite changed the title [Serialiser] ChainEncoder does not honor context support check if called repeatedly [Serializer] ChainEncoder does not honor context support check if called repeatedly Sep 28, 2021
@carsonbot
Copy link

Hey, thanks for your report!
There has not been a lot of activity here for a while. Is this bug still relevant? Have you managed to find a workaround?

@Guite
Copy link
Contributor Author
Guite commented Apr 2, 2022

still relevant; PR exists #43231

@carsonbot carsonbot removed the Stalled label Apr 2, 2022
@nicolas-grekas nicolas-grekas mentioned this issue Aug 2, 2022
Merged
@nicolas-grekas nicolas-grekas mentioned this issue Aug 17, 2022
Merged
chalasr added a commit that referenced this issue Aug 21, 2022
@chalasr
bug #47150 [Serializer] Revert deprecation of `ContextAwareEncoderInt…
f8ed3ce 
…erface` and `ContextAwareDecoderInterface` (nicolas-grekas)

This PR was merged into the 6.1 branch.

Discussion
----------

[Serializer] Revert deprecation of `ContextAwareEncoderInterface` and `ContextAwareDecoderInterface`

| Q             | A
| ------------- | ---
| Branch?       | 6.1
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

While reviewing #43231, I figured out that the correct fix for #38270 was to make `ChainEncoder` properly consider `ContextAwareEncoderInterface` when 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

Commits
-------

9ac7fb1 [Serializer] Revert deprecation of `ContextAwareEncoderInterface` and `ContextAwareDecoderInterface`
chalasr added a commit that referenced this issue Aug 21, 2022
@chalasr
bug #47304 [Serializer] Fix caching context-aware encoders/decoders i…
ade30f7 
…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
@chalasr chalasr closed this as completed Aug 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Serializer Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@Guite @xabbuh @chalasr @carsonbot
0