Skip to content
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

Better handling when unable to parse body content #9

Open
weierophinney opened this issue Dec 31, 2019 · 3 comments
Open

Better handling when unable to parse body content #9

weierophinney opened this issue Dec 31, 2019 · 3 comments

Comments

@weierophinney
Copy link
Contributor

Currently, ContentTypeListener, on PATCH, PUT, or DELETE requests with body content, will attempt one of the following:

  • parsing multipart/form-data content
  • parsing application/json content
  • parsing using parse_str()

This latter is a fallback for unknown content types. If it is unable to parse the string content using parse_str(), it then uses the raw content. However, it tries to push this into a ParameterDataContainer using its setBodyParams() method — which only accepts arrays. As a result, when such a condition occurs, a fatal error occurs due to type mismatch.

There are two potential approaches to take when this occurs:

  • Return an ApiProblemResponse with a status of 400, indicating client error, or a 415 (unsupported media type).
  • Set no data or an empty array in the ParameterDataContainer, and allow processing to continue.

The first possibility is likely most correct, as it indicates that we received content we cannot process. The second possibility will likely eventually lead to an ApiProblemResponse, however, due to inability to validate the data in zf-content-validation, or inability to process it within application code; it also leaves the possibility of processing it manually via a listener or application code.

I hereby solicit help to make this feature happen, as it will resolve a number of reported issues. I would be okay with either approach; argue the approach you take well when submitting a patch.


Originally posted by @weierophinney at zfcampus/zf-content-negotiation#76

@weierophinney
Copy link
Contributor Author

Would it be an option to segregate the parsing fromt the ContentTypeListener and pass a parser map in a config file. Like that the listener won't have to guess how the content in the request body needs to be parsed and users will be able to map their custom parser to their custom content-types. Something in the line of:

$config = [
    'parsers' => [
        'application/json' => 'ZF\ContentNegotiation\Parsers\JsonParser',
        'application/hal+json' => 'ZF\ContentNegotiation\Parsers\HalJsonParser',
        'application/hal+xml' => 'ZF\ContentNegotiation\Parsers\HalXmlParser',
        'multipart/form-data' => 'ZF\ContentNegotiation\Parsers\MultipartFormDataParser',
        'custom/content' => 'My\Custom\ContentParser'
    ]
];

A ParserPluginManager could make things even prettier...

Parsers should implement an interface (ContentParserInterface) and the ContentParserInterface::parse method would need to return an array so the result can be immediately added to ParameterDataContainer.

Like this you can get rid of all this trial and error parsing and when a client passes a content-type for which no parser has been registered the server could return a ApiProblemResponse that informs the client that the server is unable to parse the received content.

It would also allow future support for the XML variant of json


Originally posted by @Wilt at zfcampus/zf-content-negotiation#76 (comment)

@weierophinney
Copy link
Contributor Author

@weierophinney Please have a look on my solution and explanation in #78. Thanks!


Originally posted by @michalbundyra at zfcampus/zf-content-negotiation#76 (comment)

@weierophinney
Copy link
Contributor Author

@weierophinney I started on something in a branch called content-type-listener-parsing.
I really think that having several parsers would take this module (and Apigility) to the next level.
But if there is no interest I drop working on this PR because it took more time then I expected and I have lots of other work piling.


Originally posted by @Wilt at zfcampus/zf-content-negotiation#76 (comment)

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

No branches or pull requests

1 participant