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

config: change the default error reporting level in production #8219

Merged

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Nov 15, 2023

Description
There is no reason to change error level only in production.

In production, you may wish to set this to a less verbose level such as E_ALL & ~E_NOTICE & ~E_DEPRECATED, but in many cases E_ALL is also appropriate, as it may provide early warning of potential issues.
https://www.php.net/manual/en/language.errors.basics.php#language.errors.basics.handling

Checklist:

  • Securely signed commits
  • [] Component(s) with PHPDoc blocks, only if necessary or adds value
  • [] Unit testing, with >80% coverage
  • [] User guide updated
  • Conforms to style guide

@kenjis kenjis added the 4.5 label Nov 15, 2023
@michalsn
Copy link
Member

I kindly disagree. In a production environment, we generally want to suppress certain types of errors and notices that will not impact the end user. This could potentially cause our logs to grow to horrible sizes. Also, the readability of logs would suffer.

Early warning and issues should be caught in a development environment or potentially in staging.

@kenjis
Copy link
Member Author

kenjis commented Nov 15, 2023

Thanks for your comment.

I don't believe PHP's error level can tell the errors and notices will impact the end user or not.
In development environment, if a NOTICE is issued, it will be an ErrorException and it will stop the app.
It would be more frightening if the app continues to run without stopping in spite of the errors.
Personally, I don't like the behavior to change only in production environment.
We can't test that behavior in development/testing environment.

Of course, if your app works already with Warnings and Notices, you need to suppress errors.
You can do it with your config file.
However, I do not believe that such apps should be assumed as the default for the framework.

@github-actions github-actions bot added the stale Pull requests with conflicts label Nov 20, 2023
Copy link

👋 Hi, @kenjis!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

@kenjis kenjis force-pushed the change-error_reporting-in-production branch from 7b68ea9 to b6ab73f Compare November 20, 2023 04:58
@kenjis kenjis removed the stale Pull requests with conflicts label Nov 20, 2023
@MGatner
Copy link
Member

MGatner commented Nov 23, 2023

I don't know that this is a place for the framework to exert a strong opinion. What are the defaults these days in production versus development php.ini files? I would consider either mirroring those or commenting this out entirely to pass handling to the INI.

@kenjis
Copy link
Member Author

kenjis commented Nov 24, 2023

; error_reporting
;   Default Value: E_ALL
;   Development Value: E_ALL
;   Production Value: E_ALL & ~E_DEPRECATED & ~E_STRICT

https://github.com/php/php-src/blob/a80b6d7b99ae885cb450a563a788f57917cef74e/php.ini-production#L107-L110

See https://wiki.php.net/rfc/reclassify_e_strict

@MGatner
Copy link
Member

MGatner commented Nov 25, 2023

Hmm interesting. So it looks like E_STRICT doesn't do anything. So if we wanted to match php.ini in a >= PHP7 manner it would be:

E_ALL & ~E_DEPRECATED

That seems reasonable to me since deprecations are frequently unavoidable in production (I have third-party libraries trigger them all the time). There's a certain weight of argument though to the fact that we've been suppressing these since alpha 1.

@kenjis kenjis force-pushed the change-error_reporting-in-production branch from b6ab73f to 2a2748d Compare November 25, 2023 22:12
@kenjis
Copy link
Member Author

kenjis commented Nov 25, 2023

Okay, that seems reasonable. Changed to E_ALL & ~E_DEPRECATED.

@kenjis kenjis force-pushed the change-error_reporting-in-production branch from 2a2748d to 90348ef Compare November 25, 2023 22:14
@github-actions github-actions bot added the stale Pull requests with conflicts label Nov 27, 2023
Copy link

👋 Hi, @kenjis!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

@kenjis kenjis force-pushed the change-error_reporting-in-production branch from 90348ef to 017a5c4 Compare November 29, 2023 03:13
@kenjis
Copy link
Member Author

kenjis commented Nov 29, 2023

Rebased to resolve conflict.

@kenjis kenjis removed the stale Pull requests with conflicts label Nov 29, 2023
Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

Good by me! @michalsn any further pushback?

@kenjis kenjis merged commit f9679ec into codeigniter4:4.5 Dec 3, 2023
46 checks passed
@kenjis kenjis deleted the change-error_reporting-in-production branch December 3, 2023 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants