Catch trigger_error generated errors

[GoogleCodeExporter]

Purpose of code changes on this branch:

"Catch" trigger_error() generated errors in SimpleSAML_error_handler(), and 
throw SimpleSAML_Error_Error(). trigger_error() is used in OpenID library, and 
as yesterday Yahoo had problems with the OpenID, E_USER_ERROR was generated 
which caused the "white screen of death".

https://groups.google.com/d/msg/simplesamlphp/pSmsnm-cl5I/f-nBvN83lMsJ

http://developer.yahoo.com/forum/OpenID-General-Discussion/OpenID-login-with-Yah
oo-broke-today/1386713784256-49fd81b6-7752-4c9f-8b7c-728235c46bae/1386716497918

When reviewing my code changes, please focus on:

-

After the review, I'll merge this branch into:
/trunk

Original issue reported on code.google.com by comel...@gmail.com on 12 Dec 2013 at 1:33

Attachments:

• trigger_error.patch

[GoogleCodeExporter]

I don't think it is correct to catch these errors.

From what I can tell about the documentation, there is no reason for program 
execution to stop after such an error, and in some cases it doesn't. It looks 
like E_USER_ERROR aborts program execution, but it still feels wrong to catch 
that one and display an error page. It would be like trying to catch E_ERROR.

I'm tempted to say that the proper place to fix this is in the OpenID library.

Original comment by olavmrk@gmail.com on 13 Dec 2013 at 12:32

[GoogleCodeExporter]

As you probably saw in documentation, E_USER_ERROR is just like E_ERROR 
(fatal), just that it's user-generated with trigger_error(), i.e. similar as 
the user generated exception, something that you most certainly want to catch 
and display some error not generate fatal error.

May I ask how do you suggest to fix the OpenID libary? Because I presume that 
they use trigger_error for a reason, and not e.g. exceptions.

Original comment by comel...@gmail.com on 13 Dec 2013 at 12:50

[GoogleCodeExporter]

I suspect they use trigger_error() for historic reasons -- after all, that code 
is / was written for PHP 4, and it shows in a lot of places.

As I said, trying to catch E_USER_ERROR looks like trying to catch E_ERROR, but 
they are supposed to be fatal errors. Handling them specially just looks wrong, 
and it feels like papering over a problem in the OpenID library.

Unfortunately, the PHP OpenID library is really suffering from a lack of 
maintenance, so whether a patch will be accepted or not is difficult say.

Original comment by olavmrk@gmail.com on 13 Dec 2013 at 1:00

[GoogleCodeExporter]

It's not the same as E_ERROR, it's user generated, and IMO it should be catched 
(only E_USER_ERROR, warning and notice are not fatal).

It's not the question whether the patch will be accepted, but what the patch 
would look like...

Original comment by comel...@gmail.com on 13 Dec 2013 at 1:10

[GoogleCodeExporter]

The documentation says that it is "like E_ERROR", which is why I think the 
behavior of E_ERROR is appropriate. 

If it is supposed to be catched, why not use an exception?

Original comment by olavmrk@gmail.com on 13 Dec 2013 at 1:26

[GoogleCodeExporter]

The crucial part is "like", because if it were the same then there would be no 
E_USER_ERROR, do you agree? E_USER_ERROR is supposed to be triggered by the 
user, and stop the execution, but in normal application some "nice" error 
should be displayed to the user.

Yes, and then? Change all the applications where the OpenID library is used?

Original comment by comel...@gmail.com on 13 Dec 2013 at 1:35

[GoogleCodeExporter]

The reason there is an E_USER_ERROR is the same reason there is E_USER_NOTICE, 
E_USER_WARNING and E_USER_DEPRECATED. As for the acutal reason, that is beyond 
me :) It's PHP, and they decided that trigger_error() should only work with a 
specific set of error constants. I assume they added it as some sort of poor 
mans replacement of proper exception handling infrastructure, which didn't 
arrive until PHP 5.

As for what the proper solution is, I really don't know. I just feel that 
starting to handle trigger_error generated fatal errors in simpleSAMLphp is 
simply papering over the problem.

Original comment by olavmrk@gmail.com on 13 Dec 2013 at 2:09

[GoogleCodeExporter]

Yes, this is an easy, cheap fix, nothing more, and you are saying that we 
should make a big, and time expensive change on OpenID library to fix it?

Original comment by comel...@gmail.com on 13 Dec 2013 at 2:15

[GoogleCodeExporter]

Easy, cheap and wrong fix.

Even if it isn't much code, it does add code that must be maintained, and once 
it is added it is hard to remove.

(I have papered over some problems in php-openid earlier, and I am still 
regretting that decision. I do not wish to repeat it again.)

I guess what I am saying that it should be treated as a bug in the php-openid 
library. If it isn't possible to fix it easily, we will have to live with it.

Original comment by olavmrk@gmail.com on 16 Dec 2013 at 7:45

[GoogleCodeExporter]

OK, I'm closing this.

Original comment by comel...@gmail.com on 16 Dec 2013 at 8:42

• Changed state: WontFix