Unwrap HandlerFailedException for sync Messenger messages too

[Jean85]

Will fix #469

[Ocramius]

assuming (needs to be checked by maintainers) that HandlerFailedException can be un-wrapped by default within the context of ErrorListener#handleExceptionEvent(), I'm totally for this patch 👍

[Jean85]

IMO it should, because it can also contain multiple exceptions, due to multiple handlers for the same message, failing together.

[ste93cry]

Build is broken 😬

[Jean85]

CI failure seems triggered by symfony/symfony#40677

If we cannot fix it, we can (temporarily?) switch to pcov for coverage collection?

[Ocramius]

@Jean85 IMO using pcov is more reliable, if this stuff is only used for coverage.

[Ocramius]

There was 1 failure:

1) Sentry\SentryBundle\Tests\End2End\End2EndTest::testGetFatal
Wrong number of events sent: 
7222250960a0483195f2e5ea66896f9e: Error: Class Serializable@anonymous contains 2 abstract methods and must therefore be declared abstract or implement the remaining methods (Serializable::serialize, Serializable::unserialize)
###
3284d7459e2d4725a34edd8ee85ce6c8: Error: Class Serializable@anonymous contains 2 abstract methods and must therefore be declared abstract or implement the remaining methods (Serializable::serialize, Serializable::unserialize)
###

Failed asserting that actual size 2 matches expected size 1.

/home/runner/work/sentry-symfony/sentry-symfony/tests/End2End/End2EndTest.php:258
/home/runner/work/sentry-symfony/sentry-symfony/tests/End2End/End2EndTest.php:157

I wonder if that's because the messenger encounters the fatal error twice? Then the exception would indeed be captured twice, with the current change.

[Jean85]

Why would encounter it twice?

Also, tests do NOT fail locally, there's definitely something wonky here..

[Ocramius]

The code around the assertions looks completely wonky:

sentry-symfony/tests/End2End/End2EndTest.php

Lines 141 to 155 in 43cd505

	try {
	$client->insulate(true);
	$client->request('GET', '/fatal');
	$response = $client->getResponse();
	$this->assertInstanceOf(Response::class, $response);
	$this->assertSame(500, $response->getStatusCode());
	$this->assertStringNotContainsString('not happen', $response->getContent() ?: '');
	} catch (\RuntimeException $exception) {
	$this->assertStringContainsStringIgnoringCase('error', $exception->getMessage());
	$this->assertStringContainsStringIgnoringCase('contains 2 abstract methods', $exception->getMessage());
	$this->assertStringContainsStringIgnoringCase('MainController.php', $exception->getMessage());
	$this->assertStringContainsStringIgnoringCase('eval()\'d code on line', $exception->getMessage());
	}

The } catch { block should not exist there, should it?

[Jean85]

IIRC that was done in #322 due to the fact that with different Symfony version you would get different behaviours..

Let's see what happens if I revert that.

[EDIT] The reason is a bit more nuanced:

• that test triggers a fatal through evaling a string which contains an anonymous empty class extending \Serializable:

  sentry-symfony/tests/End2End/App/Controller/MainController.php

  Line 44 in 43cd505

  $foo = eval("return new class() implements \Serializable {};");

• hence $client->insulate(true), which runs the request inside a process to avoid bringing down the whole test with it
• code throws at $client->request(..), so the test continues flowing from inside the catch block
• code after $client->request(..) is just to capture behaviour if the app under test is behaving badly: controller has a fixed response if it doesn't blow up as expected:

  sentry-symfony/tests/End2End/App/Controller/MainController.php

  Line 46 in 43cd505

  return new Response('This response should not happen: ' . json_encode($foo));

[github-actions]

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

---

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀