Any method everywhere might throw some type of\Exception. I'm personally not convinced this phpdoc adds any value (and if it does, we should add it everywhere?).

Any method everywhere might throw.

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
https://phpstan.org/blog/bring-your-exceptions-under-control

Currently, with code like

try {     $serializer->serialize(...);} catch (NotEncodableValueException) {}

I'm getting

Dead catch - NotEncodableValueException is never thrown in the try block.

because the PHPdoc is not describing the exceptions thrown.

some type of\Exception.

Notice I didn't added\Exception but used the scoped interfaceSymfony\Component\Serializer\Exception\ExceptionInterface which already provides some value to

I'm personally not convinced this phpdoc adds any value

It adds value for static analysis tools (like PHPStan) users.
It also provide such value for IDE (like PHPStorm) users.

I'm personally not convinced this phpdoc adds any value (and if it does, we should add it everywhere?).

@throws ExceptionInterface exists on the NormalizerInterface

And a@throws tag exists on the EncoderInterface

I don't see a reason to add it to SerializerInterface which is a similar interface.

For reference, I already added missing@throws tag in some other class/interfaces likehttps://github.com/symfony/symfony/pull/57519/files orhttps://github.com/symfony/symfony/pull/52915/files

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?

I agree with@wouterj. If we think it's useful, we should do it consistently across the board, not just on one interface.

There is already a looot of method/class/interface already with@throws annotation.
And every times I find one missing I open (and still will open) a PR to fix it.

Maybe it's mostly useful for interfaces?

Since people works mainly with interface, it is.

This also explains which exceptions we are supposed to throw when implementing the interface.
Here customSerializerInterface::serialize implementation should only throwSymfony\Component\Serializer\Exception\ExceptionInterface.
And the Symfony Serializer implementation respects this contract.

It throws:

• Symfony\Component\Serializer\Exception\UnsupportedFormatException
• Symfony\Component\Serializer\Exception\RuntimeException
• Symfony\Component\Serializer\Exception\LogicException
• Symfony\Component\Serializer\Exception\NotNormalizableValueException
• All the exceptions thrown byNormalizerInterface::normalize (which includes ExceptionInterface)
  

  symfony/src/Symfony/Component/Serializer/Normalizer/NormalizerInterface.php

  Lines 33 to 37 in78f4d9a

  	* @throws InvalidArgumentException Occurs when the object given is not a supported type for the normalizer
  	* @throws CircularReferenceException Occurs when the normalizer detects a circular reference when no circular
  	* reference handler can fix it
  	* @throws LogicException Occurs when the normalizer is not called in an expected context
  	* @throws ExceptionInterface Occurs for all the other cases of errors

AndSerializerInterface::deserialize throws

• Symfony\Component\Serializer\Exception\UnsupportedFormatException
• Symfony\Component\Serializer\Exception\RuntimeException
• All the exceptions thrown byDenormalizerInterface::denormalize (which includes ExceptionInterface)
  

  symfony/src/Symfony/Component/Serializer/Normalizer/DenormalizerInterface.php

  Lines 37 to 43 in78f4d9a

  	* @throws BadMethodCallException Occurs when the normalizer is not called in an expected context
  	* @throws InvalidArgumentException Occurs when the arguments are not coherent or not supported
  	* @throws UnexpectedValueException Occurs when the item cannot be hydrated with the given data
  	* @throws ExtraAttributesException Occurs when the item doesn't have attribute to receive given data
  	* @throws LogicException Occurs when the normalizer is not supposed to denormalize
  	* @throws RuntimeException Occurs if the class cannot be instantiated
  	* @throws ExceptionInterface Occurs for all the other cases of errors

But then, do we want to reference the interface or the concrete exception implementations that can be thrown?

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
@throws ExceptionInterface "fallback" (like

• The SerializerInterface use the Normalizer/DenormalizerInterface which can throw any of these exceptions
• The@throws on interfaces should be considered as "contract" about which exceptions are allowed to be thrown by custom implementation. And this interface should IMHO allows every scoped ExceptionInterface.

I considered it was already a good step to add@throws ExceptionInterface but do you prefer to be more explicit@fabpot ?

Thinking about this more, I'm actually thinking the@throw annotations can be "harmful" (and so is the PHPstan rule imho, if not all vendors use it).

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.
In this case, ourSerializer::serialize() will call normalizers. Maybe Symfony doesn't include any normalizer that throws something other thanExceptionInterface, but there is no way in knowing if any third party library and application normalizer conforms to this as well.
The same also applies to other implementations of theSerializerInterface out there. We never told them to only throwExceptionInterface exceptions, so we can't make this contract to our users. That will wrongly inform them that they can completely ignore serializer exceptions by catchingExceptionInterface, which will lead to broken applications when an unexpected exception occurs.

I would say this means that adding@throw on interfaces might be much more dangerous than adding them on actual implementations.

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:

useSymfony\Component\Serializer\Encoder\YamlEncoder;useSymfony\Component\Serializer\Normalizer\ObjectNormalizer;useSymfony\Component\Serializer\Serializer;$encoders = [newYamlEncoder()];$normalizers = [newObjectNormalizer()];$serializer =newSerializer($normalizers,$encoders);$serializer->deserialize('[1','stdClass','yaml');// throws Symfony\Component\Yaml\Exception\ParseException

Thinking about this more, I'm actually thinking the@throw annotations can be harmful (and so is the PHPstan rule imho, if not all vendors use it).

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@throws means it can throws anything, but it provides a stricter option to say that no@throws means never throw. It has to consider that itthrows anything because of the big number of poor documented method/interface, but when using well documented API, it's powerful to report dead-catch which are often code smell or possible bugs. I already solve multiple errors because of it (like the fact of not catching the right exception).

Even more, retrospectively adding it to methods creates a new contract for any implementation that they were never aware of. [...] We never told them to only throwExceptionInterface exceptions, so we can't make this contract to our users.

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 ifnone is documented

A better documentation would lead to a better try/catch.

That will wrongly inform them that they can completely ignore serializer exceptions by catchingExceptionInterface, which will lead to broken applications when an unexpected exception occurs.

Currently the interface is wrongly informing that no exception are thrown.
Adding a@throws Throwable would be the minimum needed changes but so far

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

The issue is not about the SerializerInterface contract but about the YamlEncoder whichalready does not respect the EncoderInterface contract.

This YamlEncoder was introduced in Symfony 3 while the@throws annotation exists since Symfony 2.
When looking at other Encoder, a lot of them are throwingNotEncodableValueException extends UnexpectedValueException in such situation.

So I would think the YamlEncoder need to be fixed.
I opened the PR#59323, and will be happy to add more fix if needed.

They are documenting a contract ("this method will only throw things that are instance of X") while there is no way to enforce that

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).@throws is for those cases: it's augmenting the API with erroneous situations which, from a domain pov, can be caught and deal-with. Any other kind of exceptions can be thrown, but should be unhandled, unless specific reasons. (It should also be fine to leave API-defined exceptions unhandled, unlike what some IDEs think).

So to me:@throws should be crystal clear about when catching can make sense, from a domain pov.
A generic annotation with the interface doesn't fit this requirement to me and feels useless.
We should do better.

So to me:@throws should be crystal clear about when catching can make sense, from a domain pov. A generic annotation with the interface doesn't fit this requirement to me and feels useless. We should do better.

I tried doing better by adding

• NotEncodableValueException
• NotNormalizableValueException

Would it be ok like this or should I be more explicit@nicolas-grekas ?
Having0 throws annotation is not ok to me, but I don't think it will be better to list dozen of throw annotation either so I'm trying to find the right number.

Thank you@VincentLanglet.