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

Symfony error pages are not being show correctly #63

Closed
murilolobato opened this issue Jun 30, 2017 · 61 comments
Closed

Symfony error pages are not being show correctly #63

murilolobato opened this issue Jun 30, 2017 · 61 comments

Comments

@murilolobato
Copy link

After I enable sentry, when a error 500 happens, the error page is not shown correctly. When I'm in 'dev' environment, the symfony debug page is not shown, as stated in the following images:

Bad:
bad

Good:
good

And when I'm in 'prod' environment, my customized error page is also not shown, as stated in the following images:

Bad:
bad2

Good:
good2

@Jean85
Copy link
Collaborator

Jean85 commented Jun 30, 2017

Can you provide us with a (redacted) copy of your config, regarding the Sentry bundle integration?

@murilolobato
Copy link
Author

Sure.

// app/config/parameters.yml

...
sentry_dsn_backend: null
sentry_dsn_frontend: null
sentry_environment: development
...

And:

// app/config/config.yml

...
sentry:
    dsn: "%sentry_dsn_backend%"
    release: "%sentry_release%"
    environment: "%sentry_environment%"
    skip_capture: [ Symfony\Component\HttpKernel\Exception\NotFoundHttpException, Lexik\Bundle\MaintenanceBundle\Exception\ServiceUnavailableException ]
...

@Jean85
Copy link
Collaborator

Jean85 commented Jun 30, 2017

  • What version are you using?
  • Do you see any changes between using 0.7.x and 0.8?
  • Do you use some custom code or custom listener priority for the error pages?

@murilolobato
Copy link
Author

  • I'm using 0.7.x;
  • No differences between 0.7.x and 0.8.x, problem occurs in both;
  • No custom listener.

@Jean85
Copy link
Collaborator

Jean85 commented Jul 4, 2017

I'm having issues trying to understand where the difference should be. Can you compare the listener behavior from the developer toolbar to see where the Sentry listener is interfering in this matter?

@murilolobato
Copy link
Author

Sorry, I don't know how to do that. I'll try, if I find something I will let you know, but until now I wasn't able to isolate the problem.

@Jean85
Copy link
Collaborator

Jean85 commented Jul 5, 2017

I was talking about the "Events" panel on the Symfony Profiler. Try to compare that.

@Jean85
Copy link
Collaborator

Jean85 commented Jul 27, 2017

Closing this a stale. Feel free to reply if the issue is still unresolved.

@Jean85 Jean85 closed this as completed Jul 27, 2017
@marcoreni
Copy link

I'm having a similar issue with v2.0 (I'm only enabling SentryBundle in prod environment). Right now the page that is being returned is blank and the status code is 500 (even it should be a 404, since I'm simply loading a non-existent page).

I have no custom error handlers, I just overwritten the twig templates as specified in the Symfony docs.

Sentry configuration has no option set other than the dns.

Can anyone help?

@Jean85
Copy link
Collaborator

Jean85 commented Jan 29, 2018

Since you got 500, you are getting an exception, probably it's a misconfiguration issue. Check your logs in var/logs/prod.log in production.

@murilolobato
Copy link
Author

murilolobato commented May 23, 2018

This error still happens, since the moment I enable it in AppKernel the error handling of my application start to act weird, as I described above.

This is for sure no support. This is a bug. I'll try to help, as soon as possible.

@murilolobato
Copy link
Author

This error is related with this listener: https://github.com/getsentry/sentry-symfony/blob/master/src/Resources/config/services.yml#L19

It actually doesn't matter the content of the method, just the fact of the listener being registered already breaks the expected behaviour. Anyone have any clue on how to fix this?

@Jean85
Copy link
Collaborator

Jean85 commented May 24, 2018

The listener is there to trigger the loading of the Sentry client service, which is a dependency of the listener itself; when the Sentry client is instantiated, it will register itself as an error handler, and it's possible that that's the origin of the issue.

What version of Symfony are you using? I just want to be sure that this isn't related to #119, which was solved by symfony/symfony#26568. That fix is included in Symfony version:

  • 2.7.44
  • 2.8.37
  • 3.4.7
  • 4.0.7

... or later patches.

@murilolobato
Copy link
Author

I'm using latest LTS, 3.4.10.

Actually, the Sentry\SentryBundle\EventListener\ExceptionListener listens to four events,

The line I've marked in my previous comment is the line that makes this class listen to the kernel.request event. If I'm not wrong, the method onKernelRequest of this class, is setting the current authenticated user to the Raven client.

What I'm saying, is that just the fact of this class listen to the kernel.request is breaking the expected behavior. I've came into this conclusion because I'm testing removing one tag at a time from the services.yml, and when I have the src/Resources/config/services.yml like the following:

services:
  sentry.client:
    class: '%sentry.client%'
    arguments: ['%sentry.dsn%', '%sentry.options%']
    public: true
    calls:
      - [install, []]

  sentry.exception_listener:
    class: '%sentry.exception_listener%'
    arguments:
      - '@sentry.client'
      - '@event_dispatcher'
      - '%sentry.skip_capture%'
      - '@?security.token_storage'
      - '@?security.authorization_checker'
    tags:
      - { name: kernel.event_listener, event: kernel.exception,  method: onKernelException, priority: '%sentry.listener_priorities.kernel_exception%' }
      - { name: kernel.event_listener, event: console.command,   method: onConsoleCommand }
      - { name: kernel.event_listener, event: console.error,     method: onConsoleError, priority: '%sentry.listener_priorities.console_exception%' }

Everything behave as expected. I don't think the issue you said has relation to this cause, I don't know why this weird behaviour is happening.

What is even more weird, is that if I leave the kernel.request registered on the services.yml, and clear the scope of the method in the ExceptionListener class, the bug persists. This is why I conclude, that is not a implementation problem, it is just the fact of listen to the kernel.request event that is causing this issue.

I'll try to change the moment where the authenticated user is being registered to the Raven client.

@murilolobato murilolobato changed the title Symfony error pages are not being show Symfony error pages are not being show correctly May 24, 2018
@murilolobato
Copy link
Author

I think we can re-open the issue, due to its importance, and it is now active again.

@murilolobato
Copy link
Author

I've discovered something really special.

Removing the kernel.request was a non sense solution, once the method wasn't doing anything wrong.

I've discovered that in the same service.yml, in the sentry.client definition, there was a call to the install method, and this call is changing the behavior of Symfony error handling. And why removing the tag kernel.request was making it work? Because when the tag exists, the container build add a higher priority to these instructions, and the install does something. When the tag is removed, these instructions loses priority and the install is no longer relevant, because Symfony already registered errors handlers.

My proposal is to remove the install call, because Symfony already is taking over errors and exceptions handling, and we can stick only to the kernel events to pass the errors to sentry.

@Jean85
Copy link
Collaborator

Jean85 commented May 25, 2018

I'm reopening the issue to follow through with your reports, thanks for the effort.

Yes, your finding are correct; the listener listens to the kernel.request event for two reasons: add user's info, and trigger the install() method; unfortunately, removing the install() call is not a feasible approach, because that covers a few kind of errors that Symfony cannot catch, like fatals or memory exhaustion.

BTW, I closed this because I was unable to reproduce the issue. Do you have the issue in the dev environment or in prod? Can you reproduce the issue with a minimal Symfony 3 standard installation, to have an easier to debug situation?

@Jean85 Jean85 reopened this May 25, 2018
@Jean85 Jean85 added Type: Bug and removed support labels May 25, 2018
@Jean85
Copy link
Collaborator

Jean85 commented May 25, 2018

Is it possible that this bug is related to #120 (comment)? Maybe we can apply the same solution, trigger the listener later, after the Symfony's error handler registration?

@ragusa87
Copy link

ragusa87 commented May 28, 2018

I can confirm that is related to #120, I am working on a fix.

@ragusa87 ragusa87 mentioned this issue May 28, 2018
@murilolobato
Copy link
Author

We are talking about different problems, but are some relation.

When you've said:

Yes, your finding are correct; the listener listens to the kernel.request event for two reasons: add user's info, and trigger the install() method; unfortunately, removing the install() call is not a feasible approach, because that covers a few kind of errors that Symfony cannot catch, like fatals or memory exhaustion.

You're saying that the bundle listens to kernel.request to make the install call. This makes no sense. The install is called from the dependency injection when the container is being built. The fact of the bundle is listening to the kernel.request makes it have a higher priority, and the install is being called in a early moment. Changing the priority of this listener is wrong by two reasons: The problem has nothing to do with this listener, and it will no solve the problem.

And about don't call the install, I don't have 100% sure, but I think that it will solve the problem and will no have any colateral effect. Because, Symfony, as I know, WILL catch all errors. Even the ones you said above.

I vote for removing the install call and do some testing. I will do this.

And about reproduce this, you can easily reproduce this in any symfony install you have. Just install the bundle, make an erroneus $foo->bar(); in any controller you would like, an access it through app.php and app_dev.php. You will achieve the exactly same results as me. (And I don't need to warn you about clear the cache)

@Jean85
Copy link
Collaborator

Jean85 commented May 29, 2018

Thanks @murilolobato for your continued effort.
I've investigated a little more about this, and it seems that you're correct; the "lazyness" of the listeners is probably not in effect for this one, since the kernel.request event is always called, as first.

It seems that you're also correct in pointing out that the kernel.exception event is always thrown for any kind of error, so even memory exhaustion errors are reported without the install(); that is GREAT news, we can simplify this a lot by removing that call, but I fear that that isn't feasible for PHP 5.6, which is supported by version 1.x of this bundle. This is because there we do not have \Throwable and catchable fatal errors.

Can you investigate on that? Otherwise I will be forced to implement this fix only in a future 2.1 version, supporting only PHP 7.1+

@starred-gijs
Copy link
Contributor

php 7.1.17 and Symfony 4.1.6

@Jean85
Copy link
Collaborator

Jean85 commented Oct 29, 2018

Ok, I had the time of using the reproducer given me by @PapyDanone.

I'm not sure, but it seems that Sentry registration mess up with the Symfony error handler. What I've seen is that the Symfony handler does handleException() and frees the memory, then afterward it calls handleFatalError(), it doesn't find any memory to free, and bails early. All of this without invoking the exception controller.

I think I'll report this to Symfony, but for this bundle the suggested workaround is to disable the Client::install() call, since it creates just a mess, and Symfony should trap any error anyway.

@PapyDanone
Copy link

Thanks for looking into this @Jean85

Note: I found this issue on the Symfony project that sounds very similar. Although it is marked as closed, it seems to have resurfaced for some.

the suggested workaround is to disable the Client::install() call

Could you please document the cleanest way to achieve this?

@Jean85
Copy link
Collaborator

Jean85 commented Oct 29, 2018

To disable the install, the fastest way right now is to re-declare the service, like in the code excert in #63 (comment)

@starred-gijs
Copy link
Contributor

starred-gijs commented Jan 3, 2019

Did anyone made progress on this? I may have some time to investigate if needed?

@Jean85
Copy link
Collaborator

Jean85 commented Jan 4, 2019

I don't have any more time to throw at this right now. Also, any spare time on my side will be dedicated on the next major of this bundle, to support the new Sentry 2.0 SDK; in that version, I hope to solve this issue once and for all.

@rpodwika
Copy link

rpodwika commented Jan 11, 2019

I cannot make it work I've tried with redeclaring the service omitting the call of install() also tried to inherit from class SentrySymfonyClient and override install()

I've created a class App\Sentry\Client

<?php

namespace App\Sentry;

use Sentry\SentryBundle\SentrySymfonyClient;

/**
 * Class Client
 *
 * Hack to make Raven Client not to register error handlers
 *
 * @package App\Sentry
 */
class Client extends SentrySymfonyClient
{
    /**
     * Overrides the method to disable error handlers registration
     *
     * @return $this|SentrySymfonyClient
     */
    public function install()
    {
        return $this;
    }
}

and in services.yaml

 sentry.client:
    class: App\Sentry\Client
    arguments: ['%sentry.dsn%', '%sentry.options%']
    public: true

It registers properly the sentry.client as App\Sentry\Client but when I trigger error it shows it natively and symfony's error handler does not seem to be called

php 7.1.11
symfony 4.2.2

@starred-gijs
Copy link
Contributor

I think this is fixed now by symfony/symfony#30264

@starred-gijs
Copy link
Contributor

Sorry, it is not. I was testing it in a production-like env and it fixed the situation where I throw an exception in the onKernelException event, but not actual syntax errors.

ps. Throwing an exception in onKernelException was a bit of weird solution anyway, so that is now replaced with $event->setException(...

@Jean85
Copy link
Collaborator

Jean85 commented Mar 6, 2019

I've just tagged the first beta for the 3.0 release, can you test against that?
Note that the base SDK is heavily different, keep it in mind if you have to interact directly with it.

@PapyDanone
Copy link

@Jean85 same results with 3.0.0-beta1 and latest Symfony, still based on the reproducer I created.

@Jean85
Copy link
Collaborator

Jean85 commented Mar 6, 2019

Ok, thanks @PapyDanone.

@PapyDanone
Copy link

Quick question for you @Jean85 . On 2.3.0, the skip_capture option doesn't seem to work. Do you think it is a side effect of having re-declared the service as suggested above? My config in packages/prod/sentry.yaml:

sentry:
    dsn: '%env(SENTRY_DSN)%'
    options:
        curl_method: async
    skip_capture: 
        - 'SevenShores\Hubspot\Exceptions\BadRequest'

services:
    sentry.client:
        class: '%sentry.client%'
        arguments: ['%sentry.dsn%', '%sentry.options%', '%sentry.skip_capture%']
        public: true

@Jean85
Copy link
Collaborator

Jean85 commented Apr 9, 2019

The skip capture option is passed to the listener, not the client. It's unrelated to this.

@starred-gijs
Copy link
Contributor

I can confirm that with the new release of this bundle 3.0, this issue is resolved, and the error page is shown correctly, and the error is captured in sentry.io

Awesome work @Jean85

@Jean85
Copy link
Collaborator

Jean85 commented May 15, 2019

Great, happy to hear that!

Thank you for testing & reporting, closing!

@Jean85 Jean85 closed this as completed May 15, 2019
@PapyDanone
Copy link

@starred-gijs did you test with the reproducer?

I'm still experiencing the same issue with sentry/sentry-symfony 3.0.0 and Symfony v4.2.8.

@starred-gijs
Copy link
Contributor

Nope I used my own project, that reproducer project indeed still has that bug. I will double check my project and see what is the difference.

@starred-gijs
Copy link
Contributor

No idea what is going on, but now it does not work anymore. Must have been a configuration issue on my side. Oops second time I incorrectly reported this issue as resolved, sorry for that.

@Jean85
Copy link
Collaborator

Jean85 commented May 16, 2019

Reopening. I'll try to dig deeper here, but I fear it's something deeply routed and hard to fix.

@Jean85 Jean85 reopened this May 16, 2019
@Jean85
Copy link
Collaborator

Jean85 commented May 16, 2019

I've launched it again and I may have found at least one precise problem; the error now is the following:

127.0.0.1:52854 [500]: / - Uncaught Symfony\Component\Debug\Exception\FatalThrowableError: syntax error, unexpected 'Class' (T_CLASS) in /home/alai/workspace/OSS/sentry-blank-page/src/Controller/DefaultController.php:15
Stack trace:
#0 /home/alai/workspace/OSS/sentry-blank-page/vendor/composer/ClassLoader.php(322): Composer\Autoload\includeFile('/home/alai/work...')
#1 [internal function]: Composer\Autoload\ClassLoader->loadClass('App\\Controller\\...')
#2 [internal function]: spl_autoload_call('App\\Controller\\...')
#3 /home/alai/workspace/OSS/sentry-blank-page/vendor/sentry/sentry/src/Serializer/AbstractSerializer.php(110): is_callable('App\\Controller\\...')
#4 /home/alai/workspace/OSS/sentry-blank-page/vendor/sentry/sentry/src/Serializer/RepresentationSerializer.php(18): Sentry\Serializer\AbstractSerializer->serializeRecursively('App\\Controller\\...')
#5 /home/alai/workspace/OSS/sentry-blank-page/vendor/sentry/sentry/src/Stacktrace.php(161): Sentry\Serializer\RepresentationSerializer->representationSerialize('App\\Controller\\... in /home/alai/workspace/OSS/sentry-blank-page/src/Controller/DefaultController.php on line 15

So it seems that Sentry is already kicking in and trying to serialize the controller for the callback, and doing that it fails due to autoloading a broken class. I'll try do see if this is fixable in the base SDK.

@Jean85
Copy link
Collaborator

Jean85 commented May 16, 2019

I can confirm that with getsentry/sentry-php#818 the issue is fixed! 🎉

You can try it out with composer require "sentry/sentry:dev-serializer-error as 2.0.2".

I'll try to check if I can backport it to previous versions too.
[EDIT] Nope, it's not the same issue there, I don't know how to fix that.

@Jean85
Copy link
Collaborator

Jean85 commented May 22, 2019

getsentry/sentry-php#818 got merged and released in sentry/sentry 2.1.0 🎉

You can now fix everything with a simple composer update sentry/sentry. Closing as solved.

@Jean85 Jean85 closed this as completed May 22, 2019
@PapyDanone
Copy link

Thanks for all the work @Jean85 🙏

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

7 participants