3
\$\begingroup\$

I have this Library to control errors; I would like if you can evaluate it and make a review of the code, not about the goals of the library:

Everything works for me correctly, but I am also open to hear about any type of improvement that can be implemented in the code as long as it does not affect the objective of the library.


Introduction:

What does [BEH] do? [BEH] is a very simple PHP error handler implementation that throws ErrorException exceptions instead of using the default PHP error handling behaviour. This means that all runtime errors including Fatal are presented to the developer in the form of an exception. It also means that any unhandled errors are delivered to a single point: the global exception handler. also includes support for login errors with error document

Why use [BEH]? Developers need the ability to decide how their code behaves when an error occurs. Exceptions offer the only truly consistent way to report and recover from errors in PHP.

This method of handling errors has proven to be extremely effective. Similar strategies are used in major PHP frameworks such as [Laravel]. [BEH] is a standalone implementation that can be used for any project, and not required third party library or software.


Usage: [BEH] Configuration:

  • .htaccess: for Apache Version > 2.4.
  • .user.ini: for CGI/FPM/FastCGI.
  • error_conf.php: common configuration. Note: not tested in NGINX

Custom [BEH] for PHP And Server Error It must be configured manually in: .htaccess, .user.ini and conf.json depending on the scenario or desired host configuration; let's leave some examples:

  • Single host server.
  • Virtual Host and name of the github project.
  • Development with Xampp in Locally or other stack

.htaccess

# php_value auto_prepend_file "/var/www/html/error/init_error.php"
# php_value auto_prepend_file "/var/www/virtual-hostc/git-hub-project-name/error/init_error.php"
php_value auto_prepend_file "C:/xampp/htdocs/error/error_init.php"
Note: because of dependencies you must set the configuration inside <IfModule mod_php7.c> to work correctly; .htaccess file have an example

.user.ini

# auto_prepend_file = "/var/www/html/error/init_error.php"
# auto_prepend_file = "/var/www/virtual-host/git-hub-project-name/error/init_error.php"
auto_prepend_file = "C:/xampp/htdocs/error/error_init.php"

note: .user.ini files not use php_value statement.

Error Server must be setting up with directive in .htaccess file:
ErrorDocument 400 /error/error_server.php
Error_conf.php

skin of [BEH] must be: basic, bs4, custom.

Custom skin [BEH] you must implement two files skin like.

sk_custom_server_error.php
sk_custom_handler_error.php

Content of skin for [BEH] you must add the same variable print like basic skin, but you can use your own html design.

Skin are located in Skin


Project Structure

structure


Resource Code:

.htaccess File

<Files .user.ini>
    deny from all
</Files>
<Files *error.php>
    deny from all
</Files>
<Files server_error.php>
    Order Deny,Allow
    Allow from all
</Files>
<IfModule mod_php7.c>
    #php_value auto_prepend_file "/var/www/html/test1/error/init_error.php"
    php_value auto_prepend_file "C:/xampp/htdocs/dev/t2/BEH-Basic-Error-Handler/error/init_error.php"
</IfModule>
ErrorDocument 400 /error/server_error.php
ErrorDocument 401 /error/server_error.php
ErrorDocument 402 /error/server_error.php
ErrorDocument 403 /error/server_error.php
ErrorDocument 404 /error/server_error.php
ErrorDocument 405 /error/server_error.php
ErrorDocument 406 /error/server_error.php
ErrorDocument 407 /error/server_error.php
ErrorDocument 408 /error/server_error.php
ErrorDocument 409 /error/server_error.php
ErrorDocument 410 /error/server_error.php
ErrorDocument 411 /error/server_error.php
ErrorDocument 412 /error/server_error.php
ErrorDocument 413 /error/server_error.php
ErrorDocument 414 /error/server_error.php
ErrorDocument 415 /error/server_error.php
ErrorDocument 500 /error/server_error.php
ErrorDocument 501 /error/server_error.php
ErrorDocument 502 /error/server_error.php
ErrorDocument 503 /error/server_error.php
ErrorDocument 504 /error/server_error.php
ErrorDocument 505 /error/server_error.php

.user.ini File

auto_prepend_file = "/var/www/html/test1/error/init_error.php"

index.php

<?php
ob_start();
# error Example:
require_once 'test.php';
$test = new Test();
$test->testMethod();
echo '<br>No problem Detected.<br>Go to test.php and uncomment examples in test Method';

test.php

<?php

class Test
{
    /** @noinspection PhpUnhandledExceptionInspection */
    public function testMethod()
    {

        # TODO Test 1: Uncomment to get an Error Output
        echo $foo;

        # TODO Test 2: Uncomment to get an throw exception Output
        #throw new Exception('This is an Example of throw Exception');

        # TODO Test 3: Uncomment to get an shutdown Output
        #set_time_limit(1);
        #while (true) {
        #}
    }
}

conf_error.php

<?php
$conf = [
    'skin' => 'bs4',
    'error' => 'auto',
];

init_error.php

<?php
try {
    if (ini_set('session.gc_maxlifetime', '0') === false) {
        throw new Exception('Unable to set session.gc_maxlifetime.');
    }
    if (ini_set('session.use_only_cookies', '1') === false) {
        throw new Exception('Unable to set session.use_only_cookies.');
    }
    if (ini_set('session.cookie_httponly', '1') === false) {
        throw new Exception('Unable to set session.cookie_httponly.');
    }
    if (ini_set('error_reporting', '1') === false) {
        throw new Exception('Unable to set error_reporting.');
    }
    if (ini_set('display_errors', '0') === false) {
        throw new Exception('Unable to set display_errors.');
    }
} catch (Exception $e) {
    echo 'Exception catch: ', $e->getMessage(), "\n";
}

error_reporting(E_ALL);

require_once $_SERVER['DOCUMENT_ROOT'] . '/error/handler_error.php';

server_error.php

<?php
require_once $_SERVER['DOCUMENT_ROOT'] . '/error/conf_error.php';
$error_text = 'Unknown Error';
$code = $_SERVER['REDIRECT_STATUS'];
$codes = [
    400 => 'Bad Request',
    401 => 'Unauthorized',
    402 => 'Payment Required',
    403 => 'Forbidden',
    404 => 'Not Found',
    405 => 'Method Not Allowed',
    406 => 'Not Acceptable',
    407 => 'Proxy Authentication Required',
    408 => 'Request Timeout',
    409 => 'Conflict',
    410 => 'Gone',
    411 => 'Length Required',
    412 => 'Precondition Failed',
    413 => 'Payload Too Large',
    414 => 'URI Too Long',
    415 => 'Unsupported Media Type',
    500 => 'Internal Server Error',
    501 => 'Not Implemented',
    502 => 'Bad Gateway',
    503 => 'Service Unavailable',
    504 => 'Gateway Timeout',
    505 => 'HTTP Version Not Supported',
];
$source_url = 'http' . ((!empty($_SERVER['HTTPS']) && $_SERVER['HTTPS'] != 'off') ? 's' : '') . '://' . $_SERVER['HTTP_HOST'] . $_SERVER['REQUEST_URI'];
if (array_key_exists($code, $codes) && is_numeric($code)) {
    $error_text = $codes[$code];
}

/** @var array $conf */
$error_skin = $_SERVER['DOCUMENT_ROOT'] . '/error/skin/sk_' . $conf['skin'] . '_server_error.php';
if (file_exists($error_skin)) {
    /** @noinspection PhpIncludeInspection */
    require_once $error_skin;
} else {
    require_once $_SERVER['DOCUMENT_ROOT'] . '/error/skin/sk_basic_server_error.php';
}
die;

handler_error.php

<?php
declare(strict_types=1);

namespace IcarosNet\BEHBasicErrorHandler;

class HandlerError
{

    private static ?HandlerError $instance = null;

    public function __construct()
    {
        register_shutdown_function([$this, "shutdownHandler"]);
        set_exception_handler([$this, "exceptionHandler"]);
        set_error_handler([$this, "errorHandler"]);
    }

    public function shutdownHandler()
    {
        $error = error_get_last();
        if (null != $error) {
            $this->CleanOutput();
            $error_type  = 'ShutdownHandler';
            $error_level = $error['type'];
            $error_desc  = $error['message'];
            $error_file  = $error['file'];
            $error_line  = $error['line'];
            $error_trace = array_reverse(debug_backtrace());
            array_pop($error_trace);
            $error_trace_smg = $this->getBacktrace($error_trace, $error_type);
            $this->output($error_type, $error_level, $error_desc, $error_file, $error_line, $error_trace_smg);
        }
    }

    public function exceptionHandler($e)
    {
        $this->CleanOutput();
        $error_type      = 'ExceptionHandler';
        $error_level     = ($e->getCode() == 0 ? 'Not Set' : $e->getCode());
        $error_desc      = $e->getMessage();
        $error_file      = $e->getFile();
        $error_line      = $e->getLine();
        $error_trace     = $e->getTrace();
        $error_trace_smg = $this->getBacktrace($error_trace, $error_type);
        $this->output($error_type, $error_level, $error_desc, $error_file, $error_line, $error_trace_smg);
    }

    public function errorHandler($error_level = null, $error_desc = null, $error_file = null, $error_line = null)
    {
        $this->CleanOutput();
        $error_type  = 'ErrorHandler';
        $error_trace = array_reverse(debug_backtrace());
        array_pop($error_trace);
        $error_trace_smg = $this->getBacktrace($error_trace, $error_type);
        $this->output($error_type, $error_level, $error_desc, $error_file, $error_line, $error_trace_smg);
    }

    private function getBacktrace($error_trace, $error_type)
    {
        $error_trace_smg = '';
        if (!empty($error_trace)) {
            foreach ($error_trace as $track) {
                $error_trace_smg .= '  ' . (isset($track['file']) ? $track['file'] : '<unknown file>') . ' ' . (isset($track['line']) ? $track['line'] : '<unknown line>') . ' calling Method: ' . $track['function'] . '()' . PHP_EOL;
            }
        } else {
            $error_trace_smg = 'No backtrace data in the ' . $error_type . '.';
        }
        return $error_trace_smg;
    }

    /**
     * @param $error_type
     * @param $error_level
     * @param $error_desc
     * @param $error_file
     * @param $error_line
     * @param $error_trace_smg
     */
    private function output($error_type, $error_level, $error_desc, $error_file, $error_line, $error_trace_smg)
    {
        $conf = [];
        require_once $_SERVER['DOCUMENT_ROOT'] . '/error/conf_error.php';
        $out_type = $this->getRQType($conf['error']);
        if ($out_type == 'plain') {
            /** @var array $conf */
            $error_skin = $_SERVER['DOCUMENT_ROOT'] . '/error/skin/sk_' . $conf['skin'] . '_handler_error.php';
            //header('content-type: text/plain');
            if (file_exists($error_skin)) {
                /** @noinspection PhpIncludeInspection */
                require_once $error_skin;
            } else {
                require_once $_SERVER['DOCUMENT_ROOT'] . '/error/skin/sk_basic_handler_error.php';
            }
        } else {
            header('Content-Type: application/json');
            echo json_encode(
                [
                    'error_type'      => $error_type,
                    'error_level'     => $error_level,
                    'error_desc'      => $error_desc,
                    'error_file'      => $error_file,
                    'error_line'      => $error_line,
                    'error_trace_smg' => $error_trace_smg,
                ]
            );
        }
        $this->clearLastError();
    }

    private function cleanOutput()
    {
        ob_end_clean();
        flush();
    }

    private function clearLastError()
    {
        error_clear_last();
        exit();
    }

    private function getRQType($error)
    {
        /** @var string $error */
        if ($error == 'auto') {
            if (isset($_POST) && !empty($_POST)) {
                return (count($_POST) > 1 ? 'plain' : ($this->isJson($_GET) ? 'json' : 'plain'));
            }
            if (isset($_GET) && !empty($_GET)) {
                return (count($_GET) > 1 ? 'plain' : ($this->isJson($_GET) ? 'json' : 'plain'));
            }
            return 'plain';
        } else {
            return $error;
        }
    }

    private function isJson($value)
    {
        json_decode(current($value));
        return (json_last_error() == JSON_ERROR_NONE);
    }

    public static function getInstance(): HandlerError
    {
        if (!self::$instance instanceof self) self::$instance = new self;
        return self::$instance;
    }
}

HandlerError::getInstance();
\$\endgroup\$
2
  • 2
    \$\begingroup\$ This question is discussed on meta. \$\endgroup\$
    – Mast
    Commented Jan 25, 2021 at 8:27
  • \$\begingroup\$ Thanks guys, your contributions helped me a lot. Regards. \$\endgroup\$
    – user152284
    Commented Mar 11, 2021 at 19:26

2 Answers 2

4
\$\begingroup\$

Some notes about code styling, directness, and general clean up.

  1. Regarding your server_error.php file:

    • $codes is a good example of a useful lookup array, however the actual technique used to "lookup" data doesn't look right to me.

    • A default $error_text = 'Unknown Error'; declaration is made then is conditionally overwritten, but then it doesn't ever appear to be used (unless I am missing something).

    • $code is used as a shorter alias for an unchanged array element, so I am not convinced that there is much value in re-storing the same data in a new, redundant variable. (opinions may differ on this point)

    • $source_url is declared and never used (unless I am missing something).

    • The condition: array_key_exists($code, $codes) && is_numeric($code) can be simplified using a single isset() or null coalescing check. I don't see any value in the is_numeric() check -- the value is either going to be in the lookup or not.

    • When you are writing multiple value concatenations in a single line, the syntax can become excessively wide and sometimes difficult to visually track. In these cases, it may be appropriate to wrap the concatenation onto multiple lines AND if employing ternary conditions, then sprintf() may be ideal.

      <?php
      require_once $_SERVER['DOCUMENT_ROOT'] . '/error/conf_error.php';
      $codes = [
          400 => 'Bad Request',
          401 => 'Unauthorized',
          402 => 'Payment Required',
          403 => 'Forbidden',
          404 => 'Not Found',
          405 => 'Method Not Allowed',
          406 => 'Not Acceptable',
          407 => 'Proxy Authentication Required',
          408 => 'Request Timeout',
          409 => 'Conflict',
          410 => 'Gone',
          411 => 'Length Required',
          412 => 'Precondition Failed',
          413 => 'Payload Too Large',
          414 => 'URI Too Long',
          415 => 'Unsupported Media Type',
          500 => 'Internal Server Error',
          501 => 'Not Implemented',
          502 => 'Bad Gateway',
          503 => 'Service Unavailable',
          504 => 'Gateway Timeout',
          505 => 'HTTP Version Not Supported',
      ];
      $error_text = $codes[$_SERVER['REDIRECT_STATUS']] ?? 'Unknown Error';
      
      $source_url = 'http'
          . (!empty($_SERVER['HTTPS']) && $_SERVER['HTTPS'] != 'off' ? 's' : '')
          . '://'
          . $_SERVER['HTTP_HOST']
          . $_SERVER['REQUEST_URI'];
      
      /** @var array $conf */
      $error_skin = $_SERVER['DOCUMENT_ROOT']
          . '/error/skin/sk_'
          . $conf['skin'] . '_server_error.php';
      
      if (file_exists($error_skin)) {
          /** @noinspection PhpIncludeInspection */
          require_once $error_skin;
      } else {
          require_once $_SERVER['DOCUMENT_ROOT'] . '/error/skin/sk_basic_server_error.php';
      }
      die;
      
  2. Regarding your handler_error.php file:

    • Edit your code to remove all instances of single-use variables. For example, delete $error_level = $error['type']; and just write that array element directly into the $this->output line. Writing $this->output as a multiline declaration if horizontal scrolling is a concern.

      if ($error) {
          $this->CleanOutput();
          $error_type  = 'ShutdownHandler';
          $error_trace = array_reverse(debug_backtrace());
          array_pop($error_trace);
          $error_trace_smg = $this->getBacktrace($error_trace, $error_type);
          $this->output(
              $error_type,
              $error['type'],
              $error['message'],
              $error['file'],
              $error['line'],
              $error_trace_smg
          );
      }
      
    • Use a ternary assignment for $error_level using $e->getCode():

      $error_level = $e->getCode() ?: 'Not Set';
      
    • You have a popular anti-pattern in getRQType(). Never use isset() && !empty() as a condition, there is no need for the redundant isset() check, just use !empty().

    • You have a typo in this branch (you are mixing GET and POST):

      return (count($_POST) > 1 ? 'plain' : ($this->isJson($_GET) ? 'json' : 'plain'));
      

      It would be better to consolidate the conditional logic so that only one ternary is used.

      return count($_POST) > 1 || !$this->isJson($_POST) ? 'plain' : 'json';
      
    • To avoid confusion, isJson() should be named isFirstElementJson().

    • Throughout your project, never omit curly braces -- not even if it saves you two lines of code. See public static function getInstance(): HandlerError PSR-12: Control Structures

\$\endgroup\$
1
  • \$\begingroup\$ Thanks guys, your contributions helped me a lot. Regards. \$\endgroup\$
    – user152284
    Commented Mar 11, 2021 at 19:26
3
\$\begingroup\$

That's a good approach at whole, but I can see some improvements that could be made

This means that all runtime errors including Fatal are presented to the developer in the form of an exception.

Is not actually true. A more correct term would be "provides output similar to one from an exception". However, you can actually convert a runtime error into exception with a simple error handler like this:

set_error_handler(function ($level, $message, $file = '', $line = 0)
{
    throw new ErrorException($message, 0, $level, $file, $line);
});

Which will be much better because, since errors become actual exceptions, you will be able to catch them. And a you won't have to write a duplicated code for the error handler as a bonus. Unfortunately, for the fatal errors in won't work, and the currant approach is correct (other than that debug backtraces often tend to be quite big and in case of out of memory errors getting the backtrace would definitely fail).

But the biggest problem oh this handler is that it doesn't have the production mode. Which surprises me, because the main purpose of error handlers is to serve in the production environment which is drastically different from the development mode. After all, there are many ways to get extended error output in the dev mode without any handler, such as using xDebug. Or just having a plain error message, most programmers are OK with it. Whereas such a handler is indeed essential the production mode, because:

  • not a single error detail should be shown to a site user. A casual visitor won't get any clue from it anyway but it will be invaluable feedback for a potential attacker
  • instead, a generic excuse page must be shown, and this is where design is indeed essential, following the common site design and retaining the most basic controls so a user can navigate somewhere.
  • while the actual error message should be logged for a programmer, using error_log() function or some other facility

As you can see, the production mode is drastically different from the development mode and requires totally different approach to handling errors and is a must have in any error handler.

The handler code also requires some cleanup. I do understand that you are using init_error.php as sort of a universal bootstrap file for your application but apples must be sorted out from oranges. An error handler is one thing and a bootstrap file is another. Besides there are strange things such as setting error_reporting to 1 prior setting it to E_ALL and overall approach with throwing exceptions only to catch them immediately. Why not just echo the message right away, like

echo 'Unable to set session.use_only_cookies.';

without any exceptions? But again what a site user has to do with errors like this? And why a site continues to run if it cannot configure most essential settings? I believe this part of code is not well thought and you need to rethink it.

Also, I don't see any purpose in the JSON output, unless there is a client library that understands it. But having such a client code only for the dev mode seems a bit strange. After all you can always use the Dev tools in your browser to check the actual error output for the AJAX call.

\$\endgroup\$
6
  • \$\begingroup\$ @Fran I see comments like "contribute via github" after a review has been made. However, reviewers are not here purely to seek repositories to contribute to. If you find valuable insights in a review, you are free to implement them in the repository. \$\endgroup\$ Commented Jan 24, 2021 at 21:08
  • 1
    \$\begingroup\$ Perhaps I should not speak on behalf of @YourCommonSense, but I think his stance is that the library should be covering the mentioned objective -- this is certainly in the realm of a good/complete review. \$\endgroup\$ Commented Jan 24, 2021 at 23:19
  • \$\begingroup\$ @FranciscoNúñez this is a code review site and I wrote a review the way I see it. I am not your empoyee to tell me what I should write and what I should not. \$\endgroup\$ Commented Jan 25, 2021 at 14:01
  • \$\begingroup\$ the production mode is not about "Hello user, you have omitted data" errors but exactly the same errors your handler is about. \$\endgroup\$ Commented Jan 25, 2021 at 14:30
  • \$\begingroup\$ @FranciscoNúñez I am not confusing anything which I explicitly stated in the previous comment. But it seems only after you get some experience with production mode you'll understand what I am talking about. \$\endgroup\$ Commented Jan 25, 2021 at 15:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.