Starting with Drupal 7, the system has become extremely modular. Generic functions are re-used in many places, so it's a horrible job to figure out where exactly an error originates from.
Issues like #1067750: Let Field API fail in a tale-telling way on invalid $entity only underline the problem space.
There are several problems with introducing a backtrace and the current patch attempts to address them. Some issues are:
We need better reporting for developers
A clear formatting has been setup and can easily be customized with a little CSS.
There is no one reporting format that will be universally useful
Various options to set the level of detail returned are available.
We don't want to make things complicated for the average user
These extra options are hidden in a collapsed fieldset and described as "being for developers."
We need to avoid possibly crashing from processing large variables that may even have recursion
Defaults to limit the length of variables that can be returned yet is adjustable if a developer needs more information.
Keeping it simple
The added options are added to the familiar: admin/config/development/logging configuration page.
Note: This patch may look large at first glance but once you mitigate the constant definitions at the top, the functions added to the bottom of errors.inc which only provide some nice formatting and the additions to the UI at admin/config/development/logging, it's quite modest.
Comments
Comment #1
sunComment #2
pounardYou forgot probably the most important, exception stack trace. While PHP fatal errors will make you unable to get to this, and while common warnings can be catched and probably need this exact backtrace, most of the work should focus on loosy exception catching that core does and should probably push a "production mode" concept into core for development and testing sites.
Comment #4
Lars Toomre CreditAttribution: Lars Toomre commented@pounard - Could elaborate on what you meant by 'loosy exception catching' and a 'production mode concept'?
Are you suggesting that core should respond if a different way to a variable indicating whether a site is development/staging/production (best practices BTW)?
Comment #5
pounard@#4, For the later, yes totally what you said: most frameworks and piece of software allows to configure, at least, a production and a devel mode. ZF allows user to set as many "modes" as they can via configuration override (through the Zend_Config API). Each "mode" is (for the most, outside the site configuration) a set of variables such as error display, error logging, etc..
Drupal has the error level variable, but it's pretty much all that you can have, having finer granularity over "development" mode could be better to somehow force modules to use that in order to do their own different processing depending on the runtime mode (and development mode could mean different things among different sites, so it's basically a pure set of variables). For the most, the core isn't design to have a devel mode, and the error display handler is totally unable to display back trace. I'm actually fed off switching 10 variables to // comments each time I need to this while a single named parameter in the settings.php file (or even as environment variable inside the HTTPd configuration) would be more than enough to trigger a packaged set of variable overrides).
For the first part, about exception, there is something that is actually really anoying with D7, is that when you raise some exceptions, the sites goes to fallback maintenance theme (which is a good fallback mode actually) but doesn't display the exceptions stack trace (which imposes the developer to go to watchdog screen). While some errors can sometime prevent any page to be displayed (including watchdog screen, known as recent log entries), the devel mode should always trigger the full trace to be displayed by the error handler whatever happens.
I did this exact patch on a development site once, I should have kept it (trashed it actually). The actual functions in error.inc file are absolutely not designed to do this (they only keep the error message, that should actually not get to the end user when in production mode btw) so the patch was kinda tricky. I should be able to restore it somehow.
Comment #6
sun@pounard: Any discussion pertaining to a "production mode concept" belongs into a separate issue, not this one. Thanks.
And yes, the current patch only focuses on errors, not exceptions. For exceptions, we'd have to introduce a DrupalException class, which overrides PHP's default ->getMessage() method to append a similar backtrace, leveraging the ->getTrace() provided by PHP.
Additionally, built-in + bootstrap-safe handling for t() messages and arguments would be highly beneficial to have.
Also, just now I recognize that the function in the actual error message does not contain argument values like the functions in the backtrace.
Comment #7
jbrown CreditAttribution: jbrown commentedThis is cool, but I would prefer a table.
dsm(debug_backtrace()); provides a lot more information.
Comment #8
sun@jbrown: A table for what? And why?
Also, to make one thing crystal clear:
The next time someone proposes a full backtrace output, this issue will immediately won't fix.
Comment #9
pounard@#6
FOUTAISES! (French word for "bullshits"). Creating a specific exception specialization for Drupal is certainly a good idea, for finer error handling, while doing it for display purposes doesn't make any sense (current display is good enough, and if you want a custom one, please just build using the raw stack trace you can get as an array).
EDIT: Let me explain a bit more: Each time you override code by creating a layer over an existing one only because the API doesn't fit you what you like, you create what we call code indirection. Code indirection often leads to spagetti code and pretty much fucks up any framework. OOP inheritance is a good thing, when you use it for business reasons, exception's business is to have a granular error handling mecanism, and changing exception display IS NOT exception's business, please, NEVER ever override exception only for this kind of display purpose, you will create code indirection that may impact performances even when not running in devel mode.
Further more, this patch is useless without talking before of a framework-wide error handling strategy. All it will provide in the long term is a lot code indirection (other use case of code indirection here, split up code, then attempt to glue back pieces, only provides headaches and useless glue code) and will make everybody's life harder when it will come to unify already existing code for implementing a common solution in the end.
EDIT: Pretty much rewrote the full paragraph, better english and typo.
Comment #10
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis is a duplicate of #1157850: Provide detailed backtrace in the log for Drupal fatal errors, please keep this discussion in one place, please.
Comment #11
pounardThanks Damien.
Comment #12
Damien Tournoud CreditAttribution: Damien Tournoud commentedActually, this patch seems superior. Marked the other issue as duplicate.
Comment #13
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis patch looks good to me. There is only one thing to do: extend it to exceptions by passing the
'backtrace'
key into_drupal_decode_exception()
; there is no reason not to output a simplified backtrace for exceptions too (and the current patch introduced a notice in the exception handling code path).Comment #14
pounard#13 Damien+1 It would be a good thing to do.
Comment #15
rfayWow, great stuff. Thanks for your work on this, @sun. I guess great minds think alike... or are annoyed by the same old thing over and over again. In commerce, we're seeing fatals show up in the issue queue and we have absolutely no way to get context on them.
I do like showing more information, and would appreciate you saying *why* that would make this a won't fix. Also would like to make sure we display information from exceptions, as an uncaught exception is just another kind of error.
Key differences in #1157850: Provide detailed backtrace in the log for Drupal fatal errors:
* There only fatals get a backtrace. Some sites exist with so many ongoing non-fatals that it may not be worth providing the backtrace for those. But I'm certainly open on this.
* There, exceptions get a backtrace and it seems to work just fine.
* There's a complete backtrace rather than a simplified one
* It only goes into the log. I don't really like taking an already scary fatal page and adding a stack trace to it and presenting that to the user.
Comment #16
Damien Tournoud CreditAttribution: Damien Tournoud commentedI'm fine with the current amount of information. Displaying more gets things tricky fast (do we want the full path of the files, or just the basename? something in between? do we want to display objects? how to handle that? etc.).
Comment #17
Damien Tournoud CreditAttribution: Damien Tournoud commentedAlso, I'm fine displaying the backtrace by default when the error is displayed. We might consider making the display of the backtrace optional, but I don't really see the point (error display is already optional).
Comment #18
moshe weitzman CreditAttribution: moshe weitzman commentedLooks good to me.
Comment #19
sunNo progress on exceptions yet, but
- removed removal of error source in the backtrace (so function arguments are visible for that, too)
- added safe attempt to not expose DRUPAL_ROOT
Comment #21
rfaySince we're working on this class of things, shameless plug for #1159422: Death to WSOD: Output information about what caused a fatal error
Comment #22
rfay#19: drupal.error-backtrace.19.patch queued for re-testing.
Comment #23
moshe weitzman CreditAttribution: moshe weitzman commentedI think we should restrict this to just www requests and not CLI.
Comment #25
rfayIf you're testing: I created an error-creating fatal module for testing.
Comment #28
naught101 CreditAttribution: naught101 commentedThis would be absolutely awesome to something like this in the simpletest module as well, especially because you can't get a debug manually when running simpletest (ie. using something like dpm(debug_backtrace())).
Dunno if that should be part of this bug, or a separate issue?
Comment #29
aspilicious CreditAttribution: aspilicious commentedI don't think anything is wrong with this patch, it is this line (testing line) that is breaking the testbot.
Powered by Dreditor.
Comment #30
sunLet's also move that algorithm generating a simplified/prettified backtrace output into a separate function. Would be useful for Devel and custom/other code. A full backtrace is often too heavy, especially for on-issue communication (just ran into this in another issue).
Comment #31
moonray CreditAttribution: moonray commentedIncluding line numbers might be helpful as well. Sometimes functions are called several times in a function, and you might not know which on is at fault.
Comment #32
rfayI agree; I copied this code to commerce_devel and was immediately forced to add file and line number. I used just basename($trace['file']) and $trace['line']
Comment #33
Anonymous (not verified) CreditAttribution: Anonymous commentedbig plus one from me! may even be able to retire some of the debug code i've been using for longer than i can remember.
also, i really like #30, because i often want to look at cleaned up backtraces while developing, and it has nothing to do with errors.
Comment #34
steinmb CreditAttribution: steinmb commented+1 from me too!
Running my dev. sandbox with patch #19
Comment #35
naught101 CreditAttribution: naught101 commentedWould it be possible to put those errors in a collapsed fieldset? it's not uncommon that if you get one, you get a lot, and the messages could end up being quite long.
Comment #36
chx CreditAttribution: chx commentedVery useful and well done -- but I am hesitant a little. Do we want this on screen or watchdog only? We could amend the watchdog hook so that the modules logging can provide a link if they want (dblog and mongodb will, syslog won't) . One problem here is that sometimes errors come by the dozens or even the hundreds. Not to mention that I do not want to consider all the possible security implications. Should you leave error display on? No. But, should we make that mistake so much worse?
Also, since know the file path can we link the core functions (if it does not start with profiles or sites it's core) to api.drupal.org for convinience?
Finally, the part where we ucfirst(gettype()) -- if it's not scalar, then it's a resource, an object or an array. For arrays we could display a count like
Array (20)
, for objects we could show the class likeObject (NegativeDentry)
and finally for resources:ob_start(); var_dump($x); ob_get_clean()
. Because this shows the type of resource and even the internal identifier which can be used to discern between two resources of the same type.Comment #37
chx CreditAttribution: chx commentedWe have agreed to scrap the watchdog idea but instead create a new ERROR_REPORTING_VERBOSE level.
Comment #38
slefevre1 CreditAttribution: slefevre1 commentedWhat would be handy is just to backtrace to the first (last?) non-core function in the call stack. It doesn't help me to know that there was a problem in
form_execute_handlers()
, but if it at least goes back to whatever last non-core function is in the call stack, that'd be a big help. Any PHP error that drupal reports to me is way too generic. You might as well get rid of them for what they're presently worth.Q: are most/all errors going to originate in non-core functions? In other words, would this work/be useful in most cases? I don't know.
Comment #39
jhedstromHere's the patch from 19 without the
$foo = $bar
line, so the tests should pass. I also added the ERROR_REPORTING_DISPLAY_VERBOSE constant, the change to the error form, and the logic of whether to display the backtrace or not.Comment #40
Owen Barton CreditAttribution: Owen Barton commentedSubscribe - I think this will at least give some people better clues about where to look for and/or report bugs.
@jhedstrom - #39 is missing the patch you mention
Comment #41
moonray CreditAttribution: moonray commentedOne thing to watch for is large strings...
drupal_render_page()
for instance can be passed a VERY large string value.The following will need to include some kind of trimming.
Comment #42
jhedstromOops. Here's the patch I forgot to actually upload in #39.
Comment #43
steinmb CreditAttribution: steinmb commentedAttached screenshot with running with #42
Comment #44
bryancasler CreditAttribution: bryancasler commentedsubscribe
Comment #45
naught101 CreditAttribution: naught101 commentedIs there really much point having a backtrace with no line numbers or filenames?
Comment #46
pingers CreditAttribution: pingers commentedTend to agree, line and file helps a lot. Problems don't always stem from the last call in the stack (obviously). Objects can have overridden methods, so determining which class contains the actual called code is problematic without filename. Line number is useful when a function is called more than once from the originating function.
Comment #47
mrfelton CreditAttribution: mrfelton commentedI too think we need filenames and line numbers for each function call in the stack.
Comment #48
pounardDo this patch also add a backtrace field to watchdog database table and hook signature documentation so that those trace will always be present when logged?
Comment #49
rfayWill need reroll after the /core patch went in.
Comment #50
wapnik CreditAttribution: wapnik commentedWhat about displaying more information, like for example symfony does too. Few lines of code around the error, request info, ... What you think about adding Symfony\Component\HttpKernel\Debug\ErrorHandler and Symfony\Component\HttpKernel\Debug\ExceptionHandler to Drupal?
Comment #51
pounardI like #50 suggestion, and because those two components are parts of the HttpKernel Synfony component, it's tied to what WSCCI is doing and will probably be included as a part of HttpKernel. I would definitely cleanup core error handling code.
Speaking of design, we could easily extend those two and raise events (using Synfony's EventDispatcher package) on errors so modules could act on those (we can keep a hook for registering module listeners though at first).
I definitely +1 wapnik suggestion.
EDIT: The event listener doesn't seems like a good pattern here, my opinion is that it should never exist more than one error listener; But, in order to reproduce the actual core hook_watchdog() behavior, we can do as I described upper easily.
Comment #52
Alan D. CreditAttribution: Alan D. commented(Off topic) Could we get permissions added while this is being refactored? The function / method signatures could be used to see what modules and versions are enabled, allowing easy discovery of possible security vulnerabilities. Maybe a new permission "view on-screen errors and warnings" or something?
Comment #53
artfulrobot CreditAttribution: artfulrobot commentedPatch from #42 did not apply cleanly on 7.14 for me. Here's one that did.
Comment #54
rfayComment #56
tim.plunkettRerolling #53 for D8.
Comment #57
tim.plunkettComment #58
chx CreditAttribution: chx commentedThe purpose of this patch is to provide a short backtrace to give some clue. Those needing more are welcome to install devel. I for myself like this.
Comment #59
amateescu CreditAttribution: amateescu commentedFixed in the attached patch. This is only a minor change so leaving at RTBC.
Comment #60
Alan D. CreditAttribution: Alan D. commentedTrying not to sound like a broken record, can we slip in a permission check here for the screen errors?
I've used this code on a custom D7 module that is happy spitting out backtraces all over the place (Fields and Panels are generate so many warnings), but our clients is blissfully unaware and we are resolving these bugs much faster because of the code replicated from here and the fact that we get an instant backtrace.
And then in all of the corresponding locations:
Comment #61
tstoecklerCan we do #60 in a follow-up / separate issue?!
Seems like that needs some more discussion and this has been lingering for so long already.
Comment #62
amateescu CreditAttribution: amateescu commentedAgreed with #61. I'm tired of installing commerce_devel just to get this important piece of DX.
Comment #63
Alan D. CreditAttribution: Alan D. commentedSounds good to me. :)
See #1608034: Add permissions for displaying errors
Comment #64
salvisI was on the brink of adding the commerce_devel code to devel, but it requires duplicating an awful lot of core code to get it right, because core is so uncooperative.
Let me look into this to see what it would take to provide a reasonable integration point.
Comment #65
chx CreditAttribution: chx commented. Hrm. What is missing? Please file an issue for any missing hooks, alters etc
Comment #66
salvisYes, I'll open a new issue. We'll need a new hook.
devel_commerce duplicates the entire 76-line _drupal_log_error() function just to customize two lines.
We could and maybe should do more here. I have found that if Drupal is symlinked into the webroot directory (for easy sharing among multiple sites), then DRUPAL_ROOT will contain the path in the webroot, but PHP delivers the resolved paths in the file system. This means there is no match and the file system path is exposed.
Comment #67
catchI'm not sure we should add the DRUPAL_ROOT check at all here, see http://groups.drupal.org/node/230373 for example.
If we add it, but it doesn't work all the time, we'll get more 'full path disclosure vulnerability!?!!!!" crap.
If we don't do it at all, then we might still get that but we can just say we don't care at all.
I can maybe see an argument for shortening the filenames just to reduce verbosity, but then is that the right place?
Comment #68
sunThe paths are shortened to reduce verbosity only. At no point in time I considered "security" to be a reason for the shortening.
To account for that, for now, I'd recommend to implant the verbosity reason into code comments, because - yes, the code does not explain why this is done, so anyone reading it might mistakenly believe that this is a (poor) security "feature".
I'd suggest to enhance both the constant definition as well as the inline comment above the code block that performs the stripping:
Done so.
I'd prefer a separate follow-up issue for #66 to improve the symlink case. We might be able to use
realpath(DRUPAL_ROOT)
, but as usual, I guess that needs a lot of dedicated testing on its own.Comment #69
salvisEDIT: Ignore this comment!
'backtrace' is not '!backtrace', and thus no replacement candidate value; 'severity_level' is not intended for t() either.
Once this patch has landed, I'd like to propose a new hook_log_error($error) that passes $error for any module that wants to display/log/whatever the message in an alternative way. It would be nice to pass the backtrace, too. This could be an additional parameter, but why not just leave it in $error and pass $error unchanged?
Comment #70
salvis(didn't mean to change the status)
Comment #71
salvisActually, the stripping is not related to the "backtrace information" -- it's about the path in the error message. Drop the second paragraph.
t() does not like having additional stuff in its $variables argument, especially non-scalars. So this is needed — ignore my comment in #69.
Fix the comment to not mislead.
In addition to the items above, I've made the following changes:
Fixed the docblock of _drupal_log_error() to document the new 'backtrace' key.
Moved the formatting of the backtrace to a separate _drupal_format_backtrace() function. This improves the code structure, and the function can be useful for contribs.
Added passing string arguments through filter_xss().
Comment #72
mrfelton CreditAttribution: mrfelton commentedNice. Here it is for those that want in their D7 make files.
Comment #73
chx CreditAttribution: chx commentedThanks for working on the Drupal 7 backport. However, it's premature -- we only backport once the patch is committed.
I must say I hate with a fiery passion when people post patches for drush make! It typically makes the queue harder to follow and flies straight against http://drupal.org/best-practices/do-not-hack-core .
Comment #74
chx CreditAttribution: chx commented#67 is addressed so back to green. Perhaps we could salvage #72 as well as if this goes in.
Comment #75
mrfelton CreditAttribution: mrfelton commentedSorry chx, but it's a very useful patch wether for Drupal 8 or Drupal 7. I tested #71 on D8, it did the job, it passed all the tests, so I backported it. This issue was tagged as "needs backport to Drupal 7" back in comment #56. I marked the patch clearly as a D7 patch, and one that the testbot should ignore. I don't know how you can possibly call that hacking core.
Comment #76
chx CreditAttribution: chx commentedI thought #73 was quite clear on what my problem was -- and sorry for lashing out at you. In this case it's not that bad -- but often we see these drush make patches in very long, convoluted issues and even on those that have zero chance of getting in. I expressed my hopes in fact that your patch can be used, after all.
Comment #77
sun_ denotes a private function though, which shouldn't be used by anything else. I agree with the intention and general usefulness, so here's a quick fix for that.
Comment #78
salvisAgreed, but no scope prefix, i.e. format_backtrace()? There are certainly other ways to format a backtrace.
I modeled the docblock after the one for _drupal_get_last_caller(), but I agree with sun's improvements in #71. Should we improve the _drupal_get_last_caller($backtrace) docblock accordingly (and add the type hint)? Can/should we remove that underline, too?
Comment #79
sunI only adjusted the phpDoc of the newly added function. Fixing phpDoc of existing functions is left for dedicated general code clean-up issues.
Comment #80
webchickThis seems like a nice useful feature, but it's a feature, not a task. I'll get back to this again when I'm through the rest of the major/critical tasks/bugs.
Comment #81
catchCommitted/pushed to 8.x, moving to 7.x for backport.
Comment #82
mikeytown2 CreditAttribution: mikeytown2 commentedPretty much a straight port from #77 to D7.
Comment #83
chx CreditAttribution: chx commentedI think this is good to go , I do not see any string changes only a single addition
t('All messages, with backtrace information')
Comment #84
steinmb CreditAttribution: steinmb commentedLooking good, RTBC.
Comment #85
Wim LeersOMG YES. This will make it a zillion times easier for contrib modules to debug things (because many non-technical users don't know the kind of information we need). Thanks to all of you who made this happen!
Comment #86
sunFollow-up: #1704422: Error level constants cannot be used in settings.php
Comment #87
Alan D. CreditAttribution: Alan D. commentedAnd just in case this is forgotten about: #1608034: Add permissions for displaying errors
Live sites have errors too and errors shouldn't show to non-admins / developers!
Comment #88
geek-merlinSo this has 8.x and 7.x RTBC patches.
Will this get attention as 7.x issue?
Comment #89
Alan D. CreditAttribution: Alan D. commented#82 appears to have the go ahead to commit into 7.x - it is already in 8.x
#86 is to allow developers to alter the settings in the settings.php file without resorting to meaningless numbers like 32767 or 30719 for E_ALL (PHP version dependent)
#87 is to allow site builders to run this on live sites without exposing the internals to the entire world
Comment #90
David_Rothstein CreditAttribution: David_Rothstein commentedSorry, the combination of string addition, UI change, and form structure change makes me not want to commit this right before the Drupal 7.15 release. Let's hold off and discuss again after that.
I think as long as we provide some advanced notice (and commit this towards the beginning of a release cycle) it's OK, though.
But..... why does this patch not have tests (for either Drupal 7 or Drupal 8)? There's a DrupalErrorHandlerTestCase in the codebase that attempts to test each of the 'error_level' options, but now it doesn't have any tests for the new option being added.
Comment #91
sunSorry to do this, but there is a flaw somewhere in the code, so it's probably not that bad that we didn't commit the backport yet.
For certain errors/exceptions, an additional error/exception is thrown. I wasn't able to identify yet what exact kind of errors/exceptions are triggering the additional one.
Manually dumping a debug_backtrace() yields this:
So it looks like some errors/exceptions do not have a backtrace, and/or _drupal_log_error() might be called from another place, which didn't pass the error/exception through _drupal_error_handler_real().
Comment #92
sunThis would be the obvious "quick fix", but I don't think that's the proper thing to do.
Comment #93
scitoCurrently, backtraces are not added to watchdog loggings. Is there a reason not add them to watchdog loggings?
Backtraces in watchdog entries would be useful on production sites, especially since UI logging is normally disabled on production sites.
Comment #94
rooby CreditAttribution: rooby commented@scito:
I'm guessing you haven't read any of this issue.
Comment #95
mikeytown2 CreditAttribution: mikeytown2 commentedGot testbot to throw "Undefined index: backtrace Notice".
http://qa.drupal.org/pifr/test/329463
Comment #96
mikeytown2 CreditAttribution: mikeytown2 commentedLooks like that went away, so here is a screenshot of it. More info on what failed... http://drupal.org/node/818818#comment-6417664
Comment #97
sunThanks @mikeytown2, that seems to be exactly the same nested exception I'm frequently encountering, too. I won't have time to look further into this, so I hope someone else can take over.
Comment #98
Cameron Tod CreditAttribution: Cameron Tod commentedI am currently having an error which triggers the "Undefined index: backtrace" notice, related to #1798732: Convert install_task, install_time and install_current_batch to use the state system.
I'm not sure if this of any use, but I thought I would at least make a note of it here - hopefully it helps to narrow things down.
$error is
Heres a gist with a backtrace. https://gist.github.com/3956062
Comment #99
sunIt is possible that #1536868: No trace when database is down fixed the PHP notice about the missing 'backtrace'. I did not verify this, only saw that commit in the log.
Comment #100
olli CreditAttribution: olli commentedHere is one missing line.
Comment #101
sunWell, that totally looks more like it. Thanks! :)
Comment #102
Wim LeersHeh, what a lovely patch :)
Comment #103
webchickGreat catch! :)
Committed and pushed to 8.x. Back to 7.x for porting as per the tags, though I'm not sure if David's into that idea or not.
Comment #104
BerdirGetting the following now when an exception is thrown, I think that is caused by this patch:
htmlspecialchars() expects parameter 1 to be string, array given
htmlspecialchars(Array, 3, 'UTF-8')
check_plain(Array)
drupal_placeholder(Array)
format_string('%type: !message in %function (line %line of %file). !backtrace', Array)
Drupal\simpletest\TestBase->exceptionHandler(Object)
Drupal\simpletest\TestBase->run()
_simpletest_batch_operation(Array, '208', Array)
call_user_func_array('_simpletest_batch_operation', Array)
_batch_process()
_batch_do()
_batch_page()
system_batch_page()
call_user_func_array('system_batch_page', Array)
Drupal\Core\EventSubscriber\LegacyControllerSubscriber->Drupal\Core\EventSubscriber\{closure}()
call_user_func_array(Object, Array)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1)
Drupal\Core\HttpKernel->handle(Object, 1, 1)
Symfony\Component\HttpKernel\Kernel->handle(Object)
Which is because of:
So the backtrace array is passed to format_string(), which tries to check_plain() it.
Comment #105
webchickHere we go again...
Comment #106
sunAdditionally, the entire code of errors.inc has been duplicated into ExceptionController...
None of the code that has been added by this issue exists in there.
I don't know in which situations the functions in errors.inc are triggered in HEAD — they're still registered early in the bootstrap, and I've seen verbose error messages including backtrace in HEAD up until very recently, but we completely changed the bootstrap to boot DrupalKernel instead (as of yesterday) and the Symfony Kernel is known to overload the environment.
I was under the assumption that we disabled that by overriding
Kernel::init()
in DrupalKernel, but in trying to debug an exception to debug some other functionality, I factually don't see any function in errors.inc getting invoked (excepterror_displayable()
, which is weird on its own...) — instead, the entire thing runs through Drupal\Core\ExceptionController::on500Html(), which still calls into various procedural functions, but I'm not able to make sense of it.Comment #107
sunThis essentially means that the entire patch in #77 needs to be redone. (plus the follow-up fix from #100)
Comment #108
catchComment #109
sunAttached patch re-implements #77 and #100 in ExceptionController.
I'm getting a formatted backtrace with this again.
Comment #111
olli CreditAttribution: olli commentedAttached a simple test.
Comment #112
das-peter CreditAttribution: das-peter commentedUpdated patch, fixes the mentioned issue in #104 from berdir.
Comment #114
chx CreditAttribution: chx commentedHere's some patch from the other end of the problem. It nukes the backtrace before format_stringing it from update or testbase. Not sure how helpful it is, but it's written and maybe it helps.
Comment #115
Berdir#112: 1158322-error_backtrace-112-with_test.patch queued for re-testing.
Comment #116
Berdirextact => exact.
Comment #118
Berdir#112: 1158322-error_backtrace-112-with_test.patch queued for re-testing.
Comment #119
Anonymous (not verified) CreditAttribution: Anonymous commented37ea2e09c90fd4ee6deb22040d0eae9bc06b5ef8 breaks callers of _drupal_decode_exception(). i just wasted a bunch of time trying to figure out a heisenbug over in #1331486: Move module_invoke_*() and friends to an Extensions class because of this.
please can we rollback?
Comment #120
Anonymous (not verified) CreditAttribution: Anonymous commentedif we're not going to rollback, can we commit something along the lines of #114 to stop the bleeding?
Comment #121
webchickCan you review it and mark it RTBC if it fixes your problem? :)
Comment #122
Anonymous (not verified) CreditAttribution: Anonymous commentedattaching #114 so the bot runs it. i think it's RTBC, looked at usage of _drupal_decode_exception() and this covers the cases that were broken by the 'backtrace' array in it's output.
i've added on doc fix to #114 - we had a reference to drupal_decode_exception instead of _drupal_decode_exception.
so if this comes back green, i'm going to RTBC - no commit credit, the patch is chx's work.
Comment #123
Anonymous (not verified) CreditAttribution: Anonymous commentedyay.
Comment #124
webchickHave we finally seen the end of this perma-major? Tune in tomorrow! ;)
Committed and pushed to 8.x.
Oh-so-tentatively marking down to 7.x for backport, but I recommend waiting awhile to see how the dust settles first.
Comment #125
BerdirI think not.
I'm not sure why #114 was committed and not #112, which I think contained a fix for the problem as well + other improvements and test coverage.
What #112 did not contain is the update.inc change, so let's not roll back (I'm glad at least the ugly exception in case of test exceptions was fixed) but #112 will need a re-roll now.
Comment #126
das-peter CreditAttribution: das-peter commentedI hope this is re-rolled properly.
Comment #127
mrfelton CreditAttribution: mrfelton commentedReally need something that works for Drupal 7 too. Attached is a backport of the patch in #126.
Comment #128
BerdirAlso, can we update watchdog_exception() to include the backtrace as well while we're at it? Because that's the one where it would be *really* useful.
Comment #129
Reg CreditAttribution: Reg commentedThis patch is against drupal 7.18. Since this is an issue for Drupal 8 this will probably fail testing.
Also, I added a couple of features, namely:
In any case, the prime goal here is to add flexibility. If anyone else has other outputs they find useful it would be very easy to copy the methodology I used to add another format. I mention this because I noticed earlier in the issue much debate over what should be displayed. There's never going to be one best display of information so some choices should help.
Comment #131
salvisThank you, but we need to get the D8 version committed first.
Comment #132
Reg CreditAttribution: Reg commentedWell, it's there for anyone using D7 that wants it.
I wouldn't mind doing it for D8 but my time is limited and I checked the error that was generated "Detect a non-applicable patch". I really don't know what that means or how I would go about finding exactly what it's complaining about. I'm a little shy of jumping in trying pass all the tests for a version I've never used when I can't really figure out the line(s) the tester is complaining about.
Perhaps you could give me some insight on where the detail of what created that message is? I looked around and couldn't find anything to give any extra detail.
Comment #133
David_Rothstein CreditAttribution: David_Rothstein commented@Reg, I think you already answered your own question above :)
So yeah, that's all that's happening really; the testbot is trying to apply the patch to Drupal 8 and it doesn't apply there since it was written for Drupal 7.
Most likely there are several reasons why the patch won't apply, but the main one is that in Drupal 8 all the modules and such have been moved into the top-level
core/
directory...Comment #134
Reg CreditAttribution: Reg commentedOk, I appreciate that however what I still want to know is how do I "see" that in the test results.
This is one thing: "Detect a non-applicable patch". What I would expect to see is: "File referenced in patch .../xyz not found." ...anything that's not a cryptic response.
What's concerning me is if I tackle creating a patch for a version I don't yet know that passes testing am I going to be spending all my time trying to decode what the tester is saying because that's only one error but I when I looked at the results saw dozens of checks, perhaps close to a 100... that's a lot to go wrong and spend time scratching your head over what it's telling you.
Comment #135
mikeytown2 CreditAttribution: mikeytown2 commentedGoing to retest #129 under D7
Comment #136
mikeytown2 CreditAttribution: mikeytown2 commented#129: 1158322-error_backtrace-129-D7.patch queued for re-testing.
Once this test is done, we can switch the issue back to 8.x
Comment #137
Reg CreditAttribution: Reg commentedVery cool and thank you so much. If I wasn't so tapped out for time I would tackle the D8 version but I just can't right now so I really appreciate this!
I've been using this over the last couple of days and made a couple of tweaks (changed a label and removed a couple of "notices" that came up under certain circumstances). I've attached the update here.
One thing that I know should be changed is there is a very tiny bit of styling added for a table that is generated in the PHP. I'm sure someone who knows the drupal system CSS well could remove that (it's only a couple of lines) and just hard code in some existing CSS classes. I added a TODO in there to that effect.
Comment #138
Reg CreditAttribution: Reg commentedThis has one change only. Occasionally a PDOException will get through and if you have it set to send to watchdog the PDO will reject the exception since you can't send errors from using the PDO back though the PDO. This is a restriction by PHP, and it makes perfect sense so no complaints here.
This addition checks for such an error and if you have it set to go to watchdog it simply won't thus preventing a cascade of errors. The danger here is that the error could get "thrown away" if you don't have it set to display errors as well.
I'm in two minds about this, it would easy enough to force the error to the display if we don't allow to go to watchdog however there are user settings to control that and I don't necessarily want to ignore the user settings.
I could make an argument to force to show the error or equally well to leave it up to the user. If anyone wants to chime in, please do.
Comment #139
sunFriends, this issue is getting completely out of hands again. This is still not working correctly in HEAD, since we magically skipped over an essential follow-up patch. I'm removing the backport tags and everything else. This needs to be fixed in HEAD first.
The D8 patch that is still required is #109/#126.
Also noteworthy: The committed follow-up patch from #122 added manual
unset($variables['backtrace'])
calls to various places, but that is not sufficient;hook_watchdog()
implementations still get a backtrace that may contain data that cannot be serialized: #1934738: Exception: Serialization of 'Closure' is not allowed in serialize() (line 154 of dblog.module)Comment #140
Reg CreditAttribution: Reg commented@sun:#139 The error you are referring to I just came across and fixed. I believe it is the very last bug in the code (although you never know for sure).
Anyway, the code is here: http://drupal.org/node/1937832.
If you look for the function "array_has_PDOException" and the two calls to it and incorporate it into your D8 code you will have removed that error. Sorry I couldn't do this directly on D8 yet but at least there is a fix you can adopt easily enough for whomever is working in this problem.
Comment #141
Reg CreditAttribution: Reg commentedI don't have a running copy of D8 however the code is so similar to D7 (for the parts that relate to this) it wasn't hard to manually splice the D7 code into the D8 code.
Of course there is more than a decent chance I made a mistake along the way so I would very much appreciate anyone who's actively running D8 to test this and let me know of anything that comes up. I'll fix it quickly so we can finally get better error reporting officially integrated into Drupal.
This is meant to be identical to the latest patch for D7 I did that's seems to be working perfectly (at http://drupal.org/node/1937832#comment-7154472 so you can use that as well for a reference if you want to do a little debugging yourself).
One thing I avoided is adding 2 tests. I think you really want a running copy for that however I did add a TODO where they should go.
Comment #142
BerdirThanks for working on this, here is a quick review. Don't try to do too much in here.
Comment shouldn't be longer than 80 characters.
Comments shouldn't be appended to code but always be on their own line. Also, why is this not using a stringe like the other constants?
As of PHP 5.3.6, which we now require, the arguments to that function have changed, see http://php.net/debug_backtrace
Needs spaces around the =. And it shouldn't use variable_get() but config()
That should a) be handled by dblog.module, for which there is a specific issue and b) we actually should never pass a backtrace array through to watchdog, only the generated string. Then this wouldn't be a problem.
I don't think this should be changed. As you can see in the line above, this function currently doesn't assume that it returns HTML and is returned in a HTML response.
Comment #143
Reg CreditAttribution: Reg commentedOn all but the last two points: First, it's less than I thought it would be given a manual splicing of code and having never run D8 so I'm not feeling too bad about it but it is going to be difficult to get it right since none of my work involves working on D8. Is there any chance you could tweak those few and give it whirl? That would be big help!
On the last two points:
a) Agreed, I hadn't got around to moving it but it was in the plan.
b) Agreed, and I already thought this one through a bit and the reasoning goes a little beyond what "should be". The reality is that you just don't know what people are going to do with contributed modules (or for that matter even a core bug could cause the same problem) and this PDOException getting through to watchdog is such a showstopper that's it's worth throwing in extra protection like this. You will also see a similar check right in watchdog for the same reason (they both need to be in place to work). If you don't then what can happen is you "lose" the error that caused the problem and the only problem you see is that "cannot pass PDOExecption to..." (I've experienced this first hand it's not fun to deal with) which then defeats what we are trying to achieve which is easier debugging. So, in this case, being overly careful and protective is not a bad thing at all so from already having experienced the problems that can otherwise happen, it's a good thing to keep it as is.
It's changed for a very good reason. You should test it before setting yourself on this conclusion. It is complimentary to:
return '...STACKTRACE:..'
I've been using these two quite a bit in D7 and they report well together the way I have done them. It also sets up a precedence for if/when someone may add other reporting styles, i.e., we've given each type a header to distinguish them.
Comment #144
Crell CreditAttribution: Crell commentedComing here from #1872690: Exception: Serialization of 'Closure' is not allowed in serialize.
Why exactly do we need to serialize a backtrace that in many cases is not serializable, because this is PHP and not everything is serializable?
And if we need to log exceptions, have a look at Symfony's FlattenException that is intended to address the same issue.
Comment #145
steinmb CreditAttribution: steinmb commentedhttp://api.symfony.com/2.0/Symfony/Component/HttpKernel/Exception/Flatte...
Comment #146
Reg CreditAttribution: Reg commentedI'm not sure where backtrace is getting serialized. It's certainly not in any of the code that I created to have the backtrace so I could only assume it's built into a function that gets called along the way I've not fully looked at, if at all. The full backtrace isn't used anyway, for the purpose of readability it's stripped down and summarized in two different forms, each of which you can choose to use or not. (patch at #141 above).
Regarding using symphony, right now we simply pass the error to the screen and don't let it go to the DB (most recent code #141). Given that the error was in the database that seems like a very sensible course of action to me so I'm not sure if there's any benefit in doing extra work just to see if you can send it back into the DB.
Comment #147
Reg CreditAttribution: Reg commentedFinally had some time to do this some justice.
This is a fairly significant rework so I'll probably have to iterate a couple of times but I have D8 running now which of course makes all the difference.
This will probably speed up development so it would be nice to get it into D8 ASAP.
Comment #148
Reg CreditAttribution: Reg commentedIt says status ignore on the patch. How do you make it run or not run tests on a patch?
Comment #149
tim.plunkettComment #150
Reg CreditAttribution: Reg commentedIs that what causes a patch to be tested, setting the status to needs review? - Never mind, found: https://drupal.org/node/332678
Comment #152
Reg CreditAttribution: Reg commentedThe only errors that patch produced was from the simpletest. I had a look and it was something that was there and I hadn't really worked on. Looking through it I only got so far and then came to an impasse and could use some advice from people who work in that area more.
Here's what I have found and where I am up to. I've reworked the test so that all combinations of conditions can be tested. However, I do not know how to effect an error in the context of a webtest that will inject the results of that triggered error into the HTML that's tested when you use the "assert" functions.
What won't work is you can't use set_error_handler() and then trigger_error() as the results won't go into the HTML that we want it to. I tried adding a hook so that I could create an error in the hook (using page_alter) so that I could create an error condition there but couldn't get the hook to take. I don't know if that's because the test modules won't get picked up by hooks or if I need to do something to make D8 realize the hook is there.
Another tact I was looking at was that CronRunTest:testCronExceptions() seems to create an exception so I figured there must be a way to trigger an error to do what I want but I don't really know enough to figure it out. Also I've never seen this syntax (from CronRunTest):
Does anyone know what that prepended slash does?
For now I just bypass the tests with a return at the top of the function until someone can give me enough info to know how to go about finishing it off.
Thanks.
Comment #153
Crell CreditAttribution: Crell commentedCopying from the related #2063303: A Drupal 8 AjaxResponse that must return a 403, returns a 403 and prints "A fatal error occurred: "
I know Symfony and Silex have very good debug output in case of uncaught exceptions. Symfony also has a nice debug toolbar. We should perhaps look into both of them and see if there's code we can borrow, or at least concepts.
Comment #154
mtift@Reg: the Drupal class exists to support legacy code that cannot be dependency injected. When you use it in a class you need to use the fully-qualified namespace, which is
\Drupal
(rather thanDrupal
). When it is used in procedural code (such as a hook), it can just be called likeDrupal::state
.Comment #155
Reg CreditAttribution: Reg commentedI'm working on a solution. In the meantime does anyone know why this function:
has this passed $context variable even though it's not used in the function?
Comment #156
Crell CreditAttribution: Crell commentedI'm not sure, but frankly all exception handling in errors.inc needs to be removed in favor of the existing handling in the kernel, which will catch it anyway.
Comment #157
Reg CreditAttribution: Reg commentedDrupal is not an OS, what do you do call a kernel in drupal?
Comment #158
Reg CreditAttribution: Reg commentedThis has testing of all the admin options and select error types however it's just looking for errors to be thrown so unit testing rather than functional testing.
Error trapping is a bit of a special case and I don't know if it makes sense to do functional testing with error trapping. However, if someone knows a trick to make the error trapping work with functional tests, terrific. I would like to see how it's done.
Also, could someone have a look at the error output and admin. options. I would like to know what people think of it over the previous way.
Comment #159
Crell CreditAttribution: Crell commentedThe HttpKernel class, provided by Symfony in the HttpKernel component. I strongly recommend reading through that and understanding it before working on error handling, because what you're doing here appears to be Drupal 7-era architecture. You're adding a bunch of new functions and global constants to support error handling code that is *almost* never called, and when it is it probably shouldn't be. There's an entirely different error handling model provided by HttpKernel that we should be moving more toward, not continuing to develop the legacy, ugly one that still assumes PHP 4 is a viable language. :-)
Comment #160
Reg CreditAttribution: Reg commentedGood point.
I looked at the error handling as is and right now this is the error trapping in Drupal so when an error happens this will *always* be called so this at the moment is anything but code that is almost never called. That is, no other error trap is set (unless running tests).
Symphony does have a set_error_handler() statement but at least through normal page loads without add-in modules I don't see it ever being run.
I'm happy to change this code over to Symphony integration, if in fact after study that is the way to go, however like almost everyone else I'm a volunteer so time is needed. The best steps forward I suggest are for now to use what I have written as it makes troubleshooting far better than anything previously so it should speed up development.
I'll dig into Symphony when I can next block out a chunk of time to study it and refactor this code accordingly.
Comment #161
Reg CreditAttribution: Reg commentedA small improvement.
Comment #163
das-peter CreditAttribution: das-peter commentedWell, that's impressive, a patch without changes has failed tests :D
Comment #164
rooby CreditAttribution: rooby commentedProbably has something to do with it being 0 bytes :)
Comment #165
Reg CreditAttribution: Reg commentedLOL, that is bizarre. I was about to make some excuses about working until daylight and such but it's a full patch on my system so I have no idea what happened. Maybe a slight update delay on my LAN with my NFS share ...who knows.
Comment #166
Reg CreditAttribution: Reg commentedComment #168
Reg CreditAttribution: Reg commentedThere's been an update to the git repo. which is why the last patch failed. I updated my local repo to build a new patch but even though I have the patch I can't test it before uploading because this update is throwing an exception (nothing to do with my code as far as I can see).
What's the coarse of action here? Just wait a while until whoever is working on that area fixes it?
The interesting thing here is it is a Symfony error so that's an opportunity for me to look at that part of the error handling although sooner than I really have time for.
Comment #169
das-peter CreditAttribution: das-peter commentedDid you re-install Drupal to test it? This sounds like an artefact in the configuration files. Ensure the config files are deleted before you reinstall the system.
I use this script to automate that whole thing:
rm sites/default/files/* -Rf && drush si standard --sites-subdir=default --account-pass=admin -y && drush dis overlay -y && drush en devel simpletest devel_generate -y && drush uli -l http://d8.localhost
Comment #170
Reg CreditAttribution: Reg commentedInteresting thanks! I will definitely do that later however I'm actually going to leave it as is for now because I don't want the Symfony error to go away.
The reason being I read a little on the error handling in Symfony and they say that the error handling there is limited and that the best error handling can be done by using a custom error handler. I figure this is an opportunity to figure out how to take it over through their configuration files and use ours so I'm looking at this is an "opportunity in disguise".
Of course part of that might mean moving the code over to classes as Symfony is very (if not entirely, I haven't looked that far yet) class oriented. If that's the case I'll probably do the transition in two steps, first take it over and then later do class conversion as my time is way over on what I allocated already but it would be very nice to get at least an initial Symfony integration going with the error handling.
Comment #171
sunI'm not sure what happened here. The latest patches contain completely different changes. The original change was still not completed for D8 (see #109 ... #139)
In order to get this back under control, I've created #2188523: ExceptionController does not respect configured system.logging:error_level (verbose backtrace) for the necessary follow-up fixes in D8 now.
We could consider to backport the original change here as a minor feature addition to D7, but doing so will require a very careful and cautious backport and review, since multiple patches were committed (to fix regressions of the original patch). For now, I'm tentatively marking this fixed.
Comment #172
mikeytown2 CreditAttribution: mikeytown2 commentedI'm still using #138 in D7 so getting this backported to D7 would be ideal.
Comment #174
mikeytown2 CreditAttribution: mikeytown2 commentedReopening this for 7.x. Close it if this is the wrong place and I will create a new issue.
Comment #175
AnybodyIt's a good idea to backport this to Drupal 7 because especially Exceptions are extremely hard to backtrace in Drupal 7. An error page appears without any deeper information (just the Exception message) and also the Watchdog entries show no backtrace!
Comment #176
geek-merlin+1 for this.
We might look into commerce_devel that already does exactly that.
Comment #177
david_garcia CreditAttribution: david_garcia commentedI just installed D8 and to my surprise the "revamped" error logging with detail is just useless. I am using patch from #138 in my sites for D7 and the output is much more useful.
I mean.... this is useful stuff (D7, patch from #138, includes line numbers, files, etc...):
And this is useless stuff (D8):
Am I missing something?
1) Why is stacktrace and backtrace not logged into watchdog?
2) Where is the stacktrace? (I can backtrace wich is not the same thing).
Maybe there is something not working in my D8 installation, or is that the expected output of the new error logging system.
I can tell you, the output and functionality provided by patch #138 is a huge timesaver, 90% of small bugs can be solved straight, even without replication, by just taking a look at a complete and detailed error dump.
Comment #178
Reg CreditAttribution: Reg commentedFor D7 I have a backport for that however to keep it separate from D8 I created a separate issue here https://www.drupal.org/node/1937832 .
It's a little dated and I can't remember if I fixed the seemingly random SQL error at the time of the latest patch there. However, I have it fixed and I'll upload a new patch to issue 1937832 within a couple of days for anyone who wants a D7 backtrace error patch that works on the latest 7.x Drupal.
For D8 I got stuck for time because Symphony is a whole thing in itself and I haven't been able to find the time to dedicate to learning it and then tapping into it. However, if someone wants to post a code snippet that shows how to take over it's error handling properly within the Drupal context then I'll be happy to take it from there and create a patch for D8 as well as I have everything except the Symphony part done.
Comment #179
mikeytown2 CreditAttribution: mikeytown2 commentedPulling in latest D7 patch from #1937832-1: Add backtrace to all errors for D7. It's 2 days newer than #138.
Fixed a php notice I got with that patch as well (line 222 inside of errors.inc)
Changed this
if ($st_opts[ERROR_REPORTING_DISPLAY_LOGS] || $bt_opts[ERROR_REPORTING_DISPLAY_LOGS]) {
to this
Comment #180
david_garcia CreditAttribution: david_garcia commentedA comment from: #1937832: Add backtrace to all errors for D7
@reg said: PHP won't let you write PDO exceptions to the DB, and rightly so.
I have been using that patch (modified to allow PDO dumping on database) on all our production sites and writting PDO exceptions to database does work (why sould it not?). I mean, I just cannot understand what "PHP won't let you write PDO exceptions to the DB". Exceptions are not written to the database themselves....
This needs some clarification....
Comment #181
mikeytown2 CreditAttribution: mikeytown2 commentedI think the idea is if PDO is throwing exceptions due to the database being gone then trying to write more data to it is not a good idea.
Comment #183
david_garcia CreditAttribution: david_garcia commentedPDO exception does not necesarily mean that database is gone.
But it is worth considering that situation, and prevent infinite loops.
I cannot think of a consistent way of analyzing an exception and determining if the database is down based on it, but asuming that PDO Exception means database is down is also a mistake.
Comment #184
mikeytown2 CreditAttribution: mikeytown2 commentedPinging the database would be a good way to detect. I know there is mysqli_ping(), trying to find an equivalent for PDO like this http://terenceyim.wordpress.com/2009/01/09/adding-ping-function-to-pdo/
Comment #185
david_garcia CreditAttribution: david_garcia commentedWhat about something like this:
This prevents database down infinite loop, without adding overhead. If the same PDO Code is thrown twice, we can assume it is not worth counting on database logging.
Just an idea....
Comment #186
mikeytown2 CreditAttribution: mikeytown2 commentedI modified the array_has_PDO_connection_exception() function by renaming it and seeing if the PDOException message as the text gone away in it.
Comment #188
mikeytown2 CreditAttribution: mikeytown2 commentedFixed the function name in dblog.module
Comment #189
david_garcia CreditAttribution: david_garcia commentedI think it is not reliable to count on a "gone away" string for database down detection, this can be different on different database engines. Also, database down can manifest itself as different messages on different situations (ex. when database is shutting down it will send different message compared to when it is not available, but in both cases it is not available).
I'd rather control exception code repetition, so if when logging to database we get the same PDO error that we were going to log, that means a more than potential inifinite loop.
Comment #191
mikeytown2 CreditAttribution: mikeytown2 commentedTrying to get the current way working; once this passes tests I'll look into a better way of preventing errors that cause errors due to database issues.
Comment #192
mikeytown2 CreditAttribution: mikeytown2 commentedFixed a couple of notices I was getting
Comment #193
Crell CreditAttribution: Crell commentedNeeds spaces around the =, here and in a few other places. Also, $st_opts is a meaningless variable name.
The watchdog() function is deprecated, I believe. Don't keep usages of it around.
But that aside, I will repeat my statement from over a year ago: 98% of all exceptions will get caught by our kernel exception listeners, NOT by the legacy procedural code floating about in errors.inc and friends. Those files need to get refactored out and removed, not added to. If you want to add optional backtrace output, do that in the new exception handling framework in an OOP fashion. Trying to pile more functions into the nearly-unused legacy system is going in the entirely wrong direction.
Comment #194
mikeytown2 CreditAttribution: mikeytown2 commentedThanks for pointing out the coder fixes :)
In D7 the watchdog() function seems to be alive and well according to the docs. The comment from #156 is in the context of D8 from my understanding; #139 up to #174 is talking about D8. I could be mistaken but I do not believe D7 has kernel exception listeners. If this patch doesn't belong in this issue I will re-open this issue then #1937832: Add backtrace to all errors for D7
Comment #195
mikeytown2 CreditAttribution: mikeytown2 commentedComment #196
Crell CreditAttribution: Crell commented... I completely missed that this issue had been refiled for D7. My brain was in D8 space. Sorry.
Given the length I would prefer to see a new issue for D7 to avoid such confusion.
Comment #197
Anybody#194 works great for me. Can we get some more responses? :)
It would be great and very very helpful to see this in core soon, I think.
Thanks a lot!
Comment #198
cesarmiquel CreditAttribution: cesarmiquel commentedGreat work guys! I tested #194 on an exception I have but the formatting code seems to have issues. This is how it looks:
I added a print_r() to the format_stacktrace() function in includes/errors.inc and noticed that the $backtrace array seems to not have all keys. The problem seems to be this code:
which doesn't add a blank <td></td> when the key is not set. I suggest replacing with:
With this code the following stacktrace is displayed:
Unfortunately I can't provide a patch because I don't have the latest code. Hope this helps though.
Comment #199
mikeytown2 CreditAttribution: mikeytown2 commentedNew patch based off of #194 with the fix from #198 included. Also did some UTF-8 encoding tricks with htmlentities()
Comment #200
cgmonroe CreditAttribution: cgmonroe commentedI LOVE this series of patches.. Unfortunately, I keep getting a PhP out of memory with both #199 and #194 with a nasty EntityMalformedException problem, even after increasing the limit up to 2000M (from 512M).
I didn't have time to try all the patches, so I stopped after these two and applied #138 which I never seem to have problems with. This generated the stacktraces I needed.
Sorry, this still needs some work.
Comment #201
david_garcia CreditAttribution: david_garcia commentedJust for the record, this needs even more work. I'm getting this sometimes.
This basically means that dblog has been called before errors.inc has been included, we should review the bootstraping process and see under which conditions can this happen.
The message that is trying to get looged is by the system_test module, with the message 'hook_boot'.
Comment #202
Kebz CreditAttribution: Kebz commentedWhere does "backtrace" originate? Is it in core or a specific module turns it on?
I'm just curious because when notices, errors, etc. pops up on my screen, it usually just shows the error and nothing more. For the last few days, it is now showing up with "Backtrace" right below the notices and with more info that I'm use to seeing.
For example (image attached as well)
Also, just wondering if this link https://api.drupal.org/api/devel/devel.module/function/ddebug_backtrace/7 is the same as the topic of discussion.
Comment #203
Kebz CreditAttribution: Kebz commentedComment #204
Kebz CreditAttribution: Kebz commentedUpdate: I just found my answer... lol
I downloaded the module "Commerce Devel" and didn't realize it is the source of the "Backtrace" feature that I was seeing on notices, warnings, errors. And due to the information that "Backtrace" provides, it helped resolved a problem (https://www.drupal.org/node/2142133)
Backtrace should definitely be part of core ... a useful tool for developers at any level.
Commerce Devel https://www.drupal.org/project/commerce_devel
A great introduction can be found here https://fr.commerceguys.com/node/69
Comment #205
mikeytown2 CreditAttribution: mikeytown2 commented@cgmonroe
In terms of memory usage I think if I where to use some of the new parameters added to debug_backtrace() in 5.3.6 & 5.4.0 that should help in those situation. Outside of this there is not much that can be done for older versions of PHP; debug_backtrace() can eat up a lot of ram.
Comment #206
david_garcia CreditAttribution: david_garcia commented@mikeytown2 It is too weird that he's getting out-of-memory even with 2000MB, maybe he's stuck in an infinite loop when handling the error (quite possible with the current implementation).
Comment #207
izaaksom CreditAttribution: izaaksom commentedJust a comment. When using AJAX and logging shown on pages; I got some warnings printed in the request body, and because of that some modals windows broke. So I had to turn off error logging on pages to access that configuration forms.
¿Just wondering if something it's implemented in topic?
Keep up with the good work!
Comment #208
rooby CreditAttribution: rooby commentedIn relation to #205 I think that returning just function calls would be good enough in a lot of cases and reduces risk relating to system configuration of memory limits and such.
Maybe it should be disabled for old versions of PHP or else there could be a setting page where it could be enabled/disabled with a warning about enabling it for old PHP versions.
Such a settings page could also potentially contain an option for newer PHP versions to switch between function calls and full data.
Alternatively those settings could just be variables you set in the settings.php file.
That's probably a better solution that a UI in this case.
Comment #209
Reg CreditAttribution: Reg as a volunteer commented@Crell #196: Agreed that for D7 there should be a separate issue and in fact I started on at #178 but people kept on-going with this thread. At this point, unless there is someone who can control how this thread behaves it might be more expedient to start a similar thread for D8 and leave this one for D7 since there is so much history here around D7.
Having said that, I noticed there have been a few patches against my original with some updates. Unfortunately my original at the time was a compromise between a previous implementation and something I used myself over the years. Hence the separate "backtrace" and "stacktrace" options.
Since I am revisiting the code I decided to just finish it up with the my own implementation but incorporating the features from the previous code I never bothered with. I think everyone will find this much nicer all around since it's not a "half way" measure anymore.
The basic new features are for both scalar and non-scalar variables which are handled separately which are:
If either scalar or non-scalar variables are selected then an additional column is added for the variable output.
The issue with "td" not being created for the HTML table at certain times has been fixed.
This patch is against D7.35 which is a little old and if it causes issues with passing tests then I'll create a clean install of the latest D7 and redo the patch.
Comment #210
Reg CreditAttribution: Reg as a volunteer commentedComment #212
cesarmiquel CreditAttribution: cesarmiquel commentedI'm at Drupalcon Barcelona with time on my hands to help and wanting to see this committed to core. Ping me so I can help with this issue.
Comment #213
Reg CreditAttribution: Reg as a volunteer commentedTrying again with a minor bug fix.
Comment #215
Reg CreditAttribution: Reg as a volunteer commentedSorry if I end up trying a few times before I get it right. The tests are only approximate on my local machine because I run PHP 5.6 and the drupal.org testbot runs in PHP 5.4 so I'm figuring out as much as I can locally but there's still some trial and error here that I can't get around.
On this one I simply moved a function that sometimes was being "undefined". For anyone who likes a mystery just by using "function_exists" just above where the problem was happening would cause the function to exist and the error would be avoided. E.g., this would cause a function undefined error (the function "array_has_PDOException" was the culprit not being defined):
...but this would cause the function to be found and work so neither an error would happen nor would I get a backtrace!:
Comment #216
Reg CreditAttribution: Reg as a volunteer commentedComment #218
Reg CreditAttribution: Reg as a volunteer commentedFrom about 1200 problems to 6 last time so getting close.
Comment #220
Reg CreditAttribution: Reg as a volunteer commentedThis should finally do it.
Comment #221
Reg CreditAttribution: Reg as a volunteer commentedSame as before with debugging code removed.
Comment #222
Reg CreditAttribution: Reg as a volunteer commentedThe interdiff.txt to match previous patch - ignore the one with it.
Comment #223
Reg CreditAttribution: Reg as a volunteer commentedThis is a discovered issue that resulted from these enhancements :
https://www.drupal.org/node/2606742
Comment #224
Reg CreditAttribution: Reg as a volunteer commentedIt would be nice to get this in the core but with D8 about to be launched I'm wondering if it's possible at this stage to get anything new in the D7 core. It's certainly long over due and sorely needed.
Comment #225
vrwired CreditAttribution: vrwired as a volunteer commentedI tested this on a vanilla copy. Tested it out fine there using intentional errors.
Then patched another site which already had a warning I'd been meaning to debug today - Undefined index: page_manager in workbench_moderation_form_alter().
On both sites, I was able to get backtrace info on screen and in logs.
This will be a very useful patch to d7 core indeed.
Comment #226
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThis is a bit of a confusing issue since it had a lot of commits for Drupal 8 already, but still not clear if it's fully fixed for Drupal 8 yet...
I think at this point it makes sense to leave this issue on Drupal 7 for the backport - any remaining problems with Drupal 8 can be filed as separate bug reports. However, we do have to make sure that what we're doing here for Drupal 7 is actually a backport of what already went into Drupal 8 (from a functional perspective, that is - even if the code is different). If there are new features or capabilities being proposed for Drupal 7, they need to go into Drupal 8 first.
I am not sure (since I'm not too familiar with this issue), but it looks like Drupal 8 only had one constant that was added here (ERROR_REPORTING_DISPLAY_VERBOSE, with a corresponding option in the UI) but the latest Drupal 7 patch in #221 has way more constants and way more options. Why are all those extra options being added (and why only for Drupal 7 and not Drupal 8)? Do we really need anything more than the single option that was added to Drupal 8?
(OK, looking a little closer I do kind of see the point of having it be a separate option in the UI, e.g. a checkbox that enables backtraces regardless of whether error messages are displayed on the screen or not - rather than the extra radio button that Drupal 8 has which only ever allows displaying the backtrace on the screen. Because there'd definitely be value in showing the backtrace in the logs for production sites that don't want error messages displayed to the screen at all. But (a) that kind of change would need to go into Drupal 8 first, and (b) it still seems like it would be a lot simpler and require a lot fewer options than are in the latest Drupal 7 patch here.)
In any case, thanks for working on this. It looks like a very useful feature!
Comment #227
Reg CreditAttribution: Reg as a volunteer commented@David_Rothstein:
I'll work on getting these features in D8 so it can be backported and try to make that my weekend project this week. Is there a different issue for D8 with the changes you mentioned or is it all here on this issue?
Part of the reason for the extra features which I agree might seem like a overkill compared to what it was is potential "out of memory" errors and and giving a way to just reduce what you see to what you need so you don't suffer from information overload. You will see a note next to each option on the page that's there to protect you from out of memory errors.
The additional options that are not to help with out of memory are there to fine tune what you see. You need to be able to tweak detail so you get that balance of detail you need without going overboard and there really isn't a "one size fits all" solution so you need to give people the ability to tweak for their particular troubleshooting needs at the time.
Could I entice you to apply this patch on a project of yours and play with it a little? I think you will really like what you see and I would certainly appreciate the feedback (I've been tweaking the parameters over years for myself so finally being able to sharing them is really nice to be able to do).
Comment #228
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedIt looks like #77, #100, and #122 above were the patches committed to Drupal 8 (not sure if I missed any). There may also have been changes in other issues of course. It's probably best to use the above commits as a guide and then just look in the current Drupal 8 codebase and see exactly what the corresponding code looks like now.
I did a brief test of the Drupal 7 patch now. It looked pretty neat. But doesn't the Drupal 8 patch handle those issues without any configuration, just by trying to make some smart choices for exactly what amount of information to show in the backtrace? (See Error::formatBacktrace() for the current Drupal 8 formatting code.)
Comment #229
Reg CreditAttribution: Reg as a volunteer commentedThanks for checking it out and so quickly!
I honestly don't know as I have only checked D8 out now and then and never had a chance to delve into it but I guess I'll be doing that this weekend (or sooner if I can) so I'll get back to as soon I get to delve into the details.
Comment #230
pounardThere's something bothering me with the actual patch, maybe it's a detail:
Are you really storing exceptions into the database ? It's not supposed to get in there, exceptions are tight bound to the current run stack, only reporting should be stored ?
Anyway, seeing this code (in latest D7 patch) :
watchdog('php', $message_line, $error, $error['severity_level']);
It violates the
watchdog()
function signature, I am wrong ?Comment #231
Reg CreditAttribution: Reg as a volunteer commentedThe patch doesn't try to store anything into the DB that Drupal doesn't already try to store into the DB. All as the patch does is give backtrace detail in a nice format when you choose to turn backtracing on. Of course that doesn't mean that what you are saying is wrong but we should probably raise a separate issue for that.
That bit of code is there because I came across cases where Drupal was trying to write PDOExceptions to the DB. See the discovered issue I found while doing this: https://www.drupal.org/node/2606742 as a case in point.
Comment #232
Reg CreditAttribution: Reg as a volunteer commentedRemoved some testing code.
Comment #234
klausiHm, this patch is a bit too large for my taste so I developed a smaller one with print_r($backtrace) for watchdog() in #2654406: Log backtrace of PHP errors and exceptions. It only logs a lightweight backtrace and does not show it because during development I use the devel module anyway.
Comment #237
geek-merlinGreat work to get this into core!
In the meantime the abandoned code from commerce_devel lives here:
https://www.drupal.org/project/pbt
Just for inspiration.
Comment #238
Anybody@axel.rutz thank you very much for this great module. Hopefully we'll see this in D7 core soon. Thank you all!
Comment #239
Reg CreditAttribution: Reg commentedI'm glad people are still supporting this! I kind of dropped the patch I was creating here because I didn't think anyone was interested. I'll update it so it passes the bot again in the next couple of days.
Comment #240
Chris CharltonI see commits for 8.x, any additional patches needed for 7.x?
Comment #241
Reg CreditAttribution: Reg commentedAll my patches are for 7.x and you will see that this issue is set for version 7. I'm not sure why the version 8 commit(s) are here.
Maybe just someone didn't look carefully at the version of the issue before adding a commit here?
Perhaps a new issue linked to here for version 8 would be appropriate and then a back ported version from 8 can be done here if it's needed to be done that way to keep with policy?
Comment #242
jimmyko CreditAttribution: jimmyko as a volunteer commentedIt is so weird that it is committed to Drupal 8 on a issue tagged with Drupal 7. And the committers just ignoring the questions raised here and even not leaving any message here for what they have done and reasons...
Comment #243
Alan D. CreditAttribution: Alan D. commented#235 & #336 are just auto-generated comments the old commits going to the new branch I believe from way back in 2012 :)
http://cgit.drupalcode.org/drupal/commit/?id=fd352bf
Comment #244
jimmyko CreditAttribution: jimmyko as a volunteer commented@AlanD Thanks for the explanation. So this feature is still absent from both D7 and D8...
Comment #245
sinasalek CreditAttribution: sinasalek at Practicalidea commentedThere are several issues similar to this. this one is for D8 #2638140: Error logging should log a backtrace consistently.
And there is another one for D7 #2777955: [D7] Enable error logging to log a backtrace string which is simpler
I did a bit of comparison and the patch here is much more customizable
Comment #248
deanflory CreditAttribution: deanflory as a volunteer commentedadd_backtrace_to_all_errors-1158322-221-D7.patch applied to D7.54:
patching file includes/errors.inc
Hunk #6 FAILED at 210.
patching file modules/dblog/dblog.module
Hunk #1 FAILED at 144.
Comment #249
geek-merlinComment #250
Chris CharltonWhy did the D7 patch reviews get stalled?
Comment #251
Reg CreditAttribution: Reg as a volunteer commentedIt's been quite a long time since this patch was updated so I've updated it to get it compatible with the latest D7. I'll do an interdiff after it passes and I'm sure there's all sorts of new checks since last time I did this so that I'll have to probably try a few times before it passes.
Comment #252
Reg CreditAttribution: Reg as a volunteer commentedComment #256
Reg CreditAttribution: Reg as a volunteer commentedComment #257
Reg CreditAttribution: Reg as a volunteer commentedComment #258
Reg CreditAttribution: Reg as a volunteer commentedComment #259
Reg CreditAttribution: Reg as a volunteer commentedComment #260
Reg CreditAttribution: Reg as a volunteer commentedThat passed so I added the interdiff. The problem was actually a core fault and I created an issue for it here: https://www.drupal.org/project/drupal/issues/2934298.
There was also a tag suggesting the issue summary needed to be updated so I have updated it and removed the tag.
Comment #261
Reg CreditAttribution: Reg as a volunteer commentedComment #262
Reg CreditAttribution: Reg as a volunteer commentedComment #263
Reg CreditAttribution: Reg as a volunteer commentedComment #264
Reg CreditAttribution: Reg as a volunteer commentedComment #265
Alan D. CreditAttribution: Alan D. commentedShouldn't all the string trims simply use drupal_substr() here? The mb_* functions are not a requirement
Comment #266
Reg CreditAttribution: Reg as a volunteer commented@alan-d I would love it to be that easy but I tested many combinations, here is the test code (please forgive how long "$link = ..." is here, the editor did this):
and THE ONLY ONE that gave a result that made sense and that the DB & tests passed was the results from:
file_put_contents('./mbstrimwidth.txt', mb_strimwidth($link, 0, 255, 'UTF-8'));
.Many times I have deferred to the wisdom of what has already been done (there are many smarter people than me working on this) but in this case there is empirical evidence to the contrary.
drupal_substr()
will not solve this problem and perhaps given enough stress testing it will be modified to incorporatemb_strimwidth()
however these functions perform different tasks and I suspect thatdrupal_substr()
will remain as is and thatmb_strimwidth()
will we used where appropriate, like here.On another note, this part of
drupal_substr()
:predisposes there is some global recognition of multibyte being present (that that ONLY checking for UNICODE is sufficient) as a factor. I don't think that's an appropriate flag in this case. It's better to assume the worst case and always get it right regardless of any flags.
Comment #267
Alan D. CreditAttribution: Alan D. commentedThe main issue is that mbstring is a non-default extension with PHP that is not required to run Drupal, so this will cause a fatal error on many systems where that extension is not installed.
As an aside.
My local tests had no issue using mb_substr() via drupal_strlen() nor mb_strimwidth() writing to the watchdog table using your example above. My table charset was utf8,
Tests done on a running Drupal install with mbstring enabled
substr() failed both when it wrote the message to the screen (last char was invalid) and the database. (an exception)
#251 changed drupal_strlen() with substr(), the failing test doesn't suprise me
#256 changed drupal_strlen() with substr() & mb_strimwidth(), so this removes the additional charset protection on type & hostname
When $multibyte == UNICODE_MULTIBYTE comes into play is with _unicode_check() that will only happen after mbstring is set up via
Personally, is a link doesn't fit 255 chars, it's becomes meaningless once trimed, but that's another story lol
Comment #268
Reg CreditAttribution: Reg as a volunteer and commentedLOL, I was thinking the same thing about the link when I was looking at it getting truncated. Anyway, if some systems not having multibyte functions installed is the only issue, I think we have simple solution... looking through the Drupal unicode.inc file I found drupal_truncate_bytes() which seems to do the trick. That said, I couldn't imagine any system not having multibyte functions these days.
New patch using drupal_truncate_bytes() attached.
Comment #269
Reg CreditAttribution: Reg as a volunteer and commentedComment #271
Reg CreditAttribution: Reg as a volunteer and commentedComment #272
Reg CreditAttribution: Reg as a volunteer and commentedComment #274
Alan D. CreditAttribution: Alan D. commentedThese were the 3 bits I was looking at. This happily executes watchdog('test', 'A test', array(), WATCHDOG_DEBUG, $link); using your link example above.
From
To
Re-rolled to run the automated testing. Note, I have not reviewed the rest of the code other than getting rid of the "No newline at end of file" part of the patch in #272
Comment #275
Reg CreditAttribution: Reg as a volunteer and commentedI went through and cleaned up as much of the formatting notices as I could and did a couple of other improvements as well.
It's interesting that drupal_substr() on the link doesn't cause a DB error because it creates a total length of 723 bytes so the 255 DB limit is clearly multibyte friendly.
Comment #276
Reg CreditAttribution: Reg as a volunteer and commentedComment #277
Alan D. CreditAttribution: Alan D. commentedNice work on the tidy up. It will help speed things up :)
Note: I tried with and without the mbstring extension enabled without issue, but the db was default the MySQL CHARSET=utf8 from the install I was testing on (old Drupal 7 one from back in 2104).
I'd assume that if drupal_substr() failed here it would potpourri of unrelated issues, which was why I found it unusual the need for the change :)
Checking core, it seems to be handled via drupal_strlen() normally, as most things indirectly set the limit via forms:
i.e. Node form title #maxlength = 255 & the length gets validated via drupal_strlen()
But a couple of places do use truncate_utf8(). I guess these handle data from sources outside of Drupal that could contain characters that need special processing? But these to be the exception rather than the norm.
Comment #278
Reg CreditAttribution: Reg as a volunteer and commentedComment #279
Reg CreditAttribution: Reg as a volunteer and commentedJust some more code formatting improvements.
Comment #280
Reg CreditAttribution: Reg as a volunteer and commentedGuys,
This issue was started in 2011 and I got involved in 2013, that's 7 & 5 years ago! respectively. There's a lot of interest and a definite need for it and we finally got an old patch up to date with the latest Drupal (7).
It would be great if someone from the core Drupal team would pick this up and run with it before this patch again gets so dated that it needs a serious revamp.
Any takers?
Comment #281
Chris Charlton+1 to #280.
:)
Comment #282
SKAUGHTComment #283
Alan D. CreditAttribution: Alan D. commentedQuick followup code std review to help speed things up even more ;)
https://www.drupal.org/pift-ci-job/853993
798 coding standards messages - 36 more than branch result
There seems to be trailing whitespace in a few places.
Search for "Whitespace found at end of line" in https://www.drupal.org/pift-ci-job/853993 or enable trim on save options in your IDE :)
Doc comment short description must end with a full stop
The first word after the colon should be in lowercase unless it is a proper noun.
Automatic testing complains about multiline comments a bit.
Most of the "Error reporting type of debug information" parts appear redundant?
Indicates that backtrace information is added to the messages on the page.
Indicates that backtrace information is added to the logs.
Display character limit of debug backtrace scalar arguments.
etc
Required? See alternative below for format_backtrace() that would make this redundant.
includes/bootstrap.inc
function array_has_PDOException($array) {
2636 Doc comment short description must be on a single line, further text should be a separate paragraph
2638 Expected "mixed" but found "type" for parameter type
2639 Parameter comment must end with a full stop
2641 Expected "bool" but found "boolean" for function return type
2644 Invalid function name, expected array_has_p_do_exception but found array_has_PDOException
Func name lowercase: array_has_pdoexception()
Short and sweet description should be enough?
Parses an array containing a debug backtrace for a PDOException class.
The type indicator type should be array if you use that, maybe $debug_info instead of $array so it reads nicer?
includes/errors.inc
399 Equals sign not aligned with surrounding assignments; expected 7 spaces but found 11 spaces
400 Equals sign not aligned with surrounding assignments; expected 4 spaces but found 8 spaces
401 Equals sign not aligned with surrounding assignments; expected 1 space but found 5 spaces
402 Equals sign not aligned with surrounding assignments; expected 2 spaces but found 6 spaces
Not sure on the std here, though this is easier to read :P
includes/errors.inc
466 Space found before semicolon; expected "EOT;" but found "EOT ;"
function add_backtrace_styling()
modules/dblog/dblog.module
171 Comment indentation error, expected only 1 spaces
Maybe the @code tags?
Personal preference, it's nice to see what random integers mean when they are used:
And readability within format_backtrace() easier with this?
Comment #284
Chris CharltonCan't wait to share this with our meetup group when it gets in!
Comment #285
SKAUGHTComment #286
Reg CreditAttribution: Reg as a volunteer and commentedI'll do a clean up on these coding conventions this weekend.
Comment #287
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI appreciate the work that has gone into this, but I'm not sure what framework-level review is needed here right now besides the one I already provided in #226 and #228 a long time ago. Because of those issues, which are substantial, I don't see how the patch could be committed to Drupal 7 in its current form.
Comment #288
SKAUGHTThanks Dave. i knew the tag was a little overzealous in that way. Which comes from it's own issue of process.. many of us do not know which tags to add to escalate issue reviews (past the group of people already watching it).
Comment #289
mikeytown2 CreditAttribution: mikeytown2 commentedWhat is needed is a straight backport from 8.x. Any new feature needs to land in 8.x and then 7.x. That is the issue with this patch, it does too much in comparison to what's in 8.x currently.
I like where this is headed but this needs to be a backport from 8.x first.
Comment #290
Reg CreditAttribution: Reg as a volunteer and commentedI don't know what you expect by a backport from Drupal 8. I would get the importance if the error trapping in D8 had some semblance to D7 but the error trapping part that matches D7 at all is a very small part of the error trapping in D8. Last I looked most of the error trapping is in symfony (although that was a few years ago so it could have changed quite a bit since then).
By the time you wrote code that gave a similar output in D8 that we are doing here there wouldn't be anything significant to backport - perhaps only the formatting function. It would instead just be a rewrite, not really a backport, which presumably would result in something like what we have now. So, if it's going to be code that almost doesn't relate at all, what do you hope to gain by requiring to do it that way?
Comment #291
dalinThe most recent patch was attempting to do a lot more than just add basic backtrace info to error logs/messages. It also had some problematic (mis)use of Drupal APIs, and confusing nomenclature.
Here's a much stripped down version that is fairly similar to D8. Hopefully this is a whole lot more core-worthy. Admittedly I haven't read all 290 comments on this issue, so I'm not sure if someone has already tried this simple approach.
Comment #292
dalinFixed a bug and plugged a vulnerability.
Comment #293
dalinComment #294
dalinNow with support for exceptions.
Comment #295
nerdsteinI tested the patch in #294 and it works great.
To deploy, I ran the following Drush command:
Comment #296
Gnanasampandan Velmurgan CreditAttribution: Gnanasampandan Velmurgan as a volunteer commentedPatch #294 Its looks good to me. Its works. Thanks!
Comment #297
jhedstromGiven #2908079: Move some of the bootstrap.inc PHP-related constants to \Drupal and deprecate the old versions, we should probably avoid adding new constants to
bootstrap.inc
I think...Comment #299
anrikun CreditAttribution: anrikun commented+1 for #294
Comment #300
anrikun CreditAttribution: anrikun commentedComment #301
klausiUnfortunately the patch from #294 does not handle exceptions that are thrown somewhere in code. Then the backtrace is useless, example:
When we have an exception then we need to log the backtrace of the exception, not the backtrace to the exception handler.
Attached is a small update to the patch that handles that.
Comment #302
aurelianzaha CreditAttribution: aurelianzaha at jobiqo - job board technology commented+1 for https://www.drupal.org/project/drupal/issues/1158322#comment-14182851
Comment #303
aurelianzaha CreditAttribution: aurelianzaha at jobiqo - job board technology commentedComment #304
eliosh+1 for the #301 patch.
Is there a plan for a D8+ patch also ?
Comment #305
dalin@eliosh, while this comment thread is super long, this was first committed to D8 about 10yrs ago. All that's left is D7. The latest D7 patch is significantly better than what we have in D9, but any improvements for that should be in new issues.
Comment #306
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedPatch #301 looks good, we are also using it, thanks!
It should help a lot in a cases like
EntityMalformedException
, where you currently need to print the backtrace manually to find the root of the problem.However this last version of the patch is still a little bit "improved" over the D8 patch commited earlier. Actually, the patch #301 seems to be a combination of this issue backport and another issue backport: #2777955: [D7] Enable error logging to log a backtrace string
I am not sure if maintainers would allow this to get in or if it would needs to be in two separate backports.
Comment #307
solideogloria CreditAttribution: solideogloria commentedThe commits that were added in #298 might not be using
error_reporting()
correctly as of PHP 8.See #3322606: Does the Drupal error_reporting() check still work?
It'd be good if someone else could see if a fix is needed.
Edit: The call to
error_reporting()
actually existed before these commits, but the issue is still related, as the code might not be working as intended.Comment #308
Anybody++!
Please create follow-ups for further improvements and let's get this finally committed, before we're on page 2 here ... -.-
Comment #309
anrikun CreditAttribution: anrikun commented+1 for #301 too
Please commit!
Comment #310
Stoob CreditAttribution: Stoob as a volunteer commented+1 and thank you for the wonderful #301 patch. Don't forget to turn backtrace in settings at admin/config/development/logging after patch applied
Comment #311
anrikun CreditAttribution: anrikun commentedActually additions made in patch at #301 may cause crash below in theme_dblog_message() when the exception's class is not a standard one:
Object of class __PHP_Incomplete_Class could not be converted to string
As $error['exception'] is no longer used after getting its trace, I suggest we unset it then.
Comment #312
anrikun CreditAttribution: anrikun commentedThis new patch only adds:
unset($error['exception']);