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

replace -1 with E_ALL in error_reporting calls #8212

Merged
merged 1 commit into from
Dec 17, 2023

Conversation

ThomasMeschke
Copy link
Contributor

Description
Replaces error_reporting(-1); with error_reporting(E_ALL); for better readability.

Checklist:

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

O = does not apply

@kenjis
Copy link
Member

kenjis commented Nov 15, 2023

Thank you for sending PR!

But I don't see the benefit to change.

@kenjis kenjis added the refactor Pull requests that refactor code label Nov 15, 2023
@ThomasMeschke
Copy link
Contributor Author

Only benefit is readability =)
E_ALL is clear for everybody and no need to go and check the doc what "-1" means.
No impact when merging, no impact when not merging, but also no loss if merging. =)

@kenjis
Copy link
Member

kenjis commented Nov 15, 2023

Related #8219

@kenjis
Copy link
Member

kenjis commented Nov 15, 2023

Certainly E_ALL is easier to understand, but it seems difficult for someone who does not know what -1 is to understand the meaning of error_reporting().

I would like to hear what others say.

Copy link
Contributor

@datamweb datamweb left a comment

Choose a reason for hiding this comment

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

I believe the first user contribution should be reviewed with kindness. I'm fine with that.

@ThomasMeschke
Copy link
Contributor Author

No further comments for a week now. Let's not make this a long-runner, shall we?

@paulbalandan
Copy link
Member

From ChatGPT:

In PHP, both error_reporting(-1); and error_reporting(E_ALL); are used to set the error reporting level to the maximum, meaning that all types of errors, warnings, and notices will be reported.

The difference between the two is subtle:

  1. error_reporting(-1);: This sets the error reporting level to the maximum possible value. It includes all error types, even those that might be added in future PHP versions. Using -1 is more future-proof, as it ensures that your code will catch all errors, including any new ones introduced in later PHP versions.

  2. error_reporting(E_ALL);: This explicitly sets the error reporting level to include all errors, warnings, and notices. While this achieves the same result as -1 in current PHP versions, it might not include potential new error types introduced in future PHP versions.

In practice, either of these statements is generally acceptable for development environments where you want to catch and address all types of errors. However, if you want to be as future-proof as possible, using error_reporting(-1); might be a slightly safer choice.

For production environments, it's recommended to log errors instead of displaying them to users, and the error reporting level might be set to a lower value to avoid exposing sensitive information.

https://chat.openai.com/c/c76a03d0-714d-493b-b990-f5b92c3e51ac

@kenjis
Copy link
Member

kenjis commented Nov 21, 2023

The ChatGPT answer is not correct.

And since error levels will be added over time, the maximum value (for E_ALL) will likely change.
https://www.php.net/manual/en/errorfunc.configuration.php

@ThomasMeschke
Copy link
Contributor Author

I would prefer to trust the official documentation which in the Example and the Note declares: They are the same.
https://www.php.net/manual/en/function.error-reporting

From ChatGPT:

In PHP, both error_reporting(-1); and error_reporting(E_ALL); are used to set the error reporting level to the maximum, meaning that all types of errors, warnings, and notices will be reported.

The difference between the two is subtle:

  1. error_reporting(-1);: This sets the error reporting level to the maximum possible value. It includes all error types, even those that might be added in future PHP versions. Using -1 is more future-proof, as it ensures that your code will catch all errors, including any new ones introduced in later PHP versions.
  2. error_reporting(E_ALL);: This explicitly sets the error reporting level to include all errors, warnings, and notices. While this achieves the same result as -1 in current PHP versions, it might not include potential new error types introduced in future PHP versions.

In practice, either of these statements is generally acceptable for development environments where you want to catch and address all types of errors. However, if you want to be as future-proof as possible, using error_reporting(-1); might be a slightly safer choice.

For production environments, it's recommended to log errors instead of displaying them to users, and the error reporting level might be set to a lower value to avoid exposing sensitive information.

https://chat.openai.com/c/c76a03d0-714d-493b-b990-f5b92c3e51ac

@datamweb
Copy link
Contributor

php.ini say:

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
; Error handling and logging ;
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

; This directive informs PHP of which errors, warnings and notices you would like
; it to take action for. The recommended way of setting values for this
; directive is through the use of the error level constants and bitwise
; operators. The error level constants are below here for convenience as well as
; some common settings and their meanings.
; By default, PHP is set to take action on all errors, notices and warnings EXCEPT
; those related to E_NOTICE and E_STRICT, which together cover best practices and
; recommended coding standards in PHP. For performance reasons, this is the
; recommend error reporting setting. Your production server shouldn't be wasting
; resources complaining about best practices and coding standards. That's what
; development servers and development settings are for.
; Note: The php.ini-development file has this setting as E_ALL. This
; means it pretty much reports everything which is exactly what you want during
; development and early testing.
;
; Error Level Constants:
; E_ALL             - All errors and warnings
; E_ERROR           - fatal run-time errors
; E_RECOVERABLE_ERROR  - almost fatal run-time errors
; E_WARNING         - run-time warnings (non-fatal errors)
; E_PARSE           - compile-time parse errors
; E_NOTICE          - run-time notices (these are warnings which often result
;                     from a bug in your code, but it's possible that it was
;                     intentional (e.g., using an uninitialized variable and
;                     relying on the fact it is automatically initialized to an
;                     empty string)
; E_STRICT          - run-time notices, enable to have PHP suggest changes
;                     to your code which will ensure the best interoperability
;                     and forward compatibility of your code
; E_CORE_ERROR      - fatal errors that occur during PHP's initial startup
; E_CORE_WARNING    - warnings (non-fatal errors) that occur during PHP's
;                     initial startup
; E_COMPILE_ERROR   - fatal compile-time errors
; E_COMPILE_WARNING - compile-time warnings (non-fatal errors)
; E_USER_ERROR      - user-generated error message
; E_USER_WARNING    - user-generated warning message
; E_USER_NOTICE     - user-generated notice message
; E_DEPRECATED      - warn about code that will not work in future versions
;                     of PHP
; E_USER_DEPRECATED - user-generated deprecation warnings
;
; Common Values:
;   E_ALL (Show all errors, warnings and notices including coding standards.)
;   E_ALL & ~E_NOTICE  (Show all errors, except for notices)
;   E_ALL & ~E_NOTICE & ~E_STRICT  (Show all errors, except for notices and coding standards warnings.)
;   E_COMPILE_ERROR|E_RECOVERABLE_ERROR|E_ERROR|E_CORE_ERROR  (Show only errors)
; Default Value: E_ALL
; Development Value: E_ALL
; Production Value: E_ALL & ~E_DEPRECATED & ~E_STRICT
; https://php.net/error-reporting
error_reporting = E_ALL

@kenjis
Copy link
Member

kenjis commented Nov 22, 2023

E_ALL and -1 mean the same thing.
However, the value of E_ALL may change in the future.

It is a bit special that -1 means all, but the value does not change, and -1 is easier to understand than 32767 when checking the value of error_reporting().

Before:

error_reporting() integer -1

After:

error_reporting() integer 32767
<?php

namespace App\Controllers;

class Home extends BaseController
{
    public function index(): string
    {
        dd(error_reporting());
    }
}

@kenjis
Copy link
Member

kenjis commented Nov 22, 2023

I prefer not to change it because -1 is easier to understand.

By the way, I am also against changing this value in production environment.
See #8219

@MGatner
Copy link
Member

MGatner commented Nov 23, 2023

Well, today I learned that E_ALL != -1 😅

The docs are clear using either is equivalent, as @ThomasMeschke noted:

Passing in the value -1 will show every possible error, even when new levels and constants are added in future PHP versions. The behavior is equivalent to passing E_ALL constant.

E_ALL is easier to read in the code but -1 is more discernible in debug. I really don't have a preference - I would be inclined to agree with @datamweb that we merge just for the joy of having a new contributor (thanks @ThomasMeschke!) but @kenjis pours his life essence into this project so if he believes in -1 then let's stick with that.

@MGatner
Copy link
Member

MGatner commented Nov 23, 2023

Also: shoutout to the ChatGPT cameo - showing up just in time with a confabulation 🙄

@datamweb
Copy link
Contributor

E_ALL and -1 mean the same thing. However, the value of E_ALL may change in the future.

It is a bit special that -1 means all, but the value does not change, and -1 is easier to understand than 32767 when checking the value of error_reporting().

Before:

error_reporting() integer -1

After:

error_reporting() integer 32767
<?php

namespace App\Controllers;

class Home extends BaseController
{
    public function index(): string
    {
        dd(error_reporting());
    }
}

It was an interesting explanation.

but @kenjis pours his life essence into this project

Yes, it really is.

The good thing about this conversation was that I learned new things.
Thanks all!

@lonnieezell
Copy link
Member

First off, @ThomasMeschke thanks for your first PR.

So what was decided? Sounds like we're keeping it at -1? If that's so can we close this? Or are we still debating?

I don't have strong feelings either way. I do think that E_ALL is more readable so would be inclined to switch it, especially since the official docs say they're the same thing.

Tip Passing in the value -1 will show every possible error, even when new levels and constants are added in future PHP versions. The behavior is equivalent to passing E_ALL constant.

It also seems we just switched production to use E_ALL as part of it's default setting.

Again - no strong opinion but I do lean toward more readable. Always. :)

@MGatner MGatner requested a review from kenjis December 16, 2023 12:16
Copy link
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

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

In production, we use and will use E_ALL. So let's use it in other places.

@kenjis kenjis merged commit 521cc98 into codeigniter4:develop Dec 17, 2023
61 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Pull requests that refactor code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants