-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Serializer] Add missing @throws annotations #18854
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
Conversation
theofidry
commented
May 24, 2016
Q | A |
---|---|
Branch? | 2.3 |
Bug fix? | yes |
New feature? | no |
BC breaks? | no |
Deprecations? | no |
Tests pass? | yes |
Fixed tickets | #18798 |
License | MIT |
Doc PR | - |
@weaverryan 'dat Carson 👍 |
👍 |
LGTM 👍 |
@@ -32,6 +32,8 @@ public function __construct(array $decoders = array()) | |||
|
|||
/** | |||
* {@inheritdoc} | |||
* | |||
* @throws RuntimeException If no decoder is found. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the point in specifying @throws
for a concrete class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO you should always show which exception you're throwing, concrete class or not.
Theoretically one could want to extend/decorate the ChainDecoder
specifically and in that case you may need the concrete class implementation details (if you don't then it would be better to typehint the interface instead obviously).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@theofidry listing all exceptions explicitly is unmaintainable, if you account for all your dependencies. Any method in PHP can throw an exception when something goes wrong.
they deserve to be documented this way only when there is something useful about it (specific exception classes being used for specific errors which are likely to be interesting to be catched by the caller). @throws Exception
in the DecoderInterface does not bring anything useful IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
listing all exceptions explicitly is unmaintainable
Fair, will remove it then.
@throws Exception in the DecoderInterface does not bring anything useful IMO.
I beg to differ, if you're typehinting against interface and no @throws
tag is given in one of its method, it's only fair to not expect an exception throw by it (unless you really messed something up). Why DecoderInterface
shouldn't benefit from the @throws
tag as any other class? It's not less valuable or critical than another interface. Also not having it make it only confusing if you're implementing your own decoder: I am allowed to throw an exception? Must it be a specific one? Not adding this @throws
condemn you to check how other decorders or the Serializer
works to find an answer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my case is not specific to DecoderInterface. It is true for any part of Symfony. I just took this one as example among your changes.
Documenting @throws
in the interface is fine when the interface wants implementations to throw specific exceptions for some error cases. A plain @throws Exception
means nothing.
And this does not forbid other exceptions to happen when something goes really wrong (would you then also document things like @throws \TypeError in case you don't respect the typehint
to be complete ?). Thus, if your goal is that any possible exception throw by calling the method appears in the phpdoc, it means you have to account for all dependencies too, which gets unmaintainable very quickly (or you have to put try/catch everywhere to hide exceptions of your dependencies, even though they would appear only when your code is really buggy and so should be handled only by the global error handler displaying a 500 page and logging the error).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@throws Exception doesn't help here anyway. If you handle specific exception, you know internals of Serializer.
I'm not following, to me there's a big difference between \Exception
and \Symfony\Component\Serializer\Exception\Exception
, because with the later I know from where the exception comes from and I may handle things differently knowing that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How you can handle differently if only single exception class is specified?
You can handle differently if there are multiple @throws
statements, otherwise you break encapsulation.
I mean, you don't know what's wrong with serializer. "Something" is wrong. That doesn't help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simple, instead of doing catch (\Exception $e)
I can use catch (SerializerException $e)
and have different things in each catch block. How does that break encapsulation?
I mean, those changes simply says that the serializer & co. can throw this type of exception, which is already the current behaviour (but undocumented).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we talk about different things at the same time.
How does that break encapsulation?
I was talking about catching subclasses of SerializerException
: you either break encapsulation (coupling with implementation details) or an interface is bad-designed. If you didn't mean it, I'm sorry.
@throws SerializerException
means only that serializer failed. Why did it fail, because user input data is invalid or because configuration is wrong? Nobody knows and it doesn't help handle these cases differently. That's what I call "handle differently".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was talking about catching subclasses of SerializerException
Ah no I wasn't talking of this one. I find it useful as a doc and helps if you are extending the Serializer in a very coupled way which would break encapsulation. But as I said I personally document it because it's something easily done with PhpStorm at my level and I like saying which exceptions I'm manually throwing, but I perfectly understand that it's not something everyone want to do and would be unmaintainable at a Symfony scale project.
@throws SerializerException means only that serializer failed.
Precisely, whereas if you only has \Exception
you have no idea of what failed. Well, to put it another why, there reason why not documenting it bothers me is that I don't assume each function call I'm making throws a \Exception
exception (unless it's a critical part of the app where no failure is allowed). And if an exception is thrown, unless documented, I would expect it to be because something really unusual/weird that need to be fixed immediately. So the current behaviour make it very intolerant: Serialization failure is a critical and unexpected error. Thing is, serialization failure is quite common, and unless you dive in the source code, you might get caught by surprised to get an exception at this step, hence this piece of doc to warn the user that this can fail, that it's something you should account for.
3a3d8a3
to
4a79042
Compare
Thank you @theofidry. |
This PR was squashed before being merged into the 2.3 branch (closes #18854). Discussion ---------- [Serializer] Add missing @throws annotations | Q | A | ------------- | --- | Branch? | 2.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #18798 | License | MIT | Doc PR | - Commits ------- b73400d [Serializer] Add missing @throws annotations
I would not have merged this one. Having a generic Besides, the |
Reverted on 2.7 in #18975 |