Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
calvinalkan opened this issue May 11, 2021 · 1 comment
Open

Comments

@calvinalkan
Copy link

Version

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

  • WP Emerge: latest
  • WordPress: 5.6
  • PHP: 7.3

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');

   }); 
  1. 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
Copy link
Author

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant