Skip to content

[Serializer] fixed traceable decoration priorities #47764

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.

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

Conversation

mtarld
Copy link
Contributor

@mtarld mtarld commented Oct 2, 2022

Q A
Branch? 6.1
Bug fix? yes
New feature? no
Deprecations? no
Tickets

Related to symfony/maker-bundle#1252

The decoration priority of TraceableNormalizer and TraceableEncoder is too high, therefore while it should be the last decoration applied, it is applied before userland decorations.

I've set the decoration priority to 0, but maybe -255 might be better.

Moreover, I've seen the very same thing for the TraceableValidator, is it a bug or wanted?

@mtarld mtarld requested a review from dunglas as a code owner October 2, 2022 08:12
@carsonbot carsonbot added this to the 6.1 milestone Oct 2, 2022
@mtarld mtarld changed the title [Serializer] Fix Traceable decoration priorities [Serializer] fixed traceable decoration priorities Oct 2, 2022
@mtarld mtarld force-pushed the fix/traceable-serializer-normalizer-encoder-priority branch from 96fc2dc to c8c096d Compare October 2, 2022 08:13
@mtarld mtarld force-pushed the fix/traceable-serializer-normalizer-encoder-priority branch from c8c096d to 4857205 Compare October 3, 2022 07:27
@chalasr
Copy link
Member

chalasr commented Oct 3, 2022

This makes sense to me.
Other traceable decorators don't have such a high priority except the TraceableValidator. Here is the rationale: #22554 (comment) (tl;dr: it's for extra safety, but as said above the result is surprising).
/cc @ogizanagi I think the same fix should apply to the TraceableValidator.

@ogizanagi
Copy link
Contributor

Hmm, re-reading this, would make sense to make the traceable decorator last indeed… but since we don't have any unexpected report for this yet, perhaps keep this status-quo for the validator one. 🤷🏻‍♂️

@mtarld
Copy link
Contributor Author

mtarld commented Oct 3, 2022

Indeed, I think that decorating a validator isn't a common use case (I think almost nobody tried it).
That being said, the Validator decoration priority isn't valid IMHO, maybe it should be updated?

@chalasr
Copy link
Member

chalasr commented Oct 3, 2022

but since we don't have any unexpected report for this yet, perhaps keep this status-quo for the validator one. 🤷🏻‍♂️

I agree, let's keep it as-is as long as nobody actually complains (from my experience that kind of bugfixes in old versions is source of problems/anger -_-)

@fabpot
Copy link
Member

fabpot commented Oct 4, 2022

Thank you @mtarld.

@fabpot fabpot merged commit fdfad3f into symfony:6.1 Oct 4, 2022
@mtarld mtarld deleted the fix/traceable-serializer-normalizer-encoder-priority branch October 4, 2022 06:02
@fabpot fabpot mentioned this pull request Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants