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

Front controller won't catch exceptions of type Error #201

Open
fredericgboutin-yapla opened this issue Nov 1, 2024 · 4 comments
Open

Comments

@fredericgboutin-yapla
Copy link
Contributor

fredericgboutin-yapla commented Nov 1, 2024

Context

In recent versions of PHP, several so called errors migrated to be thrown as exceptions instead. The wording is confusing here, but the new class they created is named Error and it does NOT inherit from Exception but directly from Throwable.

What does that mean?

Let's take an example, a division by zero.

In PHP 7, dividing by zero would generate an error of type E_Warning and continue its execution.

In PHP 8, dividing by zero leads to a DivisionByZeroError exception being thrown.

Let's imagine that division by zero happened when dispatching / rendering a page, for example in the Action of a Controller. Normally, in that context, any Exception would "bubble up" through the Front and the Standard controllers dispatch()methods and be handled properly by Zend - see https://github.com/zf1s/zf1/blob/7d4b8fe7848bacd0836cc7fa5f987c7b1f0fbb2e/packages/zend-controller/library/Zend/Controller/Front.php#L980C8-L980C33

The problem is, since a DivisionByZeroError is NOT an Exception it doesn't get catch by Zend and it ends up as a global exception.

What's the problem

Well, Errors (and any Throwable) are ending up as global unhandled exceptions, with no graceful "bailout" or anything and this is something that was introduced in PHP 7 but that is getting more frequent in PHP 8 and would probably continue like that in future PHP versions as the authors are laying more on exceptions than errors.

Proposition

I suppose we should consider revisiting those try/catch in the controllers involved in dispatching, and change Exception to Throwable, or at the very least add the Error alongside Exception handling.

The minimal would be to adjust both the Front and the Standard controllers.

What do you think ?

@fredericgboutin-yapla
Copy link
Contributor Author

fredericgboutin-yapla commented Nov 1, 2024

I would also argue that, as we are moving forward with PHP 8, we are trying to strongly type our code. But as we do, there are inevitable errors that we discover in production with real life data.

We had an example like that, here is a simplified version where we decided to strongly type the $arg with an array but somehow somewhere it happened it was called with a string value instead,

function myFunction(array $arg) {
    // ...
}

myFunction("oh no!"); // throwing a TypeError

In PHP 7 and 8 it throws a TypeError that is not catched by Zend to be gracefully handled and I think it would be useful.

@fredericgboutin-yapla
Copy link
Contributor Author

fredericgboutin-yapla commented Nov 1, 2024

Another side effect we experienced was that in our setup we have both Monolog and Sentry integrated. And because Zend wouldn't catch the Error, the global handler would kick in.

The problem is, Monolog and Sentry each have dedicated Error and Exception handlers, so we ended up with both Monolog and Sentry reporting the same Error. And since we are also using the Sentry monolog handler, we ended up with 2 different events for the same Error in Sentry.

What a mess.

@falkenhawk
Copy link
Member

falkenhawk commented Nov 4, 2024

@fredericgboutin-yapla do you think that could help? Shardj/zf1-future#90
but, I am afraid this would break php 5.x, but maybe Throwable could be polyfilled there 🤔
As a side note, when you say I would also argue that, as we are moving forward with PHP 8, we are trying to strongly type our code I would suggest to try to move away from zf1 at the same time, while making code more strict and modern 😅

@fredericgboutin-yapla
Copy link
Contributor Author

do you think that could help? Shardj/zf1-future#90

It seems incomplete to me. It is missing the Standard dispatcher modifications following the Front dispatcher.

maybe Throwable could be polyfilled there

Hum, symfony/polyfill#101

But at least we could polyfill the Error class like they are doing, https://github.com/symfony/polyfill-php70/blob/v1.19.0/Resources/stubs/Error.php

I would suggest to try move away from zf1 at the same time, while making code more strict and modern 😅

I see... I really like the stability of this project compared to zf1-future, thought. But you are probably right.

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

No branches or pull requests

2 participants