[Proposal] Error handler

[Erin Millard]

Hi everyone,

Following discussion in a thread I created earlier, I decided to draft a proposal for an error handler standard. That draft is now a PR available for review at https://github.com/php-fig/fig-standards/pull/262.

I feel that standardized error handling would be a great move forward for the FIG, and I'm keen to hear your opinion on the spec. If you wish to contribute to the discussion, please be sure to read both the spec and the meta document, as the meta document contains important details of how the spec is intended to be utilized.

Thanks!

[Alexander Makarov]

Very well written. Thank you very much for putting it all together.

I've left my comments inline in a pull request because it would involve too much quoting mentioning it here in the list. Both are about meta. PSR itself looks perfect.

I'd vote +1 for it when voting is opened.

[Erin Millard]

Thanks for the feedback! I'll respond to your individual points on the PR itself :)

[Jason Judge]

Looks great :-)

One thing I have found, is that many packages can behave slightly differently depending on whether there is an error handler throwing exceptions or not. I wonder if a package could declare in some way whether it must or can have an error handler. A package that can operate consistently with or without an error handler will be using @ to suppress errors, while a package that requires an error handler will not be using @, but will have its own exception catchers (try...catch) which it will rely on to catch errors. I  guess what we don't want is for a package to bring its own error handler along or to be dependent on a specific error handler.

Maybe this is just outside the scope of PSR, and the suggestion for composer to support metadata to declare the requirement (or not) for an error handler is the way to get the idea out into the wild.

Have you compared the proposal to how other common error handlers work now? I am wondering if they all more-or-less do the same thing now.

Would it be worth combining these two points:

• The error handler MUST throw an exception of type ErrorException when an error occurs unless stated otherwise by this document.
• Exceptions thrown MAY be a subclass of ErrorException.

It feels like one statement that has been split apart, resulting in the first part having to reference the second part. Something like this maybe:

• The error handler MUST throw an exception of type ErrorException or a subclass of ErrorException.

Not sure if that needs qualifying more, to state in which circumstances it throws the exception, i.e. "...when handling an error", or if that is obvious enough. Is that more strictly \ErrorException?

-- Jaosn

[Larry Garfield]

I'm unclear on what you're asking for.

Are you asking about interest in an error handling PSR in general?  If so, I'm +1 in concept.

Are you asking for an Entrance vote on this proposal?  If so, you need to get a working group together with an Editor (presumably you), Coordinator, and Sponsor.

Are you asking for specific feedback on this proposal?  If so, I think it still has significant problems.

1) The spec itself is not stand-alone.  It makes little sense without reading the meta doc.  That's not how PSRs are supposed to work.  Rather, the spec itself must stand alone.  The meta doc is just for background and "wait, why did we do that?" type information.  As written, the meta doc is more of a "best practices guide", with the spec as an addendum.

2) The metadoc/spec is Composer-dependent.  PSR specs cannot depend on Composer for their implementation.  We can certainly talk to Jordi about easing implementation of some specs, and we have (eg, PSR-4), but no spec should be Composer-dependent.

3) The spec portion seems to imply that everyone MUST always use ErrorException for all the things(!).  I disagree.  Many PHP native functions file trigger errors for things that make little sense, but converting those to exceptions across the board is needlessly sledge-hammer-ish.  I have done that before on a project and then spent an unholy amount of time trying to deal with htmlspecialchars() triggering notices when it encountered not-quite-perfect UTF-8 strings.  That turned what should have been a minor bug into a system-breaking issue.  Saying "don't use trigger, just use exceptions" is all well and good for userspace code (and I support that), but we can't get away from native functions, and a sledgehammer approach is not sufficient.

--Larry Garfield

[Erin Millard]

Thanks for the comments Jason.

The issue of requiring a PSR-X error handler vs. being *capable* of working with one is covered to some degree in the meta document. It is possible to write code that behaves the same under both PSR-X and traditional error handlers, even though it can be tedious. The meta document currently suggests that when this is the case, the package can simply *not* use an explicit error handler requirement to indicate that it can work in either circumstance. The issue has been raised that this may not be a safe default, however, as many package writers will just not care enough to make their requirements explicit.

I will try to find the time to do a detailed analysis of how the major players are handling their errors. I've personally looked into Symfony and Laravel at least, and I know that their approach is very close to what the spec details. They do change behavior based on the configured error reporting level, which is slightly different, but I think that's a poor decision (for the reasons why, please see my reply to Larry Garfield's email).

In regards to combining those two lines, the 'unless stated otherwise by this document' part is not referring to the subclassing of ErrorException, but to the fact that the handler must not throw an exception for deprecation messages, and when error suppression is active. To combine them would result in something like:

'The error handler MUST throw an exception of type ErrorException, or a subclass of ErrorException, unless stated otherwise by this document.'

which I think is more confusing, as it's unclear to which part the 'unless' portion refers. It could be interpreted to mean that the spec allows for a completely different exception class in some circumstances, which is not the case.

Adding the namespace separator before the ErrorException class name is probably more explicit, and better, you're right. I'll fix that up soon.

- Erin

[Erin Millard]

Thanks for the feedback Larry.

To be clear, I'm not suggesting an entrance vote is appropriate at this point in time. I simply felt that the most direct route to useful discussion would be to put an idea on the table and get some immediate feedback.

I disagree that the spec is not stand-alone. The meta document is quite heavy, it's true, but this is not dissimilar to PSR-4, where the spec itself is succinct, and the meta-document has a history, justification, alternative approaches for its utilization, and discussion of the role that Composer plays. Most importantly, the main spec document has significant value in and of itself. It describes very specifically and clearly the behavior of a standard error handler; which at the end of the day, is what this is really about.

I want to very clear about this, also. The main spec is not Composer-dependent. The meta-document does discuss possible solutions revolving around Composer, but they are in the meta document, and not the main spec, for this very reason. Discussing the impact of this spec on the de-facto standard dependency management solution of the PHP community seems like a valuable thing to me. Perhaps the meta document isn't the right place for this, but it should certainly be discussed somewhere.

In response to your criticism of error exceptions as an across-the-board solution, I'm certainly open to suggestions for to how to address the issues you raised. Perhaps something revolving around the current error reporting level could be considered, as that seems to be the solution adopted by Symfony and Laravel. I don't like the idea for a few reasons. Most importantly for this group, it weakens the interoperability guarantee. If the error handler is configurable, saying that you require a 'PSR-X' error handler immediately means much less. Whereas before you knew exactly how a 'PSR-X' handler will behave in all cases, it now depends on what state the error handler has been left in by someone else. It also adds complexity to the implementation, relies on global state, and in practice would most likely require setting the error reporting level before/after individual function calls.

The issue you raise with htmlspecialchars() certainly requires a solution - sometimes you do get stuck with input that's not 100% within your control. I really hate to suggest it, and I do say this with trepidation, but in that circumstance I think it's probably appropriate to use error suppression. Its negative performance effects are negated if using a PSR-X handler, and at the end of the day, you *are* calling a function with invalid input. I don't think it's wise to weaken the spec to better support invalid input use cases.

It's also important to note that if error exceptions are not an appropriate solution, you can simply not conform to the spec, and use traditional error handling. My suggestions in the meta document allow for this.

- Erin

[Jason Judge]

On Monday, 3 March 2014 06:05:58 UTC, Erin Millard wrote:

...

In regards to combining those two lines, the 'unless stated otherwise by this document' part is not referring to the subclassing of ErrorException, but to the fact that the handler must not throw an exception for deprecation messages, and when error suppression is active. To combine them would result in something like:

'The error handler MUST throw an exception of type ErrorException, or a subclass of ErrorException, unless stated otherwise by this document.'

which I think is more confusing, as it's unclear to which part the 'unless' portion refers. It could be interpreted to mean that the spec allows for a completely different exception class in some circumstances, which is not the case.

Maybe I'm misunderstanding what those statements mean. I took it as specifying what exceptions must be raised when an exception is raised, rather than what should happen at all times. Could you expand it a little for my tiny brain? Is it saying that any exception raised MUST be derived from ErrorException, except when it's not? Or is it saying that exceptions MUST be raised in the event of an error or warning, except when they are not? Or perhaps it is trying to cover both bases, or neither (and I am way off track)? Either way, I am wondering if "MUST" is aimed at the wrong things (they MUST except when then DON'T;-)

On to Larry's comments, I saw that you very clearly kept composer dependencies out of the main spec, so I don't think there is a problem there. It does highlight how the status of the meta document is seen though - just what does the meta document cover and what should it keep clear of (I've looked, and I can't see where the scope of a meta doc is defined)?

I also wonder if there should be some clear statements at the top of each document to make it clear what stage it is in? It looks like a full proposal, and that could confuse some people (inside and outside of the FIG). Maybe a sentence at the top to state it is for discussion only and is not "official" in any way? I'm coming at this from outside the FIG, and it looks great to me, but I understand the noise levels need to be kept down until a proposal takes on an official place in the FIG. At which point, I'll shut up now :-)

-- Jason

[Erin Millard]

I'm pretty sure you had it right originally, but to put it in less spec-ish terms, if the handler throws an exception it must be an ErrorException or something that extends from it. There's never a case when throwing a different type of exception is okay. There are cases when the handler should not throw an exception at all, and that's where the 'unless stated otherwise by this document' comes in.

Originally I had worded the spec rules to include what boolean values the handler should return when it doesn't throw an exception (this was removed before I submitted the PR), and I think that first rule made more sense in context then. How about the following two statements instead?

• The error handler MUST throw an exception when an error occurs unless stated otherwise by this document.
  
• Exceptions thrown MUST be of type \ErrorException or a subclass thereof.

Regarding scope, there is a defined list of goals at the top of the document. I haven't defined what is outside the scope, but that's something that I could add.

You might be right about needing to add some status information, at least to the meta document. If a voting member could advise what the preferred process is here, that would be helpful. I'm simply following the instructions at https://github.com/php-fig/fig-standards#proposing-a-standards-recommendation.

[Karsten Dambekalns]

Hi.

> Exceptions thrown MUST be of type \ErrorException or a subclass thereof.

For me, an exception is *always* an error. It is an exceptional case, something that should never happen. So, an exception subclasss \Exception (who’d have thought) is enough, IMHO.

Could you give an example[1] of when an exception is not an error?

Regards,
Karsten

[1] Yes, we have https://git.typo3.org/Packages/TYPO3.Flow.git/blob/HEAD:/Classes/TYPO3/Flow/Mvc/Exception/StopActionException.php but that is the only exception to the "\Exception rule” I can come up with, and we would use something else if we had a clever idea.

[Jason Judge]

On Tuesday, 1 April 2014 10:04:29 UTC+1, Karsten Dambekalns wrote:

Hi.

> Exceptions thrown MUST be of type \ErrorException or a subclass thereof.

For me, an exception is *always* an error. It is an exceptional case, something that should never happen. So, an exception subclasss \Exception (who’d have thought) is enough, IMHO.

Could you give an example[1] of when an exception is not an error?

One use that comes to mind, is when processing many records with a time limit. An exception could be raised deep inside the loop to tell the process "time is just about out; clean up and get out now". Importing records, sending bulk email, are the kinds of processes I'm thinking.

I realise that there are other ways this can be implemented, but as a way to quickly jump out of multiple nested levels of code, perhaps using a library or package to do this, it can be useful. But the point is, you can do this, and that exception would not be an error.

-- Jason

[Sebastian Krebs]

2014-04-01 11:04 GMT+02:00 Karsten Dambekalns <kar...@dambekalns.de>:

Hi.

> Exceptions thrown MUST be of type \ErrorException or a subclass thereof.

For me, an exception is *always* an error. It is an exceptional case, something that should never happen. So, an exception subclasss \Exception (who’d have thought) is enough, IMHO.

Could you give an example[1] of when an exception is not an error?

If you have an error, something is broken, but Exceptions can get caught and "fixed" during runtime. If thats not possible (uncaught Exception), than they should be treatened as errors, yes, but that doesn't make them equivalent.

As a rule of thumb: Uncaught exceptions are errors.

 

Regards,
Karsten

[1] Yes, we have https://git.typo3.org/Packages/TYPO3.Flow.git/blob/HEAD:/Classes/TYPO3/Flow/Mvc/Exception/StopActionException.php but that is the only exception to the "\Exception rule” I can come up with, and we would use something else if we had a clever idea.

--
github.com/KingCrunch

[Evert Pot]

On 2014-04-01, 5:04 AM, Karsten Dambekalns wrote:
> Hi.
>
>> Exceptions thrown MUST be of type \ErrorException or a subclass thereof.
>
> For me, an exception is *always* an error. It is an exceptional case, something that should never happen. So, an exception subclasss \Exception (who’d have thought) is enough, IMHO.
>
> Could you give an example[1] of when an exception is not an error?

I think you misunderstood. PHP's built-in ErrorException[1] is
specifically used to wrap PHP errors as PHP exceptions.

Exceptions _should_ only be used when there' errors, or exceptional
cases, but ErrorException does not refer to an Error as an abstract
concept, but rather specifically "errors" as they appear in PHP.

[1]: http://www.php.net/manual/en/class.errorexception.php

[Nicolas Grekas]

Hi eveyone,

FYI, I justed posted this issue on the proposal: https://github.com/ezzatron/php-error-handler/issues/19

E_RECOVERABLE and E_USER_ERROR are special in that they are considered fatal by the native PHP error handler, even when @-silenced.

Thus, the only viable option for a user error handler is to always throw an exception for these two types, no matters the error_reporting() nor anything else.

This strategy would ensure that code paths are correct, but also that the application can continue if it decides to handle the exception.