Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[HttpKernel] Create#[Serialize] Attribute to serialize Controller Result#49518

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

Open
Koc wants to merge1 commit intosymfony:7.4
base:7.4
Choose a base branch
Loading
fromKoc:feature/serialized-response

Conversation

Koc
Copy link
Contributor

@KocKoc commentedFeb 23, 2023•
edited
Loading

QA
Branch?6.3
Bug fix?no
New feature?yes
Deprecations?no
Tickets-
LicenseMIT
Doc PRTBD

#[Serialize(code: 200, headers: [], serializationContext: [])] Allows automatically serialize Controller Result to format based on Request format.

Usage example 🔨

class Product{publicfunction__construct(publicreadonlyint$id,publicreadonlystring$name)    {    }}class GetProductController{    #[Serialize]publicfunction__invoke():Product    {returnnewProduct(1,'Asus UX550');    }}class ProductCreated{publicfunction__construct(publicreadonlyint$id)    {    }}class CreateProductController{    #[Serialize(201)]publicfunction__invoke():ProductCreated    {returnnewProductCreated(1);    }}# same controller without annotation: repeatable annoying boilerplate which has no sense to writeclass CreateProductControllerLegacy{publicfunction__construct(privatereadonlySerializerInterface$serializer)    {    }    #[OA\Response(response:201, description:'Successful response', content:newModel(type: ProductCreated::class))]publicfunction__invoke():JsonResponse    {$data =newProductCreated(1);$serialized =$this->serializer->serialize($data,'json');return JsonResponse::fromJsonString($serialized, JsonResponse::HTTP_CREATED);    }}

Todo

emomaliev, 94noni, yceruto, Xen3r0, and IonBazan reacted with thumbs up emojivaltzu and gabplch reacted with thumbs down emojidunglas reacted with confused emojiIonBazan reacted with heart emoji
@KocKoc changed the titleCreate#[SerializedResponse] Attribute to serialize Controller Result[HttpKernel] Create#[SerializedResponse] Attribute to serialize Controller ResultFeb 23, 2023
@KocKocforce-pushed thefeature/serialized-response branch from8e54594 to9e16d8cCompareFebruary 23, 2023 22:36
ro0NL

This comment was marked as outdated.

@KocKocforce-pushed thefeature/serialized-response branch froma30ee49 to06087b9CompareFebruary 24, 2023 11:41
@KocKoc changed the title[HttpKernel] Create#[SerializedResponse] Attribute to serialize Controller Result[HttpKernel] Create#[Serialized] Attribute to serialize Controller ResultFeb 24, 2023
@KocKoc changed the title[HttpKernel] Create#[Serialized] Attribute to serialize Controller Result[HttpKernel] Create#[Serialize] Attribute to serialize Controller ResultFeb 24, 2023
* Controller tag to serialize response.
*/
#[\Attribute(\Attribute::TARGET_METHOD)]
class Serialize
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

indeed, others attribute are prefixed withAs when they describe more the config of the class (AsCommand or AsMessageHandler)
here it is like a mapping/transformation, perhaps like your other PR#49138MapResponse

@KocKocforce-pushed thefeature/serialized-response branch 2 times, most recently from9eea8e6 to9d1fb32CompareFebruary 25, 2023 22:13
@yceruto
Copy link
Member

This test is failinghttps://github.com/symfony/symfony/actions/runs/4272061115/jobs/7436960556#step:7:8239

You probably have to addsymfony/serializer as a dev dependency of the http-kernel component.

@Koc
Copy link
ContributorAuthor

Koc commentedFeb 27, 2023

yes, thanx, it was added in#49138. I've not added it here to prevent merge conflicts. Also I want extend functional tests added in mentioned PR with new Attribute

yceruto reacted with thumbs up emoji

@KocKocforce-pushed thefeature/serialized-response branch from9d1fb32 to3213f4bCompareMarch 6, 2023 22:16
@KocKoc requested a review fromdunglas as acode ownerMarch 6, 2023 22:16
@KocKocforce-pushed thefeature/serialized-response branch 2 times, most recently fromfbc4eae toca0b60dCompareMarch 6, 2023 22:30
ro0NL

This comment was marked as outdated.

@KocKocforce-pushed thefeature/serialized-response branch 2 times, most recently from9845ae5 tof350273CompareApril 6, 2023 22:22
@nicolas-grekas
Copy link
Member

nicolas-grekas commentedApr 7, 2023•
edited
Loading

For what it's worth, I'm personally not convinced we should merge this. The reason is that the resulting behavior is very limited and resorting to explicit code comes very quickly - as soon as one needs more precise control over the resulting response for example. In the end, this saves two/three lines that are legit to have in a controller/similar to me.

dunglas, valtzu, jvasseur, and gabplch reacted with thumbs up emoji

@Koc
Copy link
ContributorAuthor

Koc commentedApr 7, 2023•
edited
Loading

Exactly, this can save few lines in the Controller and reduce boilerplate. Of course it can't cover all needs for an every project, but same applicable for other stuff provided by Framework. 80/20 rule in action. We have few microservices that uses only#[Serialize] Attribute without any kind of additional control for the Response.

We can go further and build OpenApi documentation based on Controller return type + Attribute existence viaNelmioApiDocBundle. It will reduce even more boilerplate code for some projects.

@KocKocforce-pushed thefeature/serialized-response branch 4 times, most recently from8ee1da3 to9058a4bCompareApril 21, 2023 07:38
@KocKocforce-pushed thefeature/serialized-response branch from9058a4b toad86ceaCompareApril 22, 2023 10:40
@Koc
Copy link
ContributorAuthor

Koc commentedApr 22, 2023

Rebased, renamedcontext toserializationContext for consistency forMap* attributes. Test failures looks unrelated.

@nicolas-grekas please review 🙏

@nicolas-grekasnicolas-grekas modified the milestones:6.3,6.4May 23, 2023
@nicolas-grekasnicolas-grekas modified the milestones:6.4,7.1Nov 15, 2023
@KocKocforce-pushed thefeature/serialized-response branch fromad86cea to3c9a9b6CompareJanuary 29, 2024 16:15
@KocKocforce-pushed thefeature/serialized-response branch from3c9a9b6 to490a4adCompareJanuary 29, 2024 16:23
@Koc
Copy link
ContributorAuthor

Koc commentedJan 29, 2024

I've rebased PR one more time + fixed tests + actualized changelog.

And as I've already mentioned - this feature needed not only for reducing boilerplate of serialization but also for simplification of the OpenApi spec generation via NelmioApiDocBundle. They alreadysuccessfully adopted#[MapRequestPayload/MapQueryString]. So, introduction of this attribute can be a good step forward in same direction.

DjordyKoert reacted with eyes emoji

* @author Konstantin Myakshin <molodchick@gmail.com>
*/
#[\Attribute(\Attribute::TARGET_METHOD)]
final class Serialize
Copy link

@DavidGarciaCatDavidGarciaCatFeb 1, 2024•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Shouldn't this#[Serialize] attribute allow to pass the groups to be used during the serialization process?

Also, theSerializerInterface and itsserialize method allows us to pass the preferred format (JSON, XML, ...) so making it an argument (even with a predefined value) makes sense to me 🤔

==== EDIT ====

By the way: with "passing groups" I meant that they seem to be a relevant setting so maybe it's a good idea to add them with a shortcut (instead of passing them via context), but that's just a secondary matter - the primary concern here is the missing format:

$data =$this->serializer->serialize(            data:$output,            format:'json',            context: ['groups' => ['Group1','Group2']]        );

so it could be like this:

#[Serialize(code:201, format:'json', headers: [], serializationContext: ['groups' => ['Group1','Group2']])]

or even

#[Serialize(code:201, format:'json', headers: [], groups: ['Group1','Group2'])]

@xabbuhxabbuh modified the milestones:7.1,7.2May 15, 2024
* @author Konstantin Myakshin <molodchick@gmail.com>
*/
#[\Attribute(\Attribute::TARGET_METHOD)]
final class Serialize
Copy link

@rrajkomarrrajkomarNov 9, 2024•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Hi,
I think this is a great idea and would be very useful for anyone developing webservice so thank you.
However wouldn't it be more logical for consistency to name the attribute in the same manner as the others ?
Something likeMapResponsePayload seems more intuitive IMHO.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

this attributes can be renamed soon, see#58709

@fabpotfabpot modified the milestones:7.2,7.3Nov 20, 2024
@fabpotfabpot modified the milestones:7.3,7.4May 26, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@stofstofstof left review comments

@ro0NLro0NLro0NL left review comments

@94noni94noni94noni left review comments

@rrajkomarrrajkomarrrajkomar left review comments

@DavidGarciaCatDavidGarciaCatDavidGarciaCat left review comments

@dunglasdunglasAwaiting requested review from dunglas

Assignees
No one assigned
Projects
None yet
Milestone
7.4
Development

Successfully merging this pull request may close these issues.

11 participants
@Koc@yceruto@nicolas-grekas@stof@ro0NL@94noni@rrajkomar@DavidGarciaCat@fabpot@xabbuh@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp