Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5.3k
[Serializer] Add missing options context#14384
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
maxhelias commentedOct 9, 2020
This PR can be transformed to document the context options of each serialization format#8505 but i need feedback 😄 |
javiereguiluz commentedOct 9, 2020
@maxhelias minor comment: there's no need to create "draft PRs" because in Symfony we consider all PRs already "in draft" and "ready to review" until they are merged. In the code repository we do the same and we don't use draft PRs. Thanks! |
maxhelias commentedOct 9, 2020
@javiereguiluz ok I'll change then. |
maxhelias commentedOct 13, 2020
Status: Needs Work |
wouterj commentedNov 5, 2020
Fyi, I've just merged the PR you mentioned. However, I've also opened another PR that changes this:#14521 After that, this PR should probably be rebased. |
maxhelias commentedNov 12, 2020
@wouterj I rebase the branch and I harmonize a little more the Normalizers & Encoders section. |
wouterj commentedNov 12, 2020
About the DOCtor errors your seeing: I reported this a couple days:OskarStark/doctor-rst#808 . Normally, we add 2 spaces between the columns in these tables, but to avoid this error I changed it to 1 space for one column :) |
0850560 tod7a7cc2Comparemaxhelias commentedNov 13, 2020
Thanks for the tips, it's fixed 👍 |
maxhelias commentedNov 30, 2020
friendly ping@wouterj 😃 |
wouterj left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sorry, somehow this PR was no longer in my notification center.
I've one minor syntax fix, which we can also fix while merging. Are all these options introduced before Symfony 4.4, or do we need a.. versionadded:: 4.4 directive for new options?
Uh oh!
There was an error while loading.Please reload this page.
maxhelias commentedNov 30, 2020
Don't worry about it. The most recent one has been integrated in version 4.1, do you want the directive anyway ? |
OskarStark commentedDec 2, 2020
The correct versionadded directives would be super helpful if you can find them out 🙏 |
maxhelias commentedDec 13, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@wouterj@OskarStark I added the right directive |
Uh oh!
There was an error while loading.Please reload this page.
maxhelias commentedJan 11, 2021
friendly ping@wouterj or@OskarStark 😃 |
* 4.4: [#14384] Minor grammar fix in versionadded directive Add missing options context
* 5.1: Removed 4.2 versionadded directive [#14384] Minor grammar fix in versionadded directive Add missing options context
* 5.2: Removed 4.2 versionadded directive [#14384] Minor grammar fix in versionadded directive Add missing options context
wouterj commentedJan 11, 2021
Thank you@maxhelias (also thank you for your patience!)! Lots of great new content in this PR :) |
Uh oh!
There was an error while loading.Please reload this page.
The options are not really in the right place since just above the text indicates the elements for associative arrays.
But we only list context options for an associative array here :https://symfony.com/doc/current/components/serializer.html#context
Wouldn't we move all the options for all encoders here ?https://symfony.com/doc/current/components/serializer.html#encoders
And in the first section display only the possible options for associative array with the link to their description.
Solve#10277 and#10286