The intent of that last part of the check is if there is a fatal error rendering the error.php template file to still display at least the error message that would be used by default in the template. Granted, many templates these days are not directly displaying the Exception message but use a more generic message or layout, but considering this is a message of last resort I don't know if I would make this part of the error handler aware of the error reporting configuration.

Hi mbabker,

I do think that a production setup in regards to security is far more important than the reason for allowing such errors to go public. What is the point of setting Error Reporting to none if we are to still allow errors to be reported ? And of course, if one wants to see the errors they would never set Error Reporting to None would they ?

The error reporting setting is actually for managing PHP's internal error reporting configuration, not related to whether Joomla will display errors built by its own code such as in the global exception handler. Otherwise you could argue that setting should disable the error page in full if you've turned off error reporting and the user should just see a blank screen on any error condition instead of a rendered template file.

To reach that specific output, you have to have a scenario where Joomla critically fails during its startup process (to the point an application isn't yet set in the factory, as you've pointed out) or the error page in the template tries to do something that results in an error. So it is legitimately a last resort effort to tell the user something went wrong. I don't think it should result completely in a blank screen as you propose here. If anything is going to be conditional, it should be whether the Exception message is displayed, but nothing more.

With 3.7 a database connection failure shouldn't result in triggering that code path thanks to #13074 (the reason it will do that now is because sessions are eagerly started in the application constructor which includes a SELECT query and potential INSERT query; this is now deferred until the application's execution begins which is after a point the application is set to JFactory).

Before I go and check the #13074 issue you gave me, a quick answer of mine is that, right after the message gets echoed, the code runs jexit(1); which will end the page anyway ? Correct me if I am wrong, and let me check the issue you mention.

It does run jexit() because that method is the global error handler so it is purposing shutting down the application. In truth I don't think that call is actually needed though (we shouldn't need to immediately call exit; but rather just let it return and let PHP finish the shutdown process).

I am not really sure in which cases JErrorPage will reach to execute echo $message; but to all the tests I have done so far, it does not take place on an html page (say on a 404), rather, it will always echo a message only, and just that (white browser background). Is that the case ?

Ways to reach that path:

• JErrorPage::render() receives a non-Exception/Throwable object as its parameter (it's not typehinted to allow PHP 7 Throwables to be handled and this can't be polyfilled in PHP 5)
• The logic to render a template's error.php file ends up throwing an Exception (the simplest way to mock this is to just add a throw new Exception('Testing error handling'); in that file). Other ways that might do it before trying to render error.php would be if JFactory::getApplication() or JFactory::getLanguage() result in an Exception being thrown.

Indeed. Then the DB errors should be handled differently.

One kinda dirty way is to also ask something like this:
(get_class($error) == 'JDatabaseExceptionConnecting')

But that would require that in the future all db errors call the same class.

Honestly, I don't think the handling when it reaches this point should differentiate based on the Exception subclass. If there is going to be any change (and personally I don't think it needs to be because this is a desperate attempt to tell the user something's broken instead of having a useless blank screen which wouldn't even get logged into PHP since it's either going to be a non-Exception object that would never trigger PHP's error logging or the result of an error thrown by Joomla rendering the page which won't log because the handler is catching the error), at most it should just be a conditional around displaying the Exception message (as in "Error displaying the error page" should always show up if you get to that point in the code).

Well we are not only talking about the admin or webmaster but also the visitor, which should not see db error messages that reveals operating system paths -- it is just very lame and proves no security measures are taken. Now once this condition is only at the very last part of this file, which only is about to echo() a message and then exit(), any logging has been done already anyway. So yes it is conditional around displayng the Exception message as you say, but only caring in this case of serious errors such as db ones.

In any case, If I was to do this to my own cms I would not just ask for the name of the class, I would rewrite the whole thing. This is why I consider this "if" statement there more of a dirty hack than of a serious approach.

If I knew that the file is not only responsible for serious errors but also renders less important ones I would maybe post a different approach all together.

Any error reaching this point in the code is serious and could result in a security leak. Filesystem errors as an example. Hence the reason I wouldn't do conditions based on Exception subclasses. We also have no idea what information is displayed in the Exception message, so saying any Exception type is blindly safe to render the message for isn't very accurate.

If there's an error rendering the error page (the entire try/catch block in the $isException condition at the start of that method), nothing gets logged to PHP for that error case because Joomla's catching that error and going to its fallback mechanism without either trying to log using our own API or an error_log(); call. So while the primary error triggering the page may have been logged to PHP and/or Joomla's logging mechanisms, the secondary error resulting in you hitting that section of code isn't.

So the only thing that should be conditional, if any conditional behavior based on error reporting is going to be introduced, should be the display of the Exception message. If it's off don't show it, but if error_reporting() > 0 then it should show the message regardless of type.

If I understand you well, in /includes/framework.php we get the error_reporting(0); by checking if $config->error_reporting is either none or 0. Which is my original approach but you were against it, no ?

The approach you suggested wrapped the entire echo statement in that type of check. I would only wrap https://github.com/joomla/joomla-cms/blob/staging/libraries/cms/error/page.php#L150-L160 so that "Error displaying the error page" is always echoed regardless of the error reporting configuration. That to me seems like an acceptable compromise, showing SOMETHING would be better than a blank screen.

Oh, one would have it set to System Default default and have it set to off or 0 in the vhost. So yeah the question should not be the global config setting, rather:

if ((int)ini_get('display_errors') != 0)

Agreed.

Ok that is a good compromise, I just tried it and I got:
Error displaying the error page

It now looks like this:

$message = 'Error displaying the error page';

if ((int)ini_get('display_errors') != 0)
{
	if ($isException)
	{
		$message .= ': ';

		if (isset($e))
		{
			$message .= $e->getMessage() . ': ';
		}

		$message .= $error->getMessage();
	}
}

echo $message;

page.tar.gz

I switched it around, this way it is faster (possibly by some nanoseconds):

$message = 'Error displaying the error page';

if ($isException)
{
	// Make sure we do not display sensitive data in production environments
	if ((int)ini_get('display_errors') != 0)
	{
		$message .= ': ';

		if (isset($e))
		{
			$message .= $e->getMessage() . ': ';
		}

		$message .= $error->getMessage();
	}
}

echo $message;

page.tar.gz

Haven't read the messages here but I have the PR so this can be closed if I am not mistaken.