Bug 6122 - [review] Mail::DKIM 0.34 removed DkimPolicy::as_string, which was used in dbg call
Summary: [review] Mail::DKIM 0.34 removed DkimPolicy::as_string, which was used in dbg...
Status: RESOLVED FIXED
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: Libraries (show other bugs)
Version: 3.2.5
Hardware: All All
: P5 minor
Target Milestone: 3.2.6
Assignee: SpamAssassin Developer Mailing List
URL:
Whiteboard: needs 1 vote for 3.2
Keywords:
Depends on:
Blocks:
 
Reported: 2009-06-02 09:19 UTC by Mark Martinec
Modified: 2011-05-23 18:39 UTC (History)
3 users (show)


Attachment Type Modified Status Actions Submitter/CLA Status
comment out a dbg call patch None Mark Martinec [HasCLA]
simplifies the dbg call patch None Mark Martinec [HasCLA]
Show Obsolete (1) Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Martinec 2009-06-02 09:19:18 UTC 
Created attachment 4457 [details]
comment out a dbg call

This is rather trivial. The Plugin::DKIM is calling a routine
Mail::DKIM::DkimPolicy::as_string within a dbg() call.
The version 0.34 of Mail::DKIM removed sub as_string,
so the following error results:

[15007] dbg: dkim: lookup failed:
  Can't locate object method "as_string" via package
  "Mail::DKIM::DkimPolicy"
  at /usr/local/lib/perl5/site_perl/5.10.0/Mail/SpamAssassin/Plugin/DKIM.pm
  line 474.

If there will ever be a 3.2.6, the attached quick patch just
comments out the dbg call, it is non-essential.

Note that 3.3 is unaffected by the change in Mail::DKIM 0.34, it
already uses its different API to fetch author signing domain policy.
Comment 1 Kevin A. McGrail 2009-06-02 10:00:24 UTC 
-1 on the patch but in spirit it's a simple patch and I have a nuance change.

Assuming there is no replacement for the as_string function that is likely below the intended external API availability, I would keep the debug line but remove the as_string call. i.e.:

dbg("dkim: policy result $policy_result");

But I'll also ask the DKIM module author if he has a recommendation.
Comment 2 Mark Martinec 2009-06-02 10:39:13 UTC 
Created attachment 4458 [details]
simplifies the dbg call

Makese sense, thanks. Here is a new patch.
Comment 3 Karsten Bräckelmann 2009-06-02 15:42:14 UTC 
+1 on the second patch
Comment 4 Mark Martinec 2009-06-09 02:13:20 UTC 
Thanks to Kevin for contacting the author of Mail::DKIM, the version 0.36
has been published, making the missing Mail::DKIM::DkimPolicy::as_string
available again. So the attached patch is only necessary for versions
0.34 and 0.35, but it does no harm to other versions (except for less
informative log entry).

There are two choices now, go on with the patch, or require a 0.36+ .
I'd still go with the patch, but the need for it is much reduced now.

My reason for dropping the logging of a policy is that this area
of SSP/ADSP processing has been much revised, both in the IETF WG,
in the Mail::DKIM plugin and in the version of DKIM Plugin in SA 3.3,
so the code section we are discussing here will go away sooner or later.
We are only discussing a quick fix here.
Comment 5 Kevin A. McGrail 2010-05-25 18:25:05 UTC 
(In reply to comment #4)
> Thanks to Kevin for contacting the author of Mail::DKIM, the version 0.36
> has been published, making the missing Mail::DKIM::DkimPolicy::as_string
> available again. So the attached patch is only necessary for versions
> 0.34 and 0.35, but it does no harm to other versions (except for less
> informative log entry).
> 
> There are two choices now, go on with the patch, or require a 0.36+ .
> I'd still go with the patch, but the need for it is much reduced now.
> 
> My reason for dropping the logging of a policy is that this area
> of SSP/ADSP processing has been much revised, both in the IETF WG,
> in the Mail::DKIM plugin and in the version of DKIM Plugin in SA 3.3,
> so the code section we are discussing here will go away sooner or later.
> We are only discussing a quick fix here.

+1 to require 0.36+ of DKIM for 3.3.X next realease.  It's been almost a year since this issue was resolved by the author of DKIM.
Comment 6 Adam Katz 2010-05-25 20:01:51 UTC 
(In reply to comment #5)
> +1 to require 0.36+ of DKIM for 3.3.X next realease.  It's been almost a year
> since this issue was resolved by the author of DKIM.

spamassassin 3.3.x is in lenny-backports but libmail-dkim-perl is not present there.  I'm willing to give my vote only if the newly required version of DKIM finds its way to Debian Stable via backports, volatile, or some other supported production-grade repository ... or if this particular issue's solution is otherwise back-ported into a debian-packaged DKIM.

I'll forward the email this comment generates to the Debian Backports team as a request on that front.
Comment 7 Darxus 2011-05-23 17:51:10 UTC 
Needs 1 vote to commit to 3.2, which looks like it has less complications than changing the Mail::DKIM requirement to v0.36+.

Does not affect 3.3.
Comment 8 Mark Martinec 2011-05-23 18:39:26 UTC 
> Needs 1 vote to commit to 3.2, which looks like it has less complications than
> changing the Mail::DKIM requirement to v0.36+.
> Does not affect 3.3.

This is really a trivial patch to a debugging entry in 3.2, so I'll apply
it to 3.2 and close the ticket, despite the fact that 3.2.6 probably
won't be released at all, and that we are lacking one vote.
The problem has long been fixed in 3.3.

3.2: (!)
  Bug 6122: Mail::DKIM 0.34 removed DkimPolicy::as_string,
  which was used in dbg call
  Sending lib/Mail/SpamAssassin/Plugin/DKIM.pm
Committed revision 1126649.