-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
core/serialization.md
Outdated
@@ -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': |
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.
Shouldn't we change it to: app.serializer.normalizer.item.json
(and the previous one to app.serializer.normalizer.item.jsonld
)?
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.
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?
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.
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.
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.
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 🙂
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.
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.
Thank you @jamesisaac. |
myformat
example which is talked about but not shown.