Fix XX: Can't call method "notes" on unblessed reference at /home/smtpd/... #227
Conversation
…pd/qpsmtpd/plugins/logging/file line 282 and line 288.
| || !$transaction->notes('file-logged-this-session')) | ||
| { | ||
| unless (defined $self->maybe_reopen($transaction)) { | ||
| return DECLINED; | ||
| } | ||
| if (UNIVERSAL::can($transaction,'isa')) { | ||
| $transaction->notes('file-logged-this-session', 1) if $transaction; | ||
| if (UNIVERSAL::can($transaction,'isa') && $transaction && UNIVERSAL::can($transaction, 'notes')) { |
There was a problem hiding this comment.
Wouldn't it be better to check that $transaction is defined before we check if it's a reference?
- if (UNIVERSAL::can($transaction,'isa') && $transaction && UNIVERSAL::can($transaction, 'notes')) {
+ if ($transaction && UNIVERSAL::can($transaction,'isa') && UNIVERSAL::can($transaction, 'notes')) {There was a problem hiding this comment.
For that matter, is checking 'isa' redundant and perhaps should be dropped entirely?
There was a problem hiding this comment.
You're raising some of the same questions that I raised in the original post.
I made the minimal change to fix the issue that I was having, because I don't understand all the subtleties of the code, but I'll be glad to post an update when we have answers...
|
I find the best way to illuminate the subtleties is to write some test coverage. If this PR introduced tests that exercised the code, I bet it would be better understood, and I'd be inclined to merge it. |
|
I'm sorry, this is beyond my abilities. I earn my living in professional C++/C# development and I do other open source development, but I don't have enough exposure to Perl to understand why I've been getting that error message (and apparently no one else), what it's trying to tell me, or to write a meaningful test that would take my seemingly unique situation into account. I'll be setting up a new server in a while, and maybe this problem will go away... |
...qpsmtpd/plugins/logging/file line 282 and line 288.
Second try after #225.
I refactored the postfix if as requested, but it looks weird. If
UNIVERSAL::can($transaction,'isa')is true, then$transactionis true, too, right? So, the original postfix if was redundant?Why do we check for
'isa'anyway? Wouldn't the check for'notes'be sufficient?And, if we don't have
notes, shouldn't we add it? Could this explain why I'm seeing LOGDEBUG output from a single transaction spread over several files (with seconds in the file name)?