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

[Serializer] Added a ConstraintViolationListNormalizer#22150

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

Conversation

@lyrixx
Copy link
Member

@lyrixxlyrixx commentedMar 24, 2017
edited by javiereguiluz
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#11309
LicenseMIT
Doc PR-

It seems logical to me that Symfony is able to serialise natively some very common Symfony data structure. (and requested by@nicolas-grekas &@javiereguiluz )

Usage example (from symfony/symfony-demo):

/**     * @Route("", name="api_blog_new")     * @Method("POST")     * @Security("is_granted('ROLE_ADMIN')")     */publicfunctionnewAction(Request$request)    {$data =$request->getContent();$post =$this->get('serializer')->deserialize($data, Post::class,'json', ['groups' => ['post_write']]);$post->setAuthor($this->getUser());$violations =$this->get('validator')->validate($post);$post->setSlug($this->get('slugger')->slugify($post->getTitle()));if (count($violations) >0) {$repr =$this->get('serializer')->serialize($violations,'json');return JsonResponse::fromJsonString($repr,400);        }$this->getDoctrine()->getManager()->persist($post);$this->getDoctrine()->getManager()->flush();$repr =$this->get('serializer')->serialize($post,'json', ['groups' => ['post_read']]);return JsonResponse::fromJsonString($repr);    }

theofidry, ogizanagi, jvasseur, DavidBadura, ostrolucky, Koc, welcoMattic, and ste93cry reacted with thumbs up emoji
Copy link
Member

@javiereguiluzjaviereguiluz left a comment

Choose a reason for hiding this comment

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

👍 it looks like a useful feature to me! Thanks@lyrixx.

@lyrixxlyrixxforce-pushed theserializer-ConstraintViolationListNormalizer branch from1bea195 to72d93aeCompareMarch 24, 2017 16:32
@lyrixxlyrixxforce-pushed theserializer-ConstraintViolationListNormalizer branch 2 times, most recently from93d6d61 to9543d83CompareMarch 27, 2017 13:04
@nicolas-grekasnicolas-grekas added this to the3.3 milestoneMar 28, 2017
@dunglas
Copy link
Member

The original implementation (https://github.com/api-platform/core/blob/master/src/Hydra/Serializer/ConstraintViolationListNormalizer.php) was following the Hydra W3C draft.

WDYT about keeping Hydra support in Symfony?

@dunglas
Copy link
Member

Another alternative is to implement RFC7807 (supported by API Platform and Zend Apigility):https://github.com/api-platform/core/blob/master/src/Problem/Serializer/ConstraintViolationListNormalizer.php

@javiereguiluz
Copy link
Member

RFC7807 looks like the ideal solution to create something standard ... but its status is still: "PROPOSED STANDARD". Can we guess the chances of being changed a lot in the future or even rejected?

@lyrixxlyrixxforce-pushed theserializer-ConstraintViolationListNormalizer branch 4 times, most recently from3d9be72 to13aabb3CompareMarch 29, 2017 12:51
@lyrixx
Copy link
MemberAuthor

I implemented RFC7807 (supported by API Platform and Zend Apigility)

@fabpot
Copy link
Member

Can you add a note about the fact that it implements RFC7807 in the phpdocs?

@lyrixxlyrixxforce-pushed theserializer-ConstraintViolationListNormalizer branch from13aabb3 to6024134CompareApril 3, 2017 14:13
@lyrixx
Copy link
MemberAuthor

Comment addressed. PR updated.

Note: I also renamedpropertyPath to property_path`.

@dunglas
Copy link
Member

Note: I also renamed propertyPath toproperty_path

Why this change? AFAIK, camel case is preferred (for instance, it's what Google and Schema.org use).

@lyrixx
Copy link
MemberAuthor

Ah ok. I will revert it so. Usually I prefer Snake Case in my array / API...

@lyrixxlyrixxforce-pushed theserializer-ConstraintViolationListNormalizer branch from6024134 to4d59983CompareApril 3, 2017 14:22
@lyrixx
Copy link
MemberAuthor

Reverted. The PR is now ready.

@lyrixx
Copy link
MemberAuthor

The status code can be passed through the context (it can be optional).

But it's not mandatory, and it's boring if you need to pass 4XX each time ... :/

@nicolas-grekasnicolas-grekas modified the milestones:3.4,4.1Oct 8, 2017
@lyrixxlyrixxforce-pushed theserializer-ConstraintViolationListNormalizer branch fromb82f7a6 to82f6e0dCompareFebruary 16, 2018 14:22
@lyrixx
Copy link
MemberAuthor

lyrixx commentedFeb 16, 2018
edited
Loading

I have rebased the PR ; it's not ready

@lyrixxlyrixxforce-pushed theserializer-ConstraintViolationListNormalizer branch from82f6e0d to3bcbc5aCompareFebruary 16, 2018 15:11
@lyrixx
Copy link
MemberAuthor

Ping

@fabpot
Copy link
Member

@lyrixx "it's not ready" -> I suppose you meant "it's now ready", right?

lyrixx reacted with thumbs up emojisroze and Kocal reacted with laugh emoji

of objects that needs data insertion in constructor
* added an optional`default_constructor_arguments` option of context to specify a default data in
case the object is not initializable by its constructor because of data missing
* added optional`bool $escapeFormulas = false` argument to`CsvEncoder::__construct`
Copy link
Member

Choose a reason for hiding this comment

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

Looks like you haven't added anything here.

useSymfony\Component\Validator\ConstraintViolationListInterface;

/**
* A normalizer that normalize a ConstraintViolationListInterface instance.
Copy link
Member

Choose a reason for hiding this comment

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

normalizes

/**
* A normalizer that normalize a ConstraintViolationListInterface instance.
*
* This Normalizer implements RFC7807 {@link https://tools.ietf.org/html/rfc7807}.
Copy link
Member

Choose a reason for hiding this comment

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

If we're not fully compliant, it should be added here.

@lyrixxlyrixxforce-pushed theserializer-ConstraintViolationListNormalizer branch from3bcbc5a to971ddb5CompareMarch 22, 2018 09:35
@lyrixx
Copy link
MemberAuthor

@lyrixx "it's not ready" -> I suppose you meant "it's now ready", right?

Oups, You are right.


I have addressed your comment. It should be OK now

Copy link
Member

@fabpotfabpot left a comment

Choose a reason for hiding this comment

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

minor comment

/**
* A normalizer that normalizes a ConstraintViolationListInterface instance.
*
* This Normalizer implements partially RFC7807 {@link https://tools.ietf.org/html/rfc7807}.
Copy link
Member

Choose a reason for hiding this comment

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

I would sayimplements RFC7807 partially.

But can we be a bit more precise? Can we list what we do not support or how we diverge from the RFC? I think that would help our users.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Actually I re-read the RFC and came up to the same conclusion as@teohhanhui'scomment.

We are allowed to omit thetype property.

That's why I removed the "partially" because the normalizer do implement the RFC.

@lyrixxlyrixxforce-pushed theserializer-ConstraintViolationListNormalizer branch from971ddb5 to2a35d09CompareMarch 22, 2018 09:58
@fabpot
Copy link
Member

Thank you@lyrixx.

@fabpotfabpot merged commit2a35d09 intosymfony:masterMar 23, 2018
fabpot added a commit that referenced this pull requestMar 23, 2018
… (lyrixx)This PR was merged into the 4.1-dev branch.Discussion----------[Serializer] Added a ConstraintViolationListNormalizer| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#11309| License       | MIT| Doc PR        | ----It seems logical to me that Symfony is able to serialise natively some very common Symfony data structure. (and requested by@nicolas-grekas &@javiereguiluz )Usage example (from symfony/symfony-demo):```php    /**     *@route("", name="api_blog_new")     *@method("POST")     * @Security("is_granted('ROLE_ADMIN')")     */    public function newAction(Request $request)    {        $data = $request->getContent();        $post = $this->get('serializer')->deserialize($data, Post::class, 'json', ['groups' => ['post_write']]);        $post->setAuthor($this->getUser());        $violations = $this->get('validator')->validate($post);        $post->setSlug($this->get('slugger')->slugify($post->getTitle()));        if (count($violations) > 0) {            $repr = $this->get('serializer')->serialize($violations, 'json');            return JsonResponse::fromJsonString($repr, 400);        }        $this->getDoctrine()->getManager()->persist($post);        $this->getDoctrine()->getManager()->flush();        $repr = $this->get('serializer')->serialize($post, 'json', ['groups' => ['post_read']]);        return JsonResponse::fromJsonString($repr);    }```Commits-------2a35d09 [Serializer] Added a ConstraintViolationListNormalizer
@lyrixxlyrixx deleted the serializer-ConstraintViolationListNormalizer branchMay 3, 2018 08:38
@fabpotfabpot mentioned this pull requestMay 7, 2018
));

$expected =array(
'title' =>'An error occurred',
Copy link
Contributor

Choose a reason for hiding this comment

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

RFC 7807 is unambiguous on this point:

3.1. Members of a Problem Details Object

A problem details object can have the following members:

  • "type" (string) - ... When
    this member is not present, its value is assumed to be
    "about:blank".

4.2. Predefined Problem Types

This specification reserves the use of one URI as a problem type:

The "about:blank" URI [RFC6694], when used as a problem type,
indicates that the problem has no additional semantics beyond that of
the HTTP status code.

When "about:blank" is used, the title SHOULD be the same as the
recommended HTTP status phrase for that code (e.g., "Not Found" for
404, and so on), although it MAY be localized to suit client
preferences (expressed with the Accept-Language request header).

Please note that according to how the "type" member is defined
(Section 3.1), the "about:blank" URI is the default value for that
member. Consequently, any problem details object not carrying an
explicit "type" member implicitly uses this URI.

Perhaps:

{"type":"/validation-error","title":"Validation failed",...}

fabpot added a commit that referenced this pull requestMay 21, 2018
…izer's RFC7807 compliance (dunglas)This PR was squashed before being merged into the 4.1 branch (closes#27292).Discussion----------[Serializer] Fix and improve constraintViolationListNormalizer's RFC7807 compliance| Q             | A| ------------- | ---| Branch?       | 4.1| Bug fix?      | yes| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks?    | no     <!-- seehttps://symfony.com/bc -->| Deprecations? | yes| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->| Fixed tickets |#22150 (comment)| License       | MIT| Doc PR        | todoThis PR fixes and improves [RFC 7807](https://tools.ietf.org/html/rfc7807#section-3.2) compliance of `ConstraintViolationListNormalizer` (introduced in 4.1):* As recommended, use a specific namespace for Symfony validation error (`http://symfony.com/doc/current/validation.html`, because it already exists and gives information about the error.* Allow to set all properties defined in the RFC using the serialization context* Remove the `detail` key if no detail is provided (according to the spec)* Change the Symfony specific extension to use the same terminology than the RFC itself (type and title)* Use the proper `urn:uuid` scheme (RFC 4122) for the UUID code (more standard, and improve hypermedia capabilities).ping@teohhanhuiCommits-------3c789c6 [Serializer] Fix and improve constraintViolationListNormalizer's RFC7807 compliance
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@dunglasdunglasdunglas approved these changes

@stofstofstof requested changes

@fabpotfabpotfabpot approved these changes

@javiereguiluzjaviereguiluzjaviereguiluz approved these changes

+3 more reviewers

@teohhanhuiteohhanhuiteohhanhui left review comments

@theofidrytheofidrytheofidry left review comments

@ro0NLro0NLro0NL approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.1

Development

Successfully merging this pull request may close these issues.

10 participants

@lyrixx@dunglas@javiereguiluz@fabpot@teohhanhui@stof@ro0NL@theofidry@nicolas-grekas@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp