8000 FlattenException $previous property not documented as nullable · Issue #40385 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

FlattenException $previous property not documented as nullable #40385

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.

Sign up for GitHub

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

Jump to bottom
Closed
maciejzgadzaj opened this issue Mar 5, 2021 · 6 comments
Closed

FlattenException $previous property not documented as nullable #40385

maciejzgadzaj opened this issue Mar 5, 2021 · 6 comments
Labels
Bug ErrorHandler Status: Needs Review

Comments

@maciejzgadzaj
Copy link
maciejzgadzaj commented Mar 5, 2021 •
edited
Loading

Symfony version(s) affected: 5.2.3

Description
The $previous property of FlattenException is not documented as nullable, and it denormalizes incorrectly when set to null.

How to reproduce

        $fe = FlattenException::create(new \Exception());
        dump($fe->getPrevious());
        $sfe = $this->serializer->serialize($fe, 'json');
        $dfe = $this->serializer->deserialize($sfe, FlattenException::class, 'json');
        dump($dfe->getPrevious());

Output:

^ null
^ Symfony\Component\ErrorHandler\Exception\FlattenException^ {#1272
  -message: null
  -code: null
  -previous: null
  -trace: null
  -traceAsString: null
  -class: null
  -statusCode: null
  -statusText: null
  -headers: null
  -file: null
  -line: null
  -asString: null
}

Possible Solution
Super simple - just adding a PHPDoc for the property:

    /** @var FlattenException|null */
    private $previous;

then executing the same code as above returns:

^ null
^ null

Additional context
This was found out when using consuming Pub/Sub messages with symfony/messenger and sroze/messenger-enqueue-transport, which use FlattenException for ErrorDetailsStamp.

When the message handler fails processing the message for the very first time, it adds ErrorDetailsStamp with exception flattened (and "previous": null).

On first retry, the stamp is deserialized, and what originally was

        "flattenException": {
            "previous": null,

turns into

        -flattenException: Symfony\Component\ErrorHandler\Exception\FlattenException^ {#1912
          -previous: Symfony\Component\ErrorHandler\Exception\FlattenException^ {#1900
            -message: null
            -code: null
            -previous: null
            -trace: null
            -traceAsString: null
            -class: null
            -statusCode: null
            -statusText: null
            -headers: null
            -file: null
            -line: null
            -asString: null
          }

(This is our issue right here.)

Finally, when processing fails again, the ErrorDetailsStamp (with all the above nulls) fails deserialization with the following errors:

In Serializer.php line 122:
  Could not decode stamp: The type of the "message" attribute for class "Symfony\Component\ErrorHandler\Exception\FlattenException" must be one of "string" ("null" given).  

In AbstractObjectNormalizer.php line 494:
  The type of the "message" attribute for class "Symfony\Component\ErrorHandler\Exception\FlattenException" must be one of "string" ("null" given).

Which means retrying to handle the message works fine as long as max_retries is set to 1, but fails if it's anything above this.

Adding mentioned PHPDoc makes it work perfectly fine.
@derrabus
Copy link
Member
derrabus commented Mar 5, 2021

The properties on FlattenException are not documented at all. This this particular case, it appears that the setPrevious() is confusing the serializer because it does not allow null to be passed.

@derrabus derrabus mentioned this issue Mar 5, 2021
Merged
@derrabus
Copy link
Member
derrabus commented Mar 5, 2021

#40388

@fabpot fabpot closed this as completed Mar 9, 2021
fabpot added a commit that referenced this issue Mar 9, 2021
@fabpot
bug #40388 [ErrorHandler] Added missing type annotations to FlattenEx…
72c6489 
…ception (derrabus)

This PR was merged into the 4.4 branch.

Discussion
----------

[ErrorHandler] Added missing type annotations to FlattenException

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #40385
| License       | MIT
| Doc PR        | N/A

This class has no constructor where the properties' types could be inferred from. If we want potential contributors (and our serializer, see #40385) to read that code, I think it's only fair that we document the property types via doc blocks.

Commits
-------

d68832e [ErrorHandler] Added missing type annotations to FlattenException
@maciejzgadzaj
Copy link
Author

Thanks for the fix @derrabus (you were so fast you didn't give me a chance to do it myself) and the merge @fabpot.

When can I hope to see it merged into 5.x too?

@derrabus
Copy link
Member

@maciejzgadzaj I've just merged the changes up to 5.2 and 5.x.

@maciejzgadzaj
Copy link
Author

Great news, thank you very much @derrabus ! Now I just need to wait for v5.2.6...

@derrabus
Copy link
Member

Great news, thank you very much @derrabus ! Now I just need to wait for v5.2.6...

You can test the changes right away if you like.

"require": {
    "symfony/error-handler": "~5.2.6@dev"
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug ErrorHandler Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@fabpot @maciejzgadzaj @derrabus @carsonbot
0