Skip to content

Replace all debug_backtrace boilerplate with get_backtrace_loc - #1654

Merged
cpeel merged 1 commit into
DistributedProofreaders:masterfrom
bpfoley:level-7-backtrace
Oct 6, 2026
Merged

cpeel merged 1 commit into
DistributedProofreaders:masterfrom
bpfoley:level-7-backtrace

Conversation

@bpfoley

@bpfoley bpfoley commented Sep 28, 2026

Copy link
Copy Markdown
Collaborator

...which does thing in a way that PHPStan level 7 approves of and can handle missing line numbers or filename in the backtrace info.

NB this produces different DPDatabase log output
"in foo.php on line N" instead of "foo.php:N"

Comment thread pinc/misc.inc Outdated
* How deep in the call stack to get the location. 0 is the caller
* of `get_backtrace_loc`, 1 is the caller of that function, etc
* @return string
* Returns a trace location like " in foo.php on line 742"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It feels weird to have this function return leading whitespace -- that was unexpected. I'd rather the callers manage adding whitespace where needed.

Comment thread pinc/misc.inc Outdated
Comment on lines +1677 to +1684
$trace = debug_backtrace();
if (($file = $trace[$depth]["file"] ?? "")) {
$file = " in $file";
}
if (($line = $trace[$depth]["line"] ?? "")) {
$line = " on line $line";
}
return $file . $line;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Perhaps something like this:

Suggested change
$trace = debug_backtrace();
if (($file = $trace[$depth]["file"] ?? "")) {
$file = " in $file";
}
if (($line = $trace[$depth]["line"] ?? "")) {
$line = " on line $line";
}
return $file . $line;
$return = [];
$trace = debug_backtrace();
if (($file = $trace[$depth]["file"] ?? "")) {
$return[] = "in $file";
}
if (($line = $trace[$depth]["line"] ?? "")) {
$return[] = "on line $line";
}
return join(" ", $return);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The leading whitespace is deliberate though: it's there to make it easier on the caller to just append the location to the error message without having to worry about having to insert an extra space when the location isn't empty, eg Undefined property via __set(): ' . $name . get_backtrace_loc()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, but we can just:

'Undefined property via __get(): $name ' . get_backtrace_loc(),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't really care if there's a trailing whitespace on exception messages 🤷

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

ACK. Changed as you suggested.

...which does thing in a way that PHPStan level 7 approves of and can
handle missing line numbers or filename in the backtrace info.

NB this produces different DPDatabase log output
"in foo.php on line N" instead of "foo.php:N"
Comment thread pinc/misc.inc
@cpeel
cpeel merged commit 4b2be94 into DistributedProofreaders:master Oct 6, 2026
12 checks passed
@bpfoley
bpfoley deleted the level-7-backtrace branch October 6, 2026 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants