If some PHP method throws an exception, and we do not catch it, I think we should (and actually do?) add @throws already.
I started to add them also when it’s an “indirect throw”, and found it quite helpful. Because it tells you, that there is no error handling inside, and you should maybe add some.
Since we do not have anything related in the CGL, I wanted to get some input. Basically I see three options:
A: we only add @throws for thise exception thrown in the method directly, using throw
B: we always add any exception that the method may emit, because it’s not caught
C: we leave it open to the developer’s preference
My gut feeling would be C would be the most viable option, since it allows us to add those annotations, but does not force us to eventually adjust the whole codebase. This isn’t a PSR-2-style kinf of change, after all.
PS: Daniel mentioned the risk of those annotations becoming outdated if the called method changes and no longer throws an exception. I agree that this can happen, but imagine instead of @throws you’d need to adjust a try/catch block. Same story, yet noone would oppose adding try/catch for that reason. On the contrary, exactly because my IDE warns me about both cases, this is manageable today, more than it ever was.
Hey @kdambekalns thanks for bringing up the discussion and describing the pros and cons.
I personally don’t like when the comment states things which I then don’t find in the method itself. Additionally if we have the guideline (also not sure if we wrote that down somewhere) to use relative class names, we have use statements for exceptions which were actually never thrown.
I’m fine with both ways as I also understand the pros of adding the indirect throws.
But I don’t like the C option - we should at least have a guideline how we normally do it, to avoid such review discussions. That doesn’t mean to me that we need to adjust the complete code base but that we use it for new code / refactorings (like the array() syntax changes or the simple type annotations for example).
And - if we go for indirect throws - we should decide on how many method levels we search / let the IDE search for them.
Talked with Karsten about it. I also noticed over time that it can be very helpful to know all possible exceptions bubbling and not handled by a method, so option B. At some point we went for A as far as I remember, but I would also vote to change to B. yes those annotations might become outdated, but I rather have one case to much covered than one too less
I would say we should put a SHOULD do B in the CGL but not touch all the classes, but let that develop over time.
But, seriously: Yes, it would be useful if you’d know all the exceptions a method could throw but we’ll never get there by the help of annotations (some method down the chain might call a 3rd party library we don’t have control over - and even without that it’s not realistic that we keep it 100% in sync).
So hopefully the tooling around that gets better so that the IDE can tell you about it.
BTW: Java has the notion of “checked exceptions” that would “solve” the issue because you have to catch or annotate them – but C# left out that feature for similar reasons.
I think the current voted option B … is not doeable. What when you add an exception somewhere in the code … you need to crawl all the code to see if you need to update some PHP doc … a bit unrealistic I think.
I would strongly advocate to care about exceptions (errors in general) at the nearest point where you still have meaningful context. If callees of a method throw an exception, it should be catched, wrapped with some meaningful (and possibly structured) information and rethrown. Thus a method can have a @throws annotation that only exposes “its own” exceptions.
Thus you quite naturally get a hierarchy of errors where the exception message should ideally give you a lot of context without looking at stacktraces, which are sometime not meaningful (depending on the method arguments and how they are dumped) or really distracting to find the root cause of errors.
@christopher As mentioned above, I’m not sure if that discussion is relevant for the question about the annotation, but I think it’s an important topic so I gave it a shot at trying to “translate” some of the conclusions of the article you linked to the PHP world (possibly flawed cause I’m not into Go really):
Never inspect the output of error.Error
=> Don’t read the Exception.message via PHP it exists for humans, not code
Conclusion: avoid sentinel errors
=> Don’t use exceptions for non-exceptional cases.
Example: In the MVC framework we use a StopActionException and we should not
Conclusion: avoid error types
=> Avoid custom Exceptions - at least not as part of the public API (?)
Or is “error type” something different in Go?
Assert errors for behaviour, not type
In my interpretation (which is probably incomplete) this would slightly contradict the point above. But I think his example could be implemented like this:
interface Temporary
{
public function isTemporary(): bool;
}
public class SomeException extends \Exception implements Temporary
{
// ...
}
// ....
function isTemporary(\Exception $exception): bool
{
return $exception instanceof Temporary && $exception->isTemporary();
}
Annotating errors
I think this can be simulated in PHP with wrapped exceptions:
function readFile()
{
try {
// some I/O
} catch (\Exception $exception) {
throw new \RuntimeException('open failed', 1528450837, $exception);
}
}
function readConfig()
{
try {
readFile();
} catch (\Exception $exception) {
throw new \RuntimeException('could not read config', 1528450838, $exception);
}
}
I freshly re-installed PhpStorm and by default it complains about unhandled exceptions as you know.
Previously I had disabled this inspection because my German mind likes PHP classes without any warnings (and not only for cosmetic reasons, I have found some bugs this way that might have gone unseen if hidden beneath hundreds of other warnings and errors).
Long story short: I forced myself to leave the inspection on now and to satisfy them in my own code and it kind of changed my mind: It can be very useful to explicitly mark what kind of exceptions are possibly thrown.
My concerns regarding feasibility remain, but maybe it’s better to try and zap at least some gremlins rather than skipping it completely and run into exceptions you didn’t expect…
Documentation needs to be up to date, and as said earlier, we can not promise that this doc will stay correct. I’m all for improving our exception handling and removing some bad usage of exceptions. But adding all those @throw doc will not solve a lots of issue. As soon as we add an exception anywhere, we are in the need to update all the php doc comments everwhere… That’s a lots of load on the review process.
I’m really curious on how other framework (Symfony, …) handle this ?
I’m late to the game. generally I’d be in the B camp too, because I’m all for expressiveness and explicitness. But in this case I’m with Dominique and Christopher. This adds an unrealistic burden and (unfortunately) inevitably leads to inconsequential documentation, not solving the problem. Therefore I would suggest to go with Christopher’s suggestion - catch exceptions early and give context when rethrowing. Then annotate exceptions thrown in own method only. There are some edge cases where this is not sensible and especially in private methods one can ignore this rule IMO. Basically I would stick this rule to the public interface of all classes, i.e. methods that are called from the outside.
I voted for B, but at the same time find it a bit annoying that we now have a @throws StopActionException on the redirect method in action controllers (or have we had that for a while, and I never noticed?). This may actually lead ppl to catch it, which then results in your redirects no longer working. We might want to remove that @throws there, because it’s actually rather irritating, wdyt? The same applies to the ForwardException.
And forward docs say see redirect, so that’s not really a solution.
I guess that’s rather an issue of the IDE, which might suggest to catch this error and then someone unknowing doing it.
And yes, we should find a better way to stop execution of the dispatch process than throwing StopActionException/ForwardException. But only option I see is returning a StopAction/Forward object from those methods and hence require all $this->redirect/forward() calls to also return. Breaking change with lots of possible hard to debug errors, but we could already provide a code migration now and annotate the methods to actually return something. Then in 6.0 we add the breaking change with proper documentation for the remaining edge cases.
In PhpStorm some exceptions are ignored by default (i.e. \Error, \RuntimeException , \LogicException and their derivates.
So we could just make those “pseudo” exceptions extend one of those. (IMO it wasn’t the best idea to introduce a generic base exception in Flow that defeats semantics)
Alternatively you can add exceptions to be ignored in the “Languages & Frameworks / PHP / Analysis” options:
