Skip to content

Fixes to Content Negotiation / Serialization docs #1357

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
merged 1 commit into from
May 8, 2021

Conversation

jamesisaac
Copy link
Contributor

  • Docs state that only JSON-LD is enabled by default, which isn't the case: https://github.com/api-platform/core/blob/de5988dacc871a7c1565aa8dea8c30102984cff9/src/Bridge/Symfony/Bundle/DependencyInjection/Configuration.php#L216...L218
    • While JSON may only enabled for the sake of Swagger, it still means that someone can use content negotiation anywhere on the API to request data in an unexpected format... perhaps bypassing a custom normalizer. Important for security considerations developers are aware that more formats exist.
    • (HTML is enabled by default too, but from what I see this doesn't provide a way to actually interact with the main API - so probably doesn't need to be mentioned?)
  • Added the missing myformat example which is talked about but not shown.
  • In the Serialization section, the approach recommended for adding a custom serializer to multiple content types won't work due to duplicate YAML key.

@@ -694,7 +694,9 @@ services:
Note: this normalizer will work only for JSON-LD format, if you want to process JSON data too, you have to decorate another service:

```yaml
App\Serializer\ApiNormalizer:
# Need a different name to avoid duplicate YAML key
'App\Serializer\ApiNormalizer.json':
Copy link
Member

@alanpoulain alanpoulain May 7, 2021

Choose a reason for hiding this comment

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

Shouldn't we change it to: app.serializer.normalizer.item.json (and the previous one to app.serializer.normalizer.item.jsonld)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what best practice is in this scenario. Most Symfony docs these days just use the full class name, e.g.: https://symfony.com/doc/current/service_container/service_decoration.html , which makes things more concise.

But that pattern breaks if you want the same class to decorate 2 different services, as is the case here.

Maybe best to just update the name for the 2nd one to your suggestion, so that anyone who is just supporting a single content-type sees the more typical naming convention?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or maybe it's very common for users to want to support multiple content types, so worth using the app.serializer naming convention for both the services? Let me know what you'd prefer.

Copy link
Member

Choose a reason for hiding this comment

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

I think having a FQCN name is only useful for autowiring. In this case, it would probably be clearer to have the classical name for both.
But your way works too 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point about autowiring -- if these all used classical naming, then the ApiNormalizer class would not be available for autowired injection in other classes, unless an alias was also added, right? https://symfony.com/doc/current/service_container/autowiring.html#using-aliases-to-enable-autowiring

Can imagine that ending up catching some people out? I've updated PR for now to leave first instance with FQCN, and next with classical naming.

@alanpoulain alanpoulain merged commit 8cd89d0 into api-platform:2.6 May 8, 2021
@alanpoulain
Copy link
Member

Thank you @jamesisaac.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants