[Serializer] Document SerializerInterface exceptions
#59306
Conversation
|
Any method everywhere might throw some type of |
This is not true. Some methods (a lot) doesn't throws Exception at all, even in the Symfony codebase. And with well-documented methods, PHPStan helps a lot catching unwanted exceptions Currently, with code like I'm getting because the PHPdoc is not describing the exceptions thrown.
Notice I didn't added
It adds value for static analysis tools (like PHPStan) users.
And a I don't see a reason to add it to SerializerInterface which is a similar interface. For reference, I already added missing |
|
I agree with @wouterj. If we think it's useful, we should do it consistently across the board, not just on one interface. Maybe it's mostly useful for interfaces? But then, do we want to reference the interface or the concrete exception implementations that can be thrown? |
There is already a looot of method/class/interface already with
Since people works mainly with interface, it is. This also explains which exceptions we are supposed to throw when implementing the interface. It throws:
And
So far, I would say both of the strategy are used. For instance We can list all the exceptions thrown with the explanation of the reason but we still have to add a
I considered it was already a good step to add |
|
Thinking about this more, I'm actually thinking the They are documenting a contract ("this method will only throw things that are instance of X") while there is no way to enforce that. Even more, retrospectively adding it to methods creates a new contract for any implementation that they were never aware of. I would say this means that adding |
|
And this is not a theoretical thing. It took me only a quick look to find Symfony itself does not conform this new contract proposed in the PR: use Symfony\Component\Serializer\Encoder\YamlEncoder;
use Symfony\Component\Serializer\Normalizer\ObjectNormalizer;
use Symfony\Component\Serializer\Serializer;
$encoders = [new YamlEncoder()];
$normalizers = [new ObjectNormalizer()];
$serializer = new Serializer($normalizers, $encoders);
$serializer->deserialize('[1', 'stdClass', 'yaml'); // throws Symfony\Component\Yaml\Exception\ParseException |
PHPStan has two mode about the exceptions (https://phpstan.org/blog/bring-your-exceptions-under-control#what-does-absent-%40throws-above-a-function-mean%3F): By default PHPStan consider that no
Technically, there is already a contract which say "This method is not supposed to throw any exception". And any implementation is breaking this contract. I'm trying to fix/improve the contract. Symfony already rely on the possible exception thrown since some are caught in the SerializerErrorEncoder, even if none is documented A better documentation would lead to a better try/catch.
Currently the interface is wrongly informing that no exception are thrown. |
The issue is not about the SerializerInterface contract but about the YamlEncoder which already does not respect the EncoderInterface contract. symfony/src/Symfony/Component/Serializer/Encoder/DecoderInterface.php Lines 33 to 35 in 4078cb1 This YamlEncoder was introduced in Symfony 3 while the So I would think the YamlEncoder need to be fixed. |
SerializerInterface exceptions
…tLanglet) This PR was merged into the 6.4 branch. Discussion ---------- [Serializer] Fix exception thrown by `YamlEncoder` | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix the example given #59306 (comment) | License | MIT DecoderInterface::decode is supposed to throw scoped exception cf https://github.com/symfony/symfony/blob/4078cb1642332cf5bbd45f5118edf766580cfb1e/src/Symfony/Component/Serializer/Encoder/DecoderInterface.php#L33-L35 but the YamlEncoder::decode could throw ParseException. Others similar Encoder, like XmlEncoder or JsonEncoder are throwing NotEncodableValue so I did the same cf example https://github.com/symfony/symfony/blob/da155534524f0d9c501023d10bb423fce4cd4482/src/Symfony/Component/Serializer/Encoder/JsonEncode.php#L41-L45 Commits ------- ab9de2d Fix exception thrown by YamlEncoder
That is certainly not what this annotation is about. Exceptions are for cases where something unexpected happens. In some cases, they're for signaling rare situations that shouldn't happen but can happen (the unhappy path). So to me: |
I tried doing better by adding
Would it be ok like this or should I be more explicit @nicolas-grekas ? |
121e67d to
a6aeb65
Compare
|
Thank you @VincentLanglet. |
This
SerializerInterface::serialize can throws
Since it might be a lot of exception and there is no real need to list them all, I simplified with
@throws ExceptionInterface.