Currently it goes like this:

<?php
    foreach ($trace as $key => $value) {
      $rich_trace[$value['function']] = $value;
    }
?>

Better would be:

<?php
    foreach ($trace as $key => $value) {
      $rich_trace[$key . ': ' . $value['function']] = $value;
    }
?>

This way we get unique array keys, even for repeated functions.

CommentFileSizeAuthor
#8 devel.ddebug_backtrace.d6.patch797 bytesdonquixote
#8 devel.ddebug_backtrace.d7.patch785 bytesdonquixote
#3 ddebug_backtrace.0.png9.75 KBsalvis
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented

I don't have the context here...

What's the key of $rich_trace used for? What is in $key?

Will this have any effect on what is displayed? Screenshot?

donquixote’s picture

donquixote CreditAttribution: donquixote commented

Ah, sorry. I should have explained further.
http://drupalcontrib.org/api/function/ddebug_backtrace
The problem is that $value['function'] is not unique and thus not suitable as an array key.

Say you have a stack trace of
index.php -> foo_a(1) -> foo_b(2) -> foo_c(3) -> foo_b(4) -> foo_c(5) -> foo_d(6)

If you use the function name as a key, this will collapse to
index.php -> foo_a(1) -> foo_b(4) -> foo_c(5) -> foo_d(6)
because foo_b(4) overwrites foo_b(2).

$rich_trace is the array that will be displayed by ddebug_backtrace().
$key is a simple number, afaik, with the benefit of being unique.

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented
FileSize
ddebug_backtrace.0.png9.75 KB

Ok, I agree that this needs to be fixed. I provide the screenshot and you provide the patch file (and set the status to CNR when it's attached).

Can we avoid showing the numbers? They provide no information and add a bit of confusion because they go the wrong way.

donquixote’s picture

donquixote CreditAttribution: donquixote commented

Could we simply reverse the numbers?
I think they do provide some information: It is sometimes interesting to know how many stack levels we have.

patch: You will have to wait a bit for that. But it would be not that different from the code I posted in the topic starter.

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented

Yes, reversing the numbers would work for me.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

I am OK with the proposed fix. Either ascending or descending is fine with me.

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented

We're still waiting for donquixote's patch...

An alternative to numbering would be to add an (invisible) <span> tag with a class attribute containing an incrementing number. But I agree that visible numbers might be useful on some occasions, but the counting should start at menu_execute_active_handler() and not the other way around.

donquixote’s picture

donquixote CreditAttribution: donquixote commented
Status: Active » Needs review
FileSize
devel.ddebug_backtrace.d7.patch785 bytes
devel.ddebug_backtrace.d6.patch797 bytes

You are obsessed about patches..

donquixote’s picture

donquixote CreditAttribution: donquixote commented

Some variables renamed:
"$call" instead of "$value". The items in a stack trace are function calls.
"$i" instead of "$key". The keys in the original $trace array are numeric.

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented
Status: Needs review » Closed (fixed) 
+++ devel.module	7 Dec 2010 18:05:50 -0000
@@ -1631,8 +1631,10 @@
+      $key = ($count - $i) .': '. $call['function'];

The D7 coding standards prescribe spaces on both sides of the concatenation (dot) operator.

If I was obsessed by patches, I'd ask you for an updated one... :-)

On the receiving end, patches make my life easier, especially with Dreditor and an IDE like Eclipse. Try them...

Committed to D7 and D6, thanks! On to #919260: ddebug_backtrace should have a $return option like print_r...

donquixote’s picture

donquixote CreditAttribution: donquixote commented

hehe, i saw it coming .. :)
i like to have the dot directly on the quote.

(off-topic)
My problem with patching is that I usually do the bugfixes on whatever project i am currently working on, with ssh+gedit, on a server somewhere. But the cvs checkout that i need to make the patch is on my local machine. Thus, i need to checkout the latest -dev in cvs on my local machine, then manually apply the changes there, and create and upload the patch. Or, drush dl the latest dev on a test install somewhere, apply the changes, test it, then manually apply it to the cvs checkout on my local machine. With the number of bugs I report on d.o., this is a serious extra effort, compared to just explaining what is wrong in the code.
On the other hand, on my own projects, I usually don't care about patches, since most often I have my own ideas how something should be implemented. Better if someone explains what I need to change, and why.
(/off-topic)

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented

[Oh, I won't deny that preparing a patch is more work than just dropping a few lines of code, but the key to getting fixes committed is to make it as easy as possible for the committers to do their work. Getting a patch has lots of advantages, even if you don't commit it verbatim.]

donquixote’s picture

donquixote CreditAttribution: donquixote commented

Can you make a new official D6 release with the patch?

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented

No, only Moshe can...