laravel / ideas

Issues board used for Laravel internals discussions.
938 stars 28 forks source link

[Proposal] Blade files on stack trace #275

Closed rogervila closed 6 years ago

rogervila commented 7 years ago

It would be nice to see the blade file instead of the compiled view on the stack trace.

franzliedke commented 7 years ago

Hmm, it will be difficult to debug errors that way, because line numbers and code will not necessarily match the error message. Lots of pesky details that would need to be taken care of.

I personally think it is more helpful to see the compiled code because that helps me understand what is really going on (i.e. what code is being executed).

jhoff commented 7 years ago

I mean, you do get the name of the blade file at the end of the exception message but I think whats missing is the actual line number from the source blade file.

jhoff commented 7 years ago

Looking over the source code for the blade compiler, I think it might be tough ( if not impossible ) to trace the source blade directive to the exception being thrown. I think the only way to do it is to drop comment blocks in the compiled blade that indicate the source file and line. When an exception is thrown it wouldn't be too difficult to analyze the compiled code to pick up the source file and line.

resources/views/index.blade.php

@extends('layouts.app')
@section('content')
    @include('partials.alerts')
    <h1>Example Blade File</h1>
@endsection

resources/views/partials/alerts.blade.php

@if ($message = Session::get('message'))
    <div class="alert alert-success">
        <div class="wrapper">
            <p class="message">{{ $message }}</p>
        </div>
    </div>
@endif

Compiled index.blade.php

<?php /*Compiled from resources/views/index.blade.php*/
<?php /*:2*/ $__env->startSection('content'); ?>
    <?php /*:3*/ echo $__env->make('partials.alerts', array_except(get_defined_vars(), array('__data', '__path')))->render(); ?>
    <h1>Example Blade File</h1>
<?php /*:5*/ $__env->stopSection(); ?>

<?php /*:1*/ echo $__env->make('layouts.app', array_except(get_defined_vars(), array('__data', '__path')))->render(); ?>

Compiled partials/alerts.blade.php

<?php /*Compiled from resources/views/partials/alerts.blade.php*/
<?php /*:1*/ if($message = Session::get('message')): ?>
    <div class="alert alert-success">
        <div class="wrapper">
            <p class="message"><?php /*:4*/ echo e($message); ?></p>
        </div>
    </div>
<?php /*:7*/ endif; ?>
rogervila commented 7 years ago

@jhoff that could be a good solution. If we go with comments, we could event show the name of the directive that caused the error (if it was caused by a directive)

Something like:

<?php /*Compiled from resources/views/index.blade.php*/
<?php /* ['line' => 2, 'directive' => '@content'] */ $__env->startSection('content'); ?>
...

I think that the more information we add, the better.

rogervila commented 7 years ago

@franzliedke another approach could be displaying both, the compiled name and the original blade, in a really human way. For example:

@include directive could not find the view 'foo', on content.blade.php, compiled as 3da541559918a808c2402bba5012f6c60b27661c.php

inxilpro commented 7 years ago

What about using something like JavaScript source maps?

donnysim commented 7 years ago

But it already shows the blade file for errors except for FatalErrorException?

View [anotherone2] not found. (View: ...\resources\views\anotherone.blade.php)

or

ErrorException in c456aa9d0354902119844f7a65837b53477856cd.php line 1:
ads (View: ...\resources\views\anotherone.blade.php)

Is this not enough? You'd need to recreate the thrown error to add blade files to the trace which could lead to more problems. The compiled file line number could be used as an estimate from which line to search.

wotta commented 7 years ago

@donnysim This way you still need to look which file the cached file belongs to. I think the idea is to quickly see the correct file.

Personally I would love this feature, if something breaks you can find the error more quickly.

sebdesign commented 7 years ago

@inxilpro if source maps could be implemented for blade views, that would provide great benefits for dev tools like laravel-debugbar or even inside chrome dev tools. I like your idea!

nicooprat commented 7 years ago

I'm currently using PHP Console: "PHP Console allows you to handle PHP errors & exceptions, dump variables, execute PHP code remotely and many other things using Google Chrome extension PHP Console and PhpConsole server library." Might help to display line numbers in console?

Sourcemaps are really important. Looking forward to this feature!

jbrooksuk commented 7 years ago

Relevant: https://wiki.php.net/rfc/sourcemaps

jhoff commented 7 years ago

So I spent a little time over-viewing revisions 1-3 of the original source map proposals from the engineers at Google and Mozilla ( 1-2, 3 ) and I don't think it would be terribly difficult to implement a basic source map generation class based on some of these ideas. I feel like taking a modified approach similar to revision 2 makes the most sense. Json parsable file that maps a compiled PHP file back to the source blade files and corresponding line numbers and columns. I'm not sure the complexity introduced in revision 3 is necessary for Blade, as it's mostly focused on reducing the map file size and the only real cost in this implementation is server side disk space.

That said, with a cursory look over the source code I don't think this would be something that can easily be shoehorned into the current blade implementation. It would likely require a pretty significant overhaul of how blade files are constructed today.

creightonja commented 7 years ago

This is probably the number one reason why we used Twig Bridge at my old shop.

rogervila commented 5 years ago

PHP Storm added support for Blade debugging. I think that this change could be a starting point to apply this proposal.

https://twitter.com/phpstorm/status/1097835230944260096?s=19

@taylorotwell I'd like to reopen this issue so we can discuss about the proposal