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

PHP notice raises ExceptionError? #187

Closed
alessandro-fazzi opened this issue Jan 17, 2018 · 6 comments
Closed

PHP notice raises ExceptionError? #187

alessandro-fazzi opened this issue Jan 17, 2018 · 6 comments
Assignees

Comments

@alessandro-fazzi
Copy link

Hello,

I encountered an issue with the following code:

= $array['non-existent-key']

I expected to get:

<p></p>

But I actually get an ExceptionError.

Undefined index AFAIK will generate a notice in PHP. Why is Phug throwing an ExceptionError? Is this expected behaviour? Is this a configurable behaviour?

Thanks!

@alessandro-fazzi
Copy link
Author

I'm going to update initial informations. Which, at his point of my comprehension, makes the situation even more complicated.

I have a pug file with

- $bar = new Foo()
= $bar->get_key('non-existent-key')

Given that Foo class is already in the scope, this is the definition

class Foo {
    $ary = array('foo' => 'baz')

    function get_key($key){
        return this->$ary[$key]
    }
}

It seems to me that since get_key method will raise a noice, the pug Renderer will raise an exception of type ErrorException.

W/o any more information this sounds as a bug to me. Looking for more.

Thanks in advance

@kylekatarnls
Copy link
Member

Hi, my proposition is to provide some settings to set error_reporting into account or a custom settings for errors:
https://github.com/phug-php/util/blob/master/src/Phug/Util/SandBox.php#L28
Right now, our sandbox catch all or any error, but it does not take into account the types of errors.

But you must understand that if we do not catch errors, we will not be able to tell where it happened in the pug code. That's why the Renderer use a sandbox to catch errors, then add infos about the matching position in the pug code and re-throw or display it.

The error handler is not a bug, we will keep it because for 99% of cases, the wrapped error is more useful than the original error. Moreover, the wrapped error contains the original error, so no information can be lost.

But I agree to provide settings to ignore notice errors, strict standard errors and so on.

@alessandro-fazzi
Copy link
Author

alessandro-fazzi commented Jan 17, 2018

Hi @kylekatarnls ,

I'm absolutely with you about the usefulness of the error handler. But :)

  • generally raising exceptions for a notice is a really opinionated approach and could be a problem in some situation such as...
  • ... consider I've required a vendor library and instantiated an object from its constructor; then in my PUG template I use a method from my 500kilobytesOfCodeLibrary to print, lets say, a String on my page. Now what's up if the library, due to any reasons, will raise a notice? I'm totally blocked, isn't it?

I'm writing a template for WordPress and using a do_shortcode function which calls logic from within a 3rd party plugin. So what? ATM I'm running around the office screaming half the time, and fixing tons of 3rd party code for the other half 😛

I and a colleague of mine thought about using a :php filter in the template, but the NOTICE is anyway catched by Phug.

Summarizing:

We've migrated from pug-php v2 to v3 and we think it's an awesome work, and the debug/error handling implementations are just great. But I can't be responsible for notices from vendor (or just another dev team from my company) libraries while building a template.

The interface I'd expect would be an error_reporting option to pass to the Pug constructor with similar arguments to the underlying PHP one.

Moreover: could we think about do honor the PHP error reporting settings?

A lot of thoughts, in the hope they could be somewhat useful.

Thanks for your support

@kylekatarnls
Copy link
Member

Yes error_reporting is already what we use. Now we provide a full-support.

You can already try the new sandbox with composer update and so you would be able to:

- error_reporting(E_ALL ^E_NOTICE)
= $array['non-existent-key']

Obviously, you can set error_reporting outside of the template (or in your php.ini).

I will add an option soon to customize the error handling inside templates.

@kylekatarnls
Copy link
Member

Hi, here is the new option you can change by using the new renderer (composer update):
https://phug-lang.com/#error-reporting-callable-int

I close this issue but do not hesitate to answer on it if you get any trouble.

@alessandro-fazzi
Copy link
Author

alessandro-fazzi commented Feb 22, 2018

I'm really sorry to come back just now. ATM here just to thank you for this improvement. I'm going to work on it.

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

No branches or pull requests

2 participants