The errorHandler class suppresses output of custom MyBB error types

[Laird]

This is a tentative bug report, because I might be misunderstanding or missing something.

In debugging a friend's board, which was intermittently showing a white screen, it turned out that a database error was being generated and sent to the error handler class's error() method by line #609 of inc/db_mysqli.php:

				$error_handler->error(MYBB_SQL, $error);

That method of the error handler, however, returned immediately due to the condition on line #171 of inc/class_error.php testing true, and so my friend never saw the error message, and saw only a white screen:

		if((error_reporting() & $type) == 0)

It seems to me that errorHandler::error() ought not to return in this scenario, and that the problem is that this conditional ignores custom MyBB error types: it will always test true for them, causing the method to return without displaying anything for those error types.

Unless I'm mistaken or missing something, it seems that lines #170-175 could safely be deleted in their entirety, because lines #176-179 perform the same job, but without mistakenly testing true for MyBB error types: by this (performing the same job) I mean that error_reporting() is in the first place initialised based on $this->ignore_types, so testing it aside from $this->ignore_types seems to be redundant.

Lines #170-175, which seem redundant:

		// Error reporting turned off for this type
		if((error_reporting() & $type) == 0)
		{
			return true;
		}

Lines #176-179, which seem to make them redundant:

		if(in_array($type, $this->ignore_types))
		{
			return true;
		}

Am I missing something here?

Unless somebody points out that I am, I'll update my Debugging white screens thread with the advice to delete those lines if the other advice in that thread's OP fails.

[Devilshakerz]

Apparently a regression from https://github.com/mybb/mybb/pull/4386/c...34482534b7, originally suggested in https://github.com/mybb/mybb/issues/4409

Looks like it wouldn't cause problems for usual values of the error reporting level, which would be higher than the arbitrary integers for custom errors, given the faulty bitmask check.

[Laird]

Looks like it wouldn't cause problems for usual values of the error reporting level

Yes, so, digging deeper, I found that the root cause was that commented out on my friend's board was the Tapatalk plugin's call, in global level code, to error_reporting(E_ALL ^ E_WARNING), just after it had called error_reporting(0).

It seems to me that plugins should not call that function unilaterally, perhaps unless to do so only temporarily, afterwards restoring the original value (Tapatalk does not restore the original value: it sets its own as shown above).

What do others think?

Apparently a regression from https://github.com/mybb/mybb/pull/4386/c...34482534b7, originally suggested in https://github.com/mybb/mybb/issues/4409

Here are two potential (but untested) rewrites of the line, depending on how we want to interpret an error_reporting value of zero (whether it does, or does not, imply that custom MyBB errors, too, are suppressed).

First, error_reporting level of zero implies suppression of custom MyBB errors too:

		if(error_reporting() == 0 || ((error_reporting() & $type) == 0 && !in_array($type, $this->mybb_error_types)))

Second, custom MyBB errors are never suppressed based on error_reporting level (my preferred approach):

		if(!in_array($type, $this->mybb_error_types) && (error_reporting() & $type) == 0)

Thoughts?

[Devilshakerz]

It probably makes sense to only check for PHP's inbuilt error types (so as in the second snippet), especially if MyBB errors that should be fatal may be accidentally left ignored without terminating execution.

For MyBB errors specifically, there should be better control with catchable database exceptions, and error handler options - both can be extended if needed.

[Laird]

It probably makes sense to only check for PHP's inbuilt error types (so as in the second snippet), especially if MyBB errors that should be fatal may be accidentally left ignored without terminating execution.

Good, we agree on this. I'll probably create an issue + PR for this on GitHub at some point unless you or somebody else gets onto it first. It seems that the process of evaluating threads in this forum to either push or reject has mostly stopped functioning.

For MyBB errors specifically, there should be better control with catchable database exceptions, and error handler options - both can be extended if needed.

I did remember that you'd done some good work on error-handling for 1.9, but had forgotten those details.