4
\$\begingroup\$

I initially started searching for a PHP script which would output PHP errors, to the JavaScript console. As the Default behaviour is just to spit them out anywhere and displace everything else.

Although that was not successful, this is the code I have put together so far. Can anyone please review it and say if it can be shortened or optimized? The CSS Class I have added onto the errors just adds position: relative and a value in my external CSS file.

=> ( Would be better to have position Absolute, but then all the errors overlap regardles of any breaks, n/ lines or PHP_EOL, may get changed to a float I'm not sure yet => They would be better because they don't take up any physical space or cause displacement).

Sure this Can be shortened or optimized somehow. But PHP is not my strong point & struggling.

Are there any security issues with using this? Are they any errors in my Arrays?

<?php                                                                           
// ::=> ErrorHandler.php [File];


        class MyError
                {
        protected static $collected = array();

        public static function getCollectedErrors()
                             {
          return self::$collected;
                             }

        protected static function addError($key, $error)
                                {
          if (!isset(self::$collected[$key]))
            self::$collected[$key] = array();
            self::$collected[$key][] = $error;
                                }
        // CATCHABLE ERRORS
        public static function captureNormal( $number, $message, 
                                              $file, $line )
                             {
            // Insert all in one table
            $error = array( 'type' => $number, 
                            'message' => $message, 
                            'file' => $file, 
                            'line' => $line 
                           );
            // Display content $error variable
            self::addError('error', $message . " at " . $file . 
                                 ':' . $line);
                             }

        public static function captureException( $exception )                   
                             {
            // Display content $exception variable
            self::addError('exception', $exception);
                             }

        // UNCATCHABLE ERRORS
        public static function captureShutdown( )
                             {
          $error = error_get_last( );
           if( $error ) {
      ## IF YOU WANT TO CLEAR ALL BUFFER, UNCOMMENT NEXT LINE:
          # ob_end_clean( );

                // Display content $error variable
                self::addError('shutdown', $error);
                         } else { self::addError('shutdown', '<none>'); 
                           return true; }                                               
                            }
    }

    set_error_handler(array( 'MyError', 'captureNormal' ));
    set_exception_handler(array( 'MyError', 'captureException' ));
    register_shutdown_function(array( 'MyError', 'captureShutdown' ));
    ?>

& in my index & sub-pages =>

<?php
        //                                             >>>>>>> Add if Admin Statement in Here & Password Match & Cookie, csrf check. 
    $errors = MyError::getCollectedErrors();        // This is Linked to ErrorHandlers.php
    foreach ($errors as $category => $items) {
        echo "<strong class=\"dispLow\" 
                      style=\"display:block;
                              position:relative;
                              max-width:248px;
                              text-wrap:wrap;
                              z-index:2000;
                              background:yellow;\">" . $category . ":
              </strong><br/>";                                                  // I added class dispLow to the outputted errors with \ escape... 
        foreach ($items as $error) {

            echo "<br>Error:" . $error . $catergory . 
                                $items . $file .
                                $line . "<br/>" .PHP_EOL;
        }
    }
?>

Related Questions I could find:

>> Php Error Handler/Logger
>> Echo to Console

\$\endgroup\$
6
  • \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. Please post a new question instead. \$\endgroup\$
    – Mast
    Commented Feb 22, 2020 at 7:04
  • \$\begingroup\$ Try tracy debug panel \$\endgroup\$
    – slepic
    Commented Feb 22, 2020 at 8:09
  • \$\begingroup\$ @Mast If I post a New question with The Exact same code it will just get deleted as a duplicate, I only edited the question to remove some of the whitespace & fix the indentation issus, So that it is more legible for other people viewing this post? Is that not allowed? \$\endgroup\$
    – Ryan Stone
    Commented Feb 23, 2020 at 1:40
  • \$\begingroup\$ @slepic is this available for firefox? \$\endgroup\$
    – Ryan Stone
    Commented Feb 23, 2020 at 1:40
  • \$\begingroup\$ So don't post a new question with the exact same code. Use the current reviews to improve the code and then post a new question. \$\endgroup\$
    – Mast
    Commented Feb 23, 2020 at 7:06

3 Answers 3

3
+50
\$\begingroup\$
  • As others have mentioned, your tabbing has quite the negative impact on readability.
  • As dusty stated, you can safely declare $collection as private.
  • There is no reason to check if the $collected class object is declared or if the designated $key exists before pushing an element into it. Just push that sucker in there!
  • In captureShutdown() method there is some redundant scripting which can be easily cleaned up with the coalescing operator like this: self::addError('shutdown', error_get_last() ?? '<none>');. You are declaring a return value, but the static call is not doing anything with it -- so it can be omitted.
  • On second thought, I actually don't like the fact that sometimes the captureShutdown() method conditionally passes a string or an array as the second parameter of addError. I would prefer this class to generate consistently typed items in $collected so that pumping the data into the view is cleaner. For those who are not instantly aware, error_get_last() returns null or an array with the keys: type, message, file and line. Instead of error_get_last() ?? '<none>', maybe it would be better to craft a very human/English string for either outcome.

Your class:

class MyError
{
    private static $collected = [];

    public static function getCollectedErrors()
    {
        return self::$collected;
    }

    private static function addError($key, $error)
    {
        self::$collected[$key][] = $error;
    }

    public static function captureNormal($number, $message, $file, $line)
    {
        self::addError('error', "{$number}: {$message} at {$file}:{$line}");
    }

    public static function captureException($exception)                   
    {
        self::addError('exception', $exception);
    }

    public static function captureShutdown()
    {
        // self::addError('shutdown', error_get_last() ?? '<none>');
        $lastError = error_get_last();
        if ($lastError) {
            $message = "{$lastError['type']}: {$lastError['message']}"
                     . " at {$lastError['file']}:{$lastError['line']}";
        } else {
            $message = "No errors present at shutdown";
        }
        self::addError('shutdown', $message);
    }
}

set_error_handler(['MyError', 'captureNormal']);
set_exception_handler(['MyError', 'captureException']);
register_shutdown_function(['MyError', 'captureShutdown']);
  • I have concerns about your mixing of strings and array in the echo of your inner foreach() -- that can't be working out well.
  • Move all of your inline styles to an external stylesheet to make the markup more readable.

Your content:

$errors = MyError::getCollectedErrors();
$messages = '';
foreach ($errors as $category => $message) {
    $messages .= "<div><h3 class=\"category\">{$category}</h3>";
    $messages .= '<div>' . implode('</div><div>', $message) . '</div></div>';
}
if ($messages) {
    echo '<div id="draggableErrorModal">
              <div id="draggableErrorModalHeader">
                  Draggable Error Modal
              </div>
              <div id="draggableErrorModalClose">&times;</div>
              <div id="draggableErrorModalBody">' . $messages . '</div>
          </div>';
}

Okay, I whipped up a basic modal with a few little UX niceties via pure javascript and css such as the ability to be dragged, resized, and closed so that the notification is always in your face but also very easy to tuck away to reveal the page content. You can style it to your heart's content. Have a play.

if (document.getElementById("draggableErrorModal")) {
  document.getElementById("draggableErrorModalClose").addEventListener('click', function(){
    document.getElementById("draggableErrorModal").style.display = 'none';
  });
  
  dragElement(document.getElementById("draggableErrorModal"));

  function dragElement(errorModal) {
    let pos1 = 0,
        pos2 = 0,
        pos3 = 0,
        pos4 = 0;
      
    document.getElementById(errorModal.id + "Header").onmousedown = dragMouseDown;

    function dragMouseDown(e) {
      e = e || window.event;
      e.preventDefault();
      // get the mouse cursor position at startup:
      pos3 = e.clientX;
      pos4 = e.clientY;
      document.onmouseup = closeDragElement;
      // call a function whenever the cursor moves:
      document.onmousemove = elementDrag;
    }

    function elementDrag(e) {
      e = e || window.event;
      e.preventDefault();
      // calculate the new cursor position:
      pos1 = pos3 - e.clientX;
      pos2 = pos4 - e.clientY;
      pos3 = e.clientX;
      pos4 = e.clientY;
      // set the element's new position:
      errorModal.style.top = (errorModal.offsetTop - pos2) + "px";
      errorModal.style.left = (errorModal.offsetLeft - pos1) + "px";
    }

    function closeDragElement() {
      // stop moving when mouse button is released:
      document.onmouseup = null;
      document.onmousemove = null;
    }
  }
}
#draggableErrorModal {
  position: absolute;
  z-index: 9;
  background-color: white;
  border: 1px solid #ff5500;
  top: 0;
  left: 0;
  resize: both;
  overflow: auto;
}

#draggableErrorModalHeader {
  padding: 10px;
  text-align: center;
  cursor: move;
  background-color: lightgrey;
  color: #ff8800;
}

#draggableErrorModalClose {
  position: absolute;
  z-index: 10;
  top: 2px;
  right: 4px;
  font-size: 30px;
  cursor: pointer;
}

#draggableErrorModalBody {
  padding: 20px;
}

#draggableErrorModalBody div:nth-child(odd) {
  background-color: #efefef;
}
<body>
<div class="regularContent">content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content content</div>
<div id="draggableErrorModal">
  <div id="draggableErrorModalHeader">
    Draggable Error Modal
  </div>
  <div id="draggableErrorModalClose">&times;</div>
  <div id="draggableErrorModalBody">
    <div>Message 1: This is where your<br>errors will be.</div>
    <div>Message 2: This is where your errors will be.</div>
    <div>Message 3: Your errors will be here and the modal can be resized and closed entirely.</div>
    <div>Message 4: This is where your<br>errors will be.</div>
    </div>
</div>
</body>

\$\endgroup\$
4
\$\begingroup\$

Fixing the white space alone will improve readability.

Use private variables instead of protected when you don't plan to use the variable in an extended class.

Use curly braces even when the executed code is only one line long.

If you want to display the errors in a console, you can use console.log inside of a <script>. Such as this:

$errorMessage;

foreach ($items as $error) 
{
    $errorMessage .= "Error:" . $error . $catergory . 
                            $items . $file .
                            $line . "\n";
}

echo "<script> console.log('" . $errorMessage . "'); </script>";
\$\endgroup\$
2
\$\begingroup\$

First off I must say that the indentation is quite inconsistent and makes reading this code challenging. While most of the inconsistencies appear to be indentation on braces, the indentation on comments also seems a bit wonky. While I don't adhere to everything in it, I suggest following PSR-2 - especially using 4 spaces for indentation.

One thing I observed in the sample usage code is that $file and $line are NULL. Perhaps you were intending to use those from the array $error. I also noticed captureNormal() stores an array in the variable $error but that doesn't appear to be used after it is declared (despite the comment on the next line: Display content $error variable). Maybe you took the usage of $error out of that static method and moved it into the other file?

public static function captureNormal( $number, $message, 
                                          $file, $line )
                         {
        // Insert all in one table
        $error = array( 'type' => $number, 
                        'message' => $message, 
                        'file' => $file, 
                        'line' => $line 
                       );
        // Display content $error variable
        self::addError('error', $message . " at " . $file . 
                             ':' . $line);
                         }

To avoid the need to escape double quotes in the HTML, you could use a different delimiting method like heredoc or nowdoc. And perhaps it would be simpler to move the styles into CSS.

\$\endgroup\$

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.