Skip to content

[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

lyrixx
Copy link
Member

@lyrixx lyrixx commented Mar 24, 2017

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):

    /**
     * @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);
    }

Copy link
Member

@javiereguiluz javiereguiluz 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.

@lyrixx lyrixx force-pushed the serializer-ConstraintViolationListNormalizer branch from 1bea195 to 72d93ae Compare March 24, 2017 16:32
@lyrixx lyrixx force-pushed the serializer-ConstraintViolationListNormalizer branch 2 times, most recently from 93d6d61 to 9543d83 Compare March 27, 2017 13:04
@nicolas-grekas nicolas-grekas added this to the 3.3 milestone Mar 28, 2017
@dunglas
Copy link
Member

dunglas commented Mar 28, 2017

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

dunglas commented Mar 28, 2017

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?

@lyrixx lyrixx force-pushed the serializer-ConstraintViolationListNormalizer branch 4 times, most recently from 3d9be72 to 13aabb3 Compare March 29, 2017 12:51
@lyrixx
Copy link
Member Author

lyrixx commented Mar 29, 2017

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

@fabpot
Copy link
Member

fabpot commented Mar 29, 2017

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

@lyrixx lyrixx force-pushed the serializer-ConstraintViolationListNormalizer branch from 13aabb3 to 6024134 Compare April 3, 2017 14:13
@lyrixx
Copy link
Member Author

lyrixx commented Apr 3, 2017

Comment addressed. PR updated.

Note: I also renamed propertyPath to property_path`.

@dunglas
Copy link
Member

dunglas commented Apr 3, 2017

Note: I also renamed propertyPath to property_path

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

@lyrixx
Copy link
Member Author

lyrixx commented Apr 3, 2017

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

@lyrixx lyrixx force-pushed the serializer-ConstraintViolationListNormalizer branch from 6024134 to 4d59983 Compare April 3, 2017 14:22
@lyrixx
Copy link
Member Author

lyrixx commented Apr 3, 2017

Reverted. The PR is now ready.

@lyrixx
Copy link
Member Author

lyrixx commented Oct 5, 2017

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-grekas nicolas-grekas modified the milestones: 3.4, 4.1 Oct 8, 2017
@lyrixx lyrixx force-pushed the serializer-ConstraintViolationListNormalizer branch from b82f7a6 to 82f6e0d Compare February 16, 2018 14:22
@lyrixx
Copy link
Member Author

lyrixx commented Feb 16, 2018

I have rebased the PR ; it's not ready

@lyrixx lyrixx force-pushed the serializer-ConstraintViolationListNormalizer branch from 82f6e0d to 3bcbc5a Compare February 16, 2018 15:11
@lyrixx
Copy link
Member Author

lyrixx commented Mar 14, 2018

Ping

@fabpot
Copy link
Member

fabpot commented Mar 22, 2018

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

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.

use Symfony\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.

@lyrixx lyrixx force-pushed the serializer-ConstraintViolationListNormalizer branch from 3bcbc5a to 971ddb5 Compare March 22, 2018 09:35
@lyrixx
Copy link
Member Author

lyrixx commented Mar 22, 2018

@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

@fabpot fabpot 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 say implements 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
Member Author

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's comment.

We are allowed to omit the type property.

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

@lyrixx lyrixx force-pushed the serializer-ConstraintViolationListNormalizer branch from 971ddb5 to 2a35d09 Compare March 22, 2018 09:58
@fabpot
Copy link
Member

fabpot commented Mar 23, 2018

Thank you @lyrixx.

@fabpot fabpot merged commit 2a35d09 into symfony:master Mar 23, 2018
fabpot added a commit that referenced this pull request Mar 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
@lyrixx lyrixx deleted the serializer-ConstraintViolationListNormalizer branch May 3, 2018 08:38
@fabpot fabpot mentioned this pull request May 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 request May 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     <!-- see https://symfony.com/bc -->
| Deprecations? | yes| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #22150 (comment)
| License       | MIT
| Doc PR        | todo

This 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 @teohhanhui

Commits
-------

3c789c6 [Serializer] Fix and improve constraintViolationListNormalizer's RFC7807 compliance
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.

10 participants