View exception handling fix (discussion) #7965

kirkbushell · 2015-03-11T16:40:37Z

I've started this PR as it needs to be fixed, but unsure as to how you would like it handled @taylorotwell . If you can provide some direction I'll resolve and then update this PR.

taylorotwell · 2015-03-11T16:46:06Z

Can you describe the issue?

taylorotwell · 2015-03-11T16:46:45Z

Also it seems sort of weird we would return exception results back out as the view result? Wouldn't taht potentially show an exception message to users of the application?

kirkbushell · 2015-03-11T16:47:39Z

If I do an include in a view:

@include('partials.something')

And that partial generates an exception - you get a message something like this:

__toString should not generat an exception at....

And it doesn't give any real feedback information.

The code in the PR is just to demonstrate and get the discussion going - am happy to implement/change whatever you think would suit it best. I also don't like how it is written currently - it's ugly. But we need to do something :)

GrahamCampbell · 2015-03-11T18:57:00Z

src/Illuminate/View/View.php

-	{
-		return $this->render();
-	}
+    /**


Please indent with tabs.

taylorotwell · 2015-03-12T01:23:57Z

Yeah, the solution presented doesn't seem acceptable as it just returns an error message as if that is what the view rendered, which would be confusing. I wish PHP would let you throw exceptions from __toString but it doesn't. How did you cause this error. Are you echoing a view without calling render?

kirkbushell · 2015-03-12T02:01:36Z

@taylorotwell na as I mentioned before, just doing an @include of a view from within a blade template that throws an exception.

kirkbushell · 2015-03-12T02:02:58Z

@taylorotwell perhaps the language-friendly approach would not to use __toString for the render. Perhaps @include could instead call another method outputRender (or something) that immediately echoes the output from the method (which is effectively what __toString is doing. This way we can still have our nice exception handling?

taylorotwell · 2015-03-12T02:07:17Z

Well @include does call ->render(), it doesn't use __toString. Was there
another view inside that view?

On Wed, Mar 11, 2015 at 9:03 PM, Kirk Bushell notifications@github.com
wrote:

@taylorotwell https://github.com/taylorotwell perhaps the
language-friendly approach would not to use __toString for the render.
Perhaps @include https://github.com/include could instead call another
method outputRender (or something) that immediately echoes the output from
the method (which is effectively what __toString is doing. This way we can
still have our nice exception handling?

—
Reply to this email directly or view it on GitHub
#7965 (comment).

kirkbushell · 2015-03-12T02:11:06Z

@taylorotwell hmmm, it could have been. It's a nested menu system, so it could have been several levels down. They all use @include, though.

It does this:

@include('partials.menu')

Then inside that, somewhere:

@include('partials.menu') or @include('partials/menuitem')

So it could be 3-4 levels deep.

taylorotwell · 2015-03-12T02:14:43Z

If you look in BladeCompiler you can see that compileInclude calls
->render() so I'm not sure how __toString would ever be called.

On Wed, Mar 11, 2015 at 9:11 PM, Kirk Bushell notifications@github.com
wrote:

@taylorotwell https://github.com/taylorotwell hmmm, it could have been.
It's a nested menu system, so it could have been several levels down. They
all use @include https://github.com/include, though.

It does this:

@include https://github.com/include('partials.menu')

Then inside that, somewhere:

@include https://github.com/include('partials.menu') or @include
https://github.com/include('partials/menuitem')

So it could be 3-4 levels deep.

—
Reply to this email directly or view it on GitHub
#7965 (comment).

kirkbushell · 2015-03-12T02:15:53Z

Alright, this is the full menu.blade.php and item.blade.php:

menu.blade.php

<ul id="menu-{{ $menu->name }}" class="menu children">
    @foreach ($menu->children() as $child)
        @include('partials.menu.item', ['item' => $child])
    @endforeach
</ul>

item.blade.php

<?php
$classes = [];

if ($item->isParent()) {
    $classes[] = 'parent';
}
if ($item->isActive()) {
    $classes[] = 'active';
}
?>
<li @if ($classes) class="{{ implode(' ', $classes) }}"@endif>
    @if ($item->isRenderable())
        @if ($item->isParent())
            <div class="submenu-title">
                @if ($item->icon())
                    <span class="af-icons af-icons-{{ $item->icon() }}"></span>
                @endif
                <span>{{ $item->text }}</span>
            </div>
            {!! HTML::menu($item->name) !!}
        @else
            <a href="{{ $item->link }}"@if ($item->isActive()) class="active"@endif>
                @if ($item->icon())
                    <span class="af-icons af-icons-{{ $item->icon() }}"></span>
                @endif
                <span class="text">{{ $item->text }}</span>
            </a>
        @endif
    @endif
</li>

The issue that was causing it previously was the $item->icon (I got it wrong, debugging it was a pain). This threw an exception (no property found iirc) which then resulted in the weird "__toString cannot throw exception" error. I had to impelment that debugging code in the PR to figure out what the hell was going on.

mvalim · 2015-03-12T14:27:42Z

I came across the same problem here, the only solution was to debug the __toString method. Yesterday, when not aware of this discussion, I even created a PR with the solution I found.
I think that debugging the __toString method is not the best solution, but was the only that I was able to find.

use Symfony\Component\Debug\ExceptionHandler as SymfonyDisplayer;
...
public function __toString()
{
    try
    {
        $contents = $this->render();
    }
    catch(\Exception $e) {
        $exceptionHandler = (new SymfonyDisplayer(config('app.debug')))->createResponse($e);
        die($exceptionHandler->getContent());
    }

    return $contents;
}

kirkbushell · 2015-03-12T15:05:22Z

Yeah we definitely need @taylorotwell's input regarding how he wants to handle this. I've encountered it more than a handful of times and each time is a real pain to debug.

taylorotwell · 2015-03-12T15:10:50Z

I think the first step is figuring out where in your application __toString is being called from, perhaps with a debug_backtrace.

kirkbushell · 2015-03-12T15:34:58Z

@taylorotwell will that help, though? I think this is an unseen issue due to perhaps the way we use blade templates and includes (not sure). I wonder if resolving how __toString is handled on the view will at least alleviate that problem not just now, but in the future.

JoostK · 2015-03-12T15:39:11Z

Once we know where it's called, we might be able to come up with a better solution where calling __toString is avoided.

For a readable stack, use:

echo (new \Exception)->getTraceAsString();

taylorotwell · 2015-03-12T16:07:07Z

Yeah my point is same as @JoostK, the framework itself should never really be calling __toString so we need to figure out where that is happening so we can make it call ->render() instead. PHP (for now) is never going to let us give a clean exception out of __toString so best to just avoid it altogether.

kirkbushell · 2015-03-12T16:16:19Z

Okay good point - will investigate further.

@JoostK thanks for the sample code, will use that to debug.

kirkbushell · 2015-03-12T16:17:58Z

Right, here's the full stack:

#0 /home/vagrant/AwardForce/storage/framework/views/6faf30a2d9347875c5edecdce8fceeb6(29): Illuminate\View\View->__toString() 
#1 /home/vagrant/AwardForce/vendor/laravel/framework/src/Illuminate/View/Engines/PhpEngine.php(39): include('/home/vagrant/A...') 
#2 /home/vagrant/AwardForce/vendor/laravel/framework/src/Illuminate/View/Engines/CompilerEngine.php(57): Illuminate\View\Engines\PhpEngine->evaluatePath('/home/vagrant/A...', Array) 
#3 /home/vagrant/AwardForce/vendor/laravel/framework/src/Illuminate/View/View.php(136): Illuminate\View\Engines\CompilerEngine->get('/home/vagrant/A...', Array) 
#4 /home/vagrant/AwardForce/vendor/laravel/framework/src/Illuminate/View/View.php(104): Illuminate\View\View->getContents() 
#5 /home/vagrant/AwardForce/vendor/laravel/framework/src/Illuminate/View/View.php(78): Illuminate\View\View->renderContents() 
#6 /home/vagrant/AwardForce/vendor/laravel/framework/src/Illuminate/Http/Response.php(44): Illuminate\View\View->render() 
#7 /home/vagrant/AwardForce/vendor/symfony/http-foundation/Symfony/Component/HttpFoundation/Response.php(202): Illuminate\Http\Response->setContent(Object(Illuminate\View\View)) 
#8 /home/vagrant/AwardForce/vendor/laravel/framework/src/Illuminate/Routing/Router.php(1185): Symfony\Component\HttpFoundation\Response->__construct(Object(Illuminate\View\View)) 
#9 /home/vagrant/AwardForce/vendor/laravel/framework/src/Illuminate/Routing/Router.php(663): Illuminate\Routing\Router->prepareResponse(Object(Illuminate\Http\Request), Object(Illuminate\View\View)) 
#10 /home/vagrant/AwardForce/vendor/laravel/framework/src/Illuminate/Routing/Router.php(618): Illuminate\Routing\Router->dispatchToRoute(Object(Illuminate\Http\Request)) 
#11 /home/vagrant/AwardForce/vendor/laravel/framework/src/Illuminate/Foundation/Http/Kernel.php(157): Illuminate\Routing\Router->dispatch(Object(Illuminate\Http\Request)) 
#12 [internal function]: Illuminate\Foundation\Http\Kernel->Illuminate\Foundation\Http\{closure}

taylorotwell · 2015-03-12T16:19:59Z

What is in that compiled view (6faf30a2d9347875c5edecdce8fceeb6) on that line 29?

kirkbushell · 2015-03-12T16:22:01Z

Ahhh, okay now we're getting closer.

Line #29, is this:

<?php echo HTML::menu('main'); ?>

Which is this macro:

HTML::macro('menu', function ($menu) {
    // If a menu instance was not passed, let's instead try and fetch the menu by name
    if (!($menu instanceof \Tectonic\Shift\Library\Menu\Menu)) {
        $menu = Menufy::menu($menu)->first();
    }

    // Render the menu
    return view('partials.menu.menu', compact('menu'));
});

I think now I realise the issue - view should probably return render?

taylorotwell · 2015-03-12T16:22:22Z

Yep. return view()->render();

kirkbushell · 2015-03-12T16:23:02Z

Okay cool, soooo....

This is a slightly different behaviour from what is recommended when working with controllers (obviously). Is there anything you'd like to change or for me to work on @taylorotwell to try and avoid this in future?

kirkbushell · 2015-03-12T16:24:03Z

I wonder if __toString is necessary in this case, as it seems to create problems if exceptions are thrown in views?

kirkbushell · 2015-03-12T16:24:11Z

Aka, should __toString be removed?

taylorotwell · 2015-03-12T16:25:02Z

What's interesting is that apparently we can use trigger_error from within the __toString method to perhaps raise an error message.

http://php.net/manual/en/function.trigger-error.php

taylorotwell · 2015-03-12T16:25:20Z

For reference: http://stackoverflow.com/questions/2429642/why-its-impossible-to-throw-exception-from-tostring

MaartenStaa · 2015-03-12T16:26:27Z

Perhaps HTML macros should automatically call ->render() if the returned value is a RenderableInterface?

taylorotwell · 2015-03-12T16:28:08Z

I'm curious if we can catch the exception in __toString and do this...

trigger_error((string) $exception, E_USER_ERROR);

kirkbushell · 2015-03-12T16:28:55Z

That's not a bad idea either (as another solution) - but html/form is being managed by laravel-collective now and they've made it clear to me that no improvements/changes/extra features will be forthcoming.

@taylorotwell will test now :)

kirkbushell · 2015-03-12T16:30:29Z

@taylorotwell unfortunately it results in the same error:

FatalErrorException in 6faf30a2d9347875c5edecdce8fceeb6 line 0:
Method Illuminate\View\View::__toString() must not throw an exception

This is when triggering an error manually if an exception has been caught:

public function __toString()
    {
        try {
            return $this->render();
        }
        catch (\Exception $e) {
            trigger_error((string) $e, E_USER_ERROR);
        }
}

kirkbushell · 2015-03-12T16:31:53Z

it seems to me that the only way this is happening is if an echo is done directly on the view in these helpers.

@taylorotwell what's the need for __toString if it's never been called by Laravel itself?

taylorotwell · 2015-03-12T16:35:26Z

The need is because some people still call it (as evidence by this PR) :) haha... but yeah ideally it would just be removed entirely

kirkbushell · 2015-03-12T16:37:39Z

@taylorotwell yup, true - but it results in an issue that cannot be easily resolved.

It seems to me using __toString on a rather lengthy/complex set of logic which can result in exceptions being thrown, simply means annoyed developers as it's rather a pain to debug. It appears to be an anti-pattern within PHP.

i double-checked by fixing up all my macros/helpers and then throw an exception in __toString - it never gets called by the framework itself.

This would be a breaking change - but I think a minor one.

taylorotwell · 2015-03-12T16:40:47Z

We're only about 8 weeks away from 5.1. We can probably remove it there.

kirkbushell · 2015-03-12T16:54:06Z

I may have another solution for now :)

kirkbushell · 2015-03-12T16:55:02Z

See: #7980

JoostK · 2015-03-12T18:49:22Z

Another hack I just thought of is assigning the exception to a static property and at the end of the request throwing that exception if it's been set. This may however give an incorrect stacktrace, I'm not sure how generating an exception and throwing it somewhere different affects the trace.

kirkbushell · 2015-03-12T18:59:01Z

Yeah that would be very hacky - the trace would start from where it's thrown.

JoostK · 2015-03-12T19:47:52Z

Actually that's not what happens, it works pretty nicely. There is no sign of it being thrown somewhere completely different.

kirkbushell · 2015-03-12T20:02:47Z

Hmmm, I've seen quite the opposite when dealing with caught exceptions that are re-thrown in Laravel. Perhaps something else was at play...

MaartenStaa · 2015-03-12T21:26:02Z

If an exception is caught and then a new exception was created, then yes. Quoting from a comment in the PHP documentation:

Note that an exception's properties are populated when the exception is *created*, not when it is thrown.  Throwing the exception does not seem to modify them.

Among other things, this means:

* The exception will blame the line that created it, not the line that threw it.

* Unlike in some other languages, rethrowing an exception doesn't muck up the trace.

* A thrown exception and an unthrown one look basically identical.  On my machine, the only visible difference is that a thrown exception has an `xdebug_message` property while an unthrown one doesn't.  Of course, if you don't have xdebug installed, you won't even get that.

kirkbushell · 2015-03-12T21:44:52Z

yeah that's super cool - it must have been something else I was fighting against when I saw another case.

kirkbushell added 2 commits March 11, 2015 16:38

View exception handling update

cc85bc6

View test update for handling exceptions

a41713a

GrahamCampbell reviewed Mar 11, 2015
View reviewed changes

src/Illuminate/View/View.php

{

return $this->render();

}

/**

Copy link

Member

GrahamCampbell Mar 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please indent with tabs.

kirkbushell closed this Mar 12, 2015

kirkbushell mentioned this pull request Mar 12, 2015

When working with macros, if a view is returned - results in this issue LaravelCollective/html#19

Closed

kirkbushell mentioned this pull request Mar 12, 2015

Fixes the problem with macros returning Renderable implementations #7980

Closed

View exception handling fix (discussion) #7965

View exception handling fix (discussion) #7965

Conversation

kirkbushell commented Mar 11, 2015

taylorotwell commented Mar 11, 2015

taylorotwell commented Mar 11, 2015

kirkbushell commented Mar 11, 2015

GrahamCampbell Mar 11, 2015

Choose a reason for hiding this comment

taylorotwell commented Mar 12, 2015

kirkbushell commented Mar 12, 2015

kirkbushell commented Mar 12, 2015

taylorotwell commented Mar 12, 2015

kirkbushell commented Mar 12, 2015

taylorotwell commented Mar 12, 2015

kirkbushell commented Mar 12, 2015

menu.blade.php

item.blade.php

mvalim commented Mar 12, 2015

kirkbushell commented Mar 12, 2015

taylorotwell commented Mar 12, 2015

kirkbushell commented Mar 12, 2015

JoostK commented Mar 12, 2015

taylorotwell commented Mar 12, 2015

kirkbushell commented Mar 12, 2015

kirkbushell commented Mar 12, 2015

taylorotwell commented Mar 12, 2015

kirkbushell commented Mar 12, 2015

taylorotwell commented Mar 12, 2015

kirkbushell commented Mar 12, 2015

kirkbushell commented Mar 12, 2015

kirkbushell commented Mar 12, 2015

taylorotwell commented Mar 12, 2015

taylorotwell commented Mar 12, 2015

MaartenStaa commented Mar 12, 2015

taylorotwell commented Mar 12, 2015

kirkbushell commented Mar 12, 2015

kirkbushell commented Mar 12, 2015

kirkbushell commented Mar 12, 2015

taylorotwell commented Mar 12, 2015

kirkbushell commented Mar 12, 2015

taylorotwell commented Mar 12, 2015

kirkbushell commented Mar 12, 2015

kirkbushell commented Mar 12, 2015

JoostK commented Mar 12, 2015

kirkbushell commented Mar 12, 2015

JoostK commented Mar 12, 2015

kirkbushell commented Mar 12, 2015

MaartenStaa commented Mar 12, 2015

kirkbushell commented Mar 12, 2015