Uploaded image for project: 'phpBB3'
  1. phpBB3
  2. PHPBB3-9079

Display backtrace on all E_USER_ERROR errors, not only SQL errors (when DEBUG_EXTRA is enabled)

    Details

    • Type: Bug
    • Status: Unverified Fix
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 3.0.x
    • Fix Version/s: 3.0.11-RC1
    • Component/s: Other
    • Labels:
      None

      Description

      Display backtrace if DEBUG_EXTRA is enabled (not just DEBUG as was suggested in IRC, went with DEBUG_EXTRA because that is what the DBAL displays the backtrace on).

      Also, always logs the backtrace to the critical log.

      1. backtrace.diff
        0.8 kB
        EXreaction [X]

        Issue Links

          Activity

          Hide
          leviatan21 leviatan21 [X] (Inactive) added a comment -

          Nice, but why the double code ?

          Show
          leviatan21 leviatan21 [X] (Inactive) added a comment - Nice, but why the double code ?
          Hide
          nickvergessen Joas Schilling added a comment -

          Why would you only log the backtrace with that conditions? I'd simply always log that...

          Show
          nickvergessen Joas Schilling added a comment - Why would you only log the backtrace with that conditions? I'd simply always log that...
          Hide
          EXreaction EXreaction [X] (Inactive) added a comment -

          Because otherwise the backtrace goes in twice...

          You could put the backtrace code below the log part and remove the inline if to always put the backtrace in and add the backtrace to the output after that.

          Show
          EXreaction EXreaction [X] (Inactive) added a comment - Because otherwise the backtrace goes in twice... You could put the backtrace code below the log part and remove the inline if to always put the backtrace in and add the backtrace to the output after that.
          Hide
          Oleg Oleg [X] (Inactive) added a comment -

          The "always logs the backtrace to the critical log" part of the patch was deleted, was this intended?

          Show
          Oleg Oleg [X] (Inactive) added a comment - The "always logs the backtrace to the critical log" part of the patch was deleted, was this intended?
          Hide
          bantu Andreas Fischer added a comment -

          Yes, it was intentional. I wanted to make the SQL error logging code consistent with the message handler error logging code. Looking at the code again, I'd actually suggest removing the $auth->acl_get('a_') condition and only add BACKTRACE if DEBUG_EXTRA is defined. It seems a little weird to me to only have a backtrace in the logs when the error is generated by an admin. What do you think?

          Show
          bantu Andreas Fischer added a comment - Yes, it was intentional. I wanted to make the SQL error logging code consistent with the message handler error logging code. Looking at the code again, I'd actually suggest removing the $auth->acl_get('a_') condition and only add BACKTRACE if DEBUG_EXTRA is defined. It seems a little weird to me to only have a backtrace in the logs when the error is generated by an admin. What do you think?
          Hide
          Oleg Oleg [X] (Inactive) added a comment - - edited

          Looks like with the patch applied the same message is logged and printed. Therefore if you remove acl check non-admins will also see the backtrace.

          Always logging the backtrace seems like the right thing to do.

          I see what Nathan was referring to earlier. I suggest simply having two separate variables, one for printed message and one for logged message.

          Show
          Oleg Oleg [X] (Inactive) added a comment - - edited Looks like with the patch applied the same message is logged and printed. Therefore if you remove acl check non-admins will also see the backtrace. Always logging the backtrace seems like the right thing to do. I see what Nathan was referring to earlier. I suggest simply having two separate variables, one for printed message and one for logged message.
          Show
          Oleg Oleg [X] (Inactive) added a comment - https://github.com/phpbb/phpbb3/pull/306#issuecomment-2716026
          Hide
          Oleg Oleg [X] (Inactive) added a comment -

          The backtrace stuff is fixed. However, the SQL bit that is left in dbal (see https://github.com/phpbb/phpbb3/pull/306 diff) does not have the "correct" behavior: SQL is logged or not logged depending on whether it is shown (which requires admin auth or debug flags turned on). I'm going to append this to http://tracker.phpbb.com/browse/PHPBB3-9359 for now instead of creating a new ticket.

          Show
          Oleg Oleg [X] (Inactive) added a comment - The backtrace stuff is fixed. However, the SQL bit that is left in dbal (see https://github.com/phpbb/phpbb3/pull/306 diff) does not have the "correct" behavior: SQL is logged or not logged depending on whether it is shown (which requires admin auth or debug flags turned on). I'm going to append this to http://tracker.phpbb.com/browse/PHPBB3-9359 for now instead of creating a new ticket.

            People

            • Assignee:
              bantu Andreas Fischer
              Reporter:
              EXreaction EXreaction [X] (Inactive)
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development