htmlburger / wpemerge

A modern, MVC-powered WordPress as a CMS workflow. 🚀
https://wpemerge.com/
GNU General Public License v2.0
453 stars 38 forks source link

Impossible to catch exceptions in views without printing output. #27

Open calvinalkan opened 3 years ago

calvinalkan commented 3 years ago

Version

Please add the exact versions used for each of the following:

Expected behavior

When an exception occurs inside rendering a view, all output buffers are cleared, and an exception is thrown to render an error page appropriately.

Actual behaviour

Right now, any error that happens inside view rendering will cause any HTML before the error to be printed to the client.

Steps to reproduce (in case of a bug)

  1. Create a route:

    \App\App::route()->get()->url('foo')->handle(function () {
    
        return \App\App::view('view.php');
    
    }); 
  2. Create a view like this:

<?php

?>

<h2> Foobar</h2>

<?php

    non_existing_function();

?>
  1. Turn on WP_DEBUG (optional)

  2. Visit yoursite.com/foo

  3. See Foobar in the error page html.

Comments

The reason this happens can be found inside the PhpView class

The toString method needs to have some form of error handling that resets the buffered content. If not PHP will automatically clear and print the output when an exception occurs and the script shuts down.

public function toString() {

        if ( empty( $this->getName() ) ) {
            throw new ViewException( 'View must have a name.' );
        }

        if ( empty( $this->getFilepath() ) ) {
            throw new ViewException( 'View must have a filepath.' );
        }

        $this->engine->pushLayoutContent( $this );

        if ( $this->getLayout() !== null ) {
 // ERROR HAPPENS HERE
            return $this->getLayout()->toString();
        }

        return $this->engine->getLayoutContent();
    }

A possible fix would be changing the relevant part to something like this:

public function toString() {

     // left out for brevity

       ob_start();
try {
   if ( $this->getLayout() !== null ) {
 // ERROR HAPPENS HERE
            return $this->getLayout()->toString();
        }

        $this->engine->getLayoutContent();
    }
    catch ( Throwbable $e ) {
    ob_end_clean();
    throw new ViewException();

    }

return ob_get_clean();
}
calvinalkan commented 3 years ago

Working fix, I cant submit a PR because I have a fastly different custom version of wpemerge but this will fix the issue:


public function toString() : string {

            $ob_level = ob_get_level();

            ob_start();

            try {

                $this->requireView();

            }
            catch ( \Throwable $e ) {

                $this->handleViewException($e, $ob_level);

            }

            return ob_get_clean();

        }

        private function requireView () {

            $this->engine->pushLayoutContent( $this );

            if ( $this->getLayout() !== null ) {
                return $this->getLayout()->requireView();
            }

            $this->engine->getLayoutContent();

        }

        private function handleViewException(\Throwable $e , $ob_level) {

            while (ob_get_level() > $ob_level) {
                ob_end_clean();
            }

            throw new ViewException('Error rendering view: [' . $this->getName() . '].');

        }

Another concern I have with this is what would happen if the size of the view exceeds the max buffer output?