markov2 / perl5-Mail-Message

Processing MIME messages
http://perl.overmeer.net/CPAN
1 stars 1 forks source link

Checking load before calling method against it. #1

Closed manwar closed 6 years ago

manwar commented 6 years ago

Hi @markov2

After such a long gap, here is one PR for you to review, please. This would hopefully resolve the fail report for your another distribution Mail::Box:

https://www.cpantesters.org/cpan/report/e839410c-2063-11e8-9917-f16481200c9d

  t/110mh-read.t ........... ok
  ERROR: Cannot create parser for t/folders/mh.src/9.
  Can't call method "toString" on an undefined value at /home/njh/.cpan/build/Mail-Message-3.006- 
  0/blib/lib/Mail/Message/Head.pm line 29.
  ERROR: Cannot create parser for t/folders/mh.src/14.
     (in cleanup) Can't call method "toString" on an undefined value at /home/njh/.cpan/build/Mail- 
         Message-3.006-0/blib/lib/Mail/Message/Head.pm line 29.
 # Looks like your test exited with 2 just after 2.

Many Thanks. Best Regards, Mohammad S Anwar

markov2 commented 6 years ago

Dear Mohammad, this is not a correct patch: it does remove the error during regression tests but does not solve the cause of the error. Tests are to detect errors.

I am very glad that the regression tests fail, because apparently something does not behave correctly in that set-up. Hopefully someone who can reproduce the error can investigate it.

manwar commented 6 years ago

Hi Mark,

I take your point and it is valid one. I accept my patch isn't telling the full truth. However, I still think we should check the object before invoking the method against it. I have modified it slightly as below. What do you think of it now? It's great having conversation with you.

-sub toString() { my $load = shift->load; return $load->toString if (defined $load); }
-sub string()   { my $load = shift->load; return $load->string   if (defined $load); }
+sub toString() {
+    my $load = shift->load;
+
+    if (defined $load) {
+        return $load->toString;
+    }
+    else {
+        carp "Missing header object.";
+    }
+}
+
+sub string()   {
+    my $load = shift->load;
+
+    if (defined $load) {
+        return $load->string;
+    }
+    else {
+        carp "Missing header object.";
+    }
+}

Best Regards, Mohammad S Anwar

markov2 commented 6 years ago

No. The "load" ensures that the whole object is available, so should always succeed. It must succeed not to give an invalid result.

The errors are only sparsely reported for 3.005 (3 fails on 658), and until now not a single failure for the 3.006 release (806 tests). I think it hits a perl bug or some other memory corruption. -- Have a good weekend.

           MarkOv

   Mark Overmeer MSc                                MARKOV Solutions
   Mark@Overmeer.net                          solutions@overmeer.net

http://Mark.Overmeer.net http://solutions.overmeer.net

manwar commented 6 years ago

You have a great weekend as well :-)