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

Make error handling a bit more friendly #35381

Closed

Conversation

lcobucci
Copy link

@lcobucci lcobucci commented Nov 26, 2020

Hello 👋

After releasing a new version of one of my libs I received several messages from Laravel users saying that their apps were broken due to deprecation messages:

This is then an attempt to help other library authors/maintainers not to break end user's apps.

Main idea behind this is to trust folks to properly configure their
environment on production.

Behaviour is kept, though, on other environments to help users catching
potential problems earlier.
@lcobucci lcobucci force-pushed the make-error-handling-a-bit-more-friendly branch from bbbd6ad to 33b3fae Compare November 26, 2020 23:20
This ensures that deprecation messages aren't treated as hard errors
(a.k.a. a broken app) on Laravel apps.

These messages are still logged so that people are aware that they can
improve things.
Comment on lines +41 to +43
if (! $app->environment('production')) {
error_reporting(-1);
}
Copy link
Author

Choose a reason for hiding this comment

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

I honestly thought about removing this line but it exists since forever - for a reason which I couldn't find in the history.

@GrahamCampbell
Copy link
Member

I don't think we can make this kind of change to 8.x, since it's breaking. Maybe this could be considered for master, though.

@GrahamCampbell
Copy link
Member

GrahamCampbell commented Nov 27, 2020

How are you firing your deprecation errors? They should be silenced like how Symfony does it. Laravel 6 definitely calls deprecated symfony code which fires silenced deprecation errors, but we don't get complaints from people there?

@GrahamCampbell
Copy link
Member

Yep, you need to add an @ sign here: lcobucci/jwt@5ce37ae.

@GrahamCampbell
Copy link
Member

GrahamCampbell commented Nov 27, 2020

@lcobucci
Copy link
Author

@GrahamCampbell I disagree with this being a breaking change but your package your rules.

I've explained the technical details on why @ wasn't used in lcobucci/jwt#550 (comment) and we don't need to go over the options and debate them here too.

It would be great to have the technical details of why error handling is so aggressive in Laravel (just so your users are aware of the fact that even deprecation in the language crashes their app).

Have a great day 👋

@lcobucci lcobucci deleted the make-error-handling-a-bit-more-friendly branch November 27, 2020 07:21
@driesvints
Copy link
Member

I've been talking to @lcobucci about this and I'm going to investigate all this soon when I find the time for it. I agree we can't make this change on a patch release and at the moment I think I also lean towards removing that line completely. But I need some more time to investigate the actual consequences of doing that and check with the community on how it effects the ecosystem around the framework.

@GrahamCampbell
Copy link
Member

I've sent a PR to fix your package: lcobucci/jwt#568.

baijunyao added a commit to baijunyao/laravel-bjyblog that referenced this pull request Nov 28, 2020
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

Successfully merging this pull request may close these issues.

3 participants