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

Fixed an error where an exception was thrown if the messenger input data has an id field #3448

Closed

Conversation

karser
Copy link
Contributor

@karser karser commented Mar 13, 2020

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tickets fixes #...
License MIT

How to reproduce:

#api_resources.yaml:
App\Dto\DummyWithIdField:
    attributes:
        messenger: true
        output: false
    collectionOperations:
        post:
            path: '/path/to/dummy'
            status: 202
    itemOperations: {}
namespace App\Dto;

class DummyWithIdField {
    /** @var string|null */
    public $id;
}
curl --location --request POST 'https://api.domain.com/path/to/dummy' \
--header 'Content-Type: application/json' \
--data-raw '{"id":"ev_16CHO7Rt6vyOp8ng"}
'

The error in response:

{
    "@context": "/contexts/Error",
    "@type": "hydra:Error",
    "hydra:title": "An error occurred",
    "hydra:description": "Update is not allowed for this operation.",
    "trace": [
        {
            "namespace": "",
            "short_class": "",
            "class": "",
            "type": "",
            "function": "",
            "file": "vendor/api-platform/core/src/Serializer/ItemNormalizer.php",
            "line": 59,
            "args": []
        },

The entire stacktrace https://gist.github.com/karser/9425657bff25da4026591e6af6e227fb

if (!isset($context[self::OBJECT_TO_POPULATE]) && isset($data['data']['id'])) {
if (!isset($context[self::OBJECT_TO_POPULATE])
&& isset($data['data']['id'])
&& (null === $this->resourceMetadataFactory || !$this->resourceMetadataFactory->create($class)->getAttribute('messenger', false))
Copy link
Member

Choose a reason for hiding this comment

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

$this->resourceMetadataFactory->create($class)

may be null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(true || x) will never evaluate x

Copy link
Member

Choose a reason for hiding this comment

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

Not resourceMetadataFactory I mean the result of calling create can be null https://3v4l.org/18tE1 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, got it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it

src/JsonLd/Serializer/ItemNormalizer.php Outdated Show resolved Hide resolved
src/Serializer/ItemNormalizer.php Outdated Show resolved Hide resolved
@karser karser force-pushed the messenger-update-is-not-allowed branch from 4c48b64 to f457e46 Compare March 16, 2020 12:34
@soyuka
Copy link
Member

soyuka commented Mar 16, 2020

can you rebase on 2.5 ?

@karser karser force-pushed the messenger-update-is-not-allowed branch from f457e46 to c8ec35e Compare March 16, 2020 14:46
@karser
Copy link
Contributor Author

karser commented Mar 16, 2020

I rebased against 2.5

@teohhanhui
Copy link
Contributor

I'm sorry, but this doesn't seem like the right fix at all.

@teohhanhui
Copy link
Contributor

If there is a bug, we should address the core issue. This workaround is wrong.

@soyuka
Copy link
Member

soyuka commented Mar 20, 2020

IMO the isset($data['data']['id']) check is already a wrong assumption, if we can avoid messenger to go through this check it simplifies some cases. I already hit that bug but can't remember what I did to fix it.

@karser karser force-pushed the messenger-update-is-not-allowed branch from c8ec35e to e5ef106 Compare December 16, 2020 15:31
@stale
Copy link

stale bot commented Nov 5, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Nov 5, 2022
@stale
Copy link

stale bot commented Jan 4, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jan 4, 2023
@stale stale bot closed this Jan 11, 2023
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.

None yet

3 participants