mongodb
trigger_error directly in file causes unpredictable system failure
- Laravel-mongodb Version: 5.5+
- PHP Version: doesn't matter
- Database Driver & Version: doesn't matter
Description:
PR https://github.com/mongodb/laravel-mongodb/pull/3408 adds trigger_error into the file directly (not inside a method). Since it is quite typical for developers to override error reporting in runtime (e.g., error_reporting(E_ALL & ~E_DEPRECATED & ~E_USER_DEPRECATED), rather than in .ini, the following happens:
On the local env/test server, the app works without caching, and the SoftDeletes class is only included/required after the initial bootstrap, and the error is ignored.
Eventually, the app is deployed to the production environment, which runs on fpm with opcache enabled, which "includes" all files before the app is bootstrapped. The app crashes because error reporting has not yet been configured to exclude deprecation warnings.
You can see that in this scenario, the problem would go unnoticed until the code was deployed into the production environment. To make things worse, developers cannot rely on error reporting tools like Sentry, because the error happens much earlier than the error handler is initialized.
Steps to reproduce
- make sure .ini allows deprecation errors reporting
- change error reporting in kernel/bootstrap to ignore deprecation warnings
- launch the app without opcache enabled (verify, OK)
- enable opcache
Expected behaviour
app is functional
Actual behaviour
app crashes
Thoughts:
A file with a class definition should not have any side effects, only the class declaration; otherwise, there's a risk of causing problems that will be very hard to spot until too late. In this case, php-fpm and opcache, combined with not relying on .ini for error reporting settings, caused this issue. Side effects could appear for worker-mode-based systems, etc.I understand the challenge of reporting whole class deprecation (in contrast of particular method deprecation), but there should be a less dangerous way.
I just realized that if I configure opcache preload to include all vendor folder, unless I change .ini, my app will break even if it doesn't use SoftDeletes trait
Blindly pre-loading all the classes from the vendor dir is counterproductive to performance because it overloads the memory. Why does your default configuration in production convert deprecations into fatal errors? It seems to me that this is more of a configuration issue on your end. If we remove this deprecation, it will be ignored.
@GromNaN yes, this is certainly an improper configuration on our server, and I already changed it to avoid this problem in the future. I'm pointing out that many devs have it the same way, and doing something "right", even if it hurts a lot of users, is not the best approach.
Although, from the code quality perspective, this approach is also imperfect, since it produces side effects (no matter how small they are). The file must only contain a class declaration, and the inclusion must only boot the class. Any executable code must sit in index.php or files specifically dedicated to it, but not the files with class declarations.