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

[4.2] Updated Whoops #4378

Merged
merged 1 commit into from May 12, 2014
Merged

[4.2] Updated Whoops #4378

merged 1 commit into from May 12, 2014

Conversation

GrahamCampbell
Copy link
Member

Whoops 1.1 has been released now.


Replaces #4316.

taylorotwell added a commit that referenced this pull request May 12, 2014
@taylorotwell taylorotwell merged commit a862125 into laravel:4.2 May 12, 2014
@taylorotwell
Copy link
Member

@GrahamCampbell was this tested at all? Laravel 4.2 now just completely white screens on error?!?!?!

@taylorotwell
Copy link
Member

This needs to be fixed pronto.

@GrahamCampbell
Copy link
Member Author

What????? I tested this about 3 months back. I'll pull it down and take another look asap...

@GrahamCampbell
Copy link
Member Author

Did you just fix it with 9d796c6, or do I still need to take a look?

@mitchellvanw
Copy link
Contributor

_cough_ run tests next time _cough_

@taylorotwell
Copy link
Member

No it is totally broken when there is a bootstrap/compiled.php file present.

@mitchellvanw
Copy link
Contributor

@taylorotwell what about a revert back to the commit before this merge?

@GrahamCampbell
Copy link
Member Author

@mitchellvanw Is there any need to do that?

@GrahamCampbell
Copy link
Member Author

That is the 4.2-dev branch. People can use the 4.2-beta tag if they need 4.2.

@mitchellvanw
Copy link
Contributor

That's not an excuse to push broken code

@taylorotwell
Copy link
Member

@GrahamCampbell less talkey, more fixey :)

@GrahamCampbell
Copy link
Member Author

@taylorotwell I can't replicate this?

@taylorotwell
Copy link
Member

Oh wtf. I'm just going to rip Whoops out then.

@GrahamCampbell
Copy link
Member Author

I am running windows 7, php 5.5. I tried with and without running php artisan optimize, and everything works for me?

@taylorotwell
Copy link
Member

Run php artisan optimize --force

@GrahamCampbell
Copy link
Member Author

@taylorotwell I have a fresh laravel 4.2 install. http://goo.gl/oUVd4E http://goo.gl/quwp2c http://goo.gl/w3Y9Hg All working here???

@taylorotwell
Copy link
Member

OK I don't care if it works on your machine. It doesn't work on MY machine.

@GrahamCampbell
Copy link
Member Author

Hmmm. Well it's kind of hard for me to debug the state of your machine...

@taylorotwell
Copy link
Member

[2014-05-12 19:41:36] production.ERROR: exception 'Symfony\Component\Debug\Exception\FatalErrorException' with message 'Uncaught exception 'RuntimeException' with message 'Could not find resource 'views/layout.html.php' in any resource paths.(searched: /home/vagrant/Code/Laravel/plain/vendor/laravel/framework/src/Illuminate/Exception/resources, /Users/taylor/Code/Laravel/plain/vendor/filp/whoops/src/Whoops/Handler/../Resources)' in /home/vagrant/Code/Laravel/plain/bootstrap/compiled.php:10562

@GrahamCampbell
Copy link
Member Author

Thanks @taylorotwell I'll dig into the whoops source. My thoughts are that maybe they are using __DIR__ somewhere which won't work as expected in the dumped form.

@taylorotwell
Copy link
Member

I have switched to 4.2 to using the Symfony debug exception displayer implementation instead of Whoops since Whoops is apparently totally broken, unreliable, and crap.

@GrahamCampbell
Copy link
Member Author

My thoughts are now that maybe the file permissions are dodgie at your end (they can be corrected in the whoops repo if needed). Have you tried chmod 755 or some shit on the vendor folder?

@GrahamCampbell
Copy link
Member Author

Maybe we could make it easily configurable to switch between whoops/symfony for 4.2, and by default have it use symfony if you don't like whoops so much?

@taylorotwell
Copy link
Member

Dunno. Don't have time to look at it. You did make permissions changes though

http://d.pr/i/pPi0/4NKhYN5F

@taylorotwell
Copy link
Member

If it can't be fixed with a pull request i'll just rip whoops out.

@GrahamCampbell
Copy link
Member Author

If I can get whoops working, do you like the idea of a config option to switch between the two displayers?

@taylorotwell
Copy link
Member

Nope.

@GrahamCampbell
Copy link
Member Author

I don't think the custom styles are much of an issue. I'd be happy to update them at any point in the future if need be.

@GrahamCampbell
Copy link
Member Author

@taylorotwell Does that mean #4397 is good to go?

@mmodler
Copy link

mmodler commented Jun 11, 2014

Whoops ist still broken with laravel v4.2.1 and compiled.php, see #4630

@taylorotwell
Copy link
Member

OK what the heck. I'm so close to just ripping out this library.

@spencerdeinum
Copy link
Contributor

As long as it shows a stack trace the Symfony one sounds fine.

Either use Symfony or remove the custom styles imo.

Eating my exceptions is unacceptable.

t_6e79237e467146ffb12a2bd98d602cab

@taylorotwell
Copy link
Member

Yeah I'm using Symfony. So done with this Whoops library.

@taylorotwell
Copy link
Member

Fixed in 4.2.2

@Anahkiasen
Copy link
Contributor

Doesn't Symfony have a better error displayer than this ? Cause this is pretty unhelpful I mean yeah I have the stack but being able to see the interior of files was a huge speed help, same for the POST parameters and all. Can this at least be made an option or something ?

@taylorotwell
Copy link
Member

I can't use a library that just randomly white screens every few releases. Take it up with Whoops.

@taylorotwell
Copy link
Member

It's never really worked right much since Graham updated it to this version.

@Anahkiasen
Copy link
Contributor

I have nothing to take up, I've never had any problems or bug with it. I'm not necessarily saying to bring it back but, at least display more than what I'd have by checking the raw log.

screenshot 2014-06-11 17 38 30

@taylorotwell
Copy link
Member

It’s white screening for me and one other person. That HAS to be fixed. It’s unacceptable. I can’t have literally one of the most crucial aspects of the framework in a “works on my machine” situation. That’s ridiculous.

On June 11, 2014 at 10:40:56 AM, Maxime Fabre (notifications@github.com) wrote:

I have nothing to take up, I've never had any problems or bug with it. I'm not necessarily saying to bring back but, at least display more than what I'd have by checking the raw log.


Reply to this email directly or view it on GitHub.

@spencerdeinum
Copy link
Contributor

It should be pretty trivial for someone to add Whoops themselves, hell someone could even make a package for it if they feel like it.

This is probably better off in community anyways, this feature really needs to be stable in the core.

@Anahkiasen
Copy link
Contributor

Again, I'm not saying to add it back, I'm saying if it's removed then Laravel needs something at least equivalent otherwise it's pretty pale in comparaison to other frameworks like Symfony.

screenshot 2014-06-11 17 52 41

@taylorotwell
Copy link
Member

To be fair there is nothing different there except showing the code, which is basically useless anyways. The file and line number is in the stack trace. Is that an editor built-in to the display or something? If not, I don’t see what that is buying me?

On June 11, 2014 at 10:55:00 AM, Maxime Fabre (notifications@github.com) wrote:

Again, I'm not saying to add it back, I'm saying if it's removed then Laravel needs something at least equivalent otherwise it's pretty pale in comparaison to other frameworks like Symfony.


Reply to this email directly or view it on GitHub.

@Anahkiasen
Copy link
Contributor

I can open every of those calls in the stack and see the line in context which prevents me from having to crawl through all the files one by one to find the right line. If you don't judge that useful I mean, it's your call, as others said, can still add Whoops back. It's just, was nice to have it by default. It's not a dramatic issue.

@taylorotwell
Copy link
Member

Well, it’s broken right now, and I’m certainly not digging into it right now because I’m working. Graham broke it the first time so I’m not sure if I want him fixing it either. If you want to figure out why it’s white screening on some people’s machine and fix it then have at it!

On June 11, 2014 at 10:58:47 AM, Maxime Fabre (notifications@github.com) wrote:

I can open every of those lines in the stack and see the line in context which prevents me from having to crawl through all the files one by one to find the right line. If you don't judge that useful I mean, it's your call, as others said, can still add Whoops back. It's just, was nice to have it by default. It's not a dramatic issue.


Reply to this email directly or view it on GitHub.

@barryvdh
Copy link
Contributor

As said before, another option would be just implementing Whoops without the custom resources (who cares if it's blue or orange).
But as said, it probably wouldn't be too hard to add a ServiceProvider for Laravel to Whoops (instead of a Whoops Serviceprovider in Laravel).
And if you just want the request vars etc and the lines around the exception, you could also use my laravel-debugbar of course ;) (as long as you use App::pushError() instead of App::error())

@taylorotwell
Copy link
Member

If someone wants to fix it then submit a pull request. Rip out all of the custom Laravel styles. We don't need to be maintaining those.

@GrahamCampbell
Copy link
Member Author

@taylorotwell Should we not just remove whoops entirely?

@GrahamCampbell
Copy link
Member Author

Could whoops be implemented by a third party laravel package instead?

@taylorotwell
Copy link
Member

I committed a tag that works no matter where you run optimize with the Symfony displayer. If someone wants to fix 4.2-dev that’s fine with me. Whoops is on 4.2-dev without any Laravel resources. Still doesn’t load if you run php artisan optimize on your host instead of the VM. It should work on either location, and it has for a long time.

On June 11, 2014 at 4:09:55 PM, Graham Campbell (notifications@github.com) wrote:

@taylorotwell Should we not just remove whoops entirely?


Reply to this email directly or view it on GitHub.

@taylorotwell
Copy link
Member

I fixed it. Removed PrettyPageHandler from the compile list. Something must have changed to break it when it compiles.

@AkenRoberts
Copy link
Contributor

The problem is two-fold:

  1. Whoops utilizes __DIR__ now to create a default resources path. Laravel's optimize is smart enough to replace it with the appropriate absolute directory path when compiling, but if done from the host machine rather than the VM, unless the absolute paths happen to be identical (unlikely), the default path will end up being incorrect.
  2. @GrahamCampbell original code for adding Laravel's resources path works fine, however, Whoops is still trying to retrieve lots of different resources. Since Laravel's directory only comes with frame_code.html.php, that is the only resource that will be found successfully. All other resources will not be found in the two paths (Laravel's, and the incorrect default path), hence errors such as the one @taylorotwell posted.

The solution: Besides removing PrettyPageHandler from the compiled list, the solution is to add the original Whoops resources path to Laravel's ExceptionServiceProvider. That way, even if there are no Laravel- or user-specific resource paths defined, the original Whoops path will still be found at run time, regardless of where optimize was run.

Unfortunately, this solution is dependent upon Whoops not changing its directory structure, capitalization, etc. So there is some dependency on Whoops to maintain non-BC changes. Removing the handler from the compile list, while retaining the additional resource path for Laravel-specific CSS/HTML is the most ideal solution to prevent issues in the future.

@crynobone
Copy link
Member

@cryode suggested solution would work fine until Whoops change the htmp at their end etc. It would require to basically lock Whoops to a specific version until the newer version compatibility can be tested.

@GrahamCampbell
Copy link
Member Author

@crynobone The whoops team said they wouldn't be making any breaking changes, Using 1.1.x should be fine.

@laravel laravel locked and limited conversation to collaborators Sep 26, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants