1108809 - SOAP::Lite 1.12 causes error when using XMLRPC API

Status: RESOLVED FIXED

(Bugzilla :: WebService, defect)

Description

[David Lawrence [:dkl]]

https://travis-ci.org/bugzilla/bugzilla/jobs/43374627

Apache error log shows multiple:

[Mon Dec  8 21:21:43 2014] xmlrpc.cgi: Can't use string ("") as a subroutine ref while "strict refs" in use at /home/travis/perl5/perlbrew/perls/5.16/lib/site_perl/5.16.3/SOAP/Transport/HTTP.pm line 384.

Will investigate further.

dkl

Comment 3

[David Lawrence [:dkl]]

Attachment: 1108809_1.patch

Comment 4

[Frédéric Buclin]

Is 4.x also affected?

Also, I don't understand why this fix works but no the current code:

  $self->setDebugLogger(\&SOAP::Trace::debug); in new() seems to do the same job, because SOAP::Trace contains:

package SOAP::Trace;

use Carp ();

my @list = qw(
    transport   dispatch    result
    parameters  headers     objects
    method      fault       freeform
    trace       debug);
{
    no strict 'refs';
    for (@list) {
        *$_ = sub {}
    }
}

So *$_ = sub {} is exactly what you did. Any idea?

Comment 5

[David Lawrence [:dkl]]

(In reply to Frédéric Buclin from comment #4)
> Is 4.x also affected?

Porbably. Will need to try it on 4.4 but if someone installs the latest SOAP::Lite then it should fail as well. We would need to put a fix in the next release.

> Also, I don't understand why this fix works but no the current code:
> 
>   $self->setDebugLogger(\&SOAP::Trace::debug); in new() seems to do the same
> job, because SOAP::Trace contains:

I thought that as well but that code is in SOAP::Transport::HTTP::Client which AFAICT is not used by the Bugzilla code.

The only change from 1.11 to 1.12 is:
https://metacpan.org/diff/file?target=PHRED/SOAP-Lite-1.12/lib/SOAP/Lite.pm&source=PHRED/SOAP-Lite-1.11/lib/SOAP/Lite.pm

If I comment out the code added to SOAP/Lite.pm, the tests run properly again.

dkl

Comment 6

[Frédéric Buclin]

(In reply to David Lawrence [:dkl] from comment #5)
> I thought that as well but that code is in SOAP::Transport::HTTP::Client
> which AFAICT is not used by the Bugzilla code.

Yes, right. If I read the code correctly, xmlrpc.cgi calls:

my $server = new Bugzilla::WebService::Server::XMLRPC;
$server->on_action(sub { $server->handle_login(WS_DISPATCH, @_) })->handle();

which calls:

if ($ENV{MOD_PERL}) {
    our @ISA = qw(XMLRPC::Transport::HTTP::Apache Bugzilla::WebService::Server);
} else {
    our @ISA = qw(XMLRPC::Transport::HTTP::CGI Bugzilla::WebService::Server);
}

But XMLRPC::Transport::HTTP::CGI calls:

@ISA = qw(SOAP::Transport::HTTP::Server);

which calls:

@ISA = qw(SOAP::Server);

which doesn't call anything else, and so debug_logger is not defined. This looks like an upstream bug, isn't it? Either that, or we use SOAP::Lite incorrectly.

Comment 7

[David Lawrence [:dkl]]

(In reply to Frédéric Buclin from comment #6)
> This looks like an upstream bug, isn't it? Either that, or we use SOAP::Lite
> incorrectly.

Definitely an upstream bug. He probably made the change for SOAP but failed to verify that XMLRPC was still working properly. I will need to find time to write a test script and create an upstream ticket.

For now we will need to work around it to get 5.0 released as it will be hopefully going out before upstream gets around to fixing the issue.

dkl

Comment 8

[Frédéric Buclin]

FYI, this bug has already been reported upstream:

https://github.com/redhotpenguin/soaplite/pull/12/files

Comment 9

[Frédéric Buclin]

And SOAP::Lite 1.13 should be released soon, per:

https://github.com/redhotpenguin/soaplite/pull/11

IMO, you should rather wait a bit, and then blacklist 1.12 from Bugzilla/Install/Requirement.pm. That's much safer.

Comment 10

[David Lawrence [:dkl]]

(In reply to Frédéric Buclin from comment #9)
> And SOAP::Lite 1.13 should be released soon, per:
> 
> https://github.com/redhotpenguin/soaplite/pull/11
> 
> IMO, you should rather wait a bit, and then blacklist 1.12 from
> Bugzilla/Install/Requirement.pm. That's much safer.

Ok. Doesn't seem like 5.0RC will be out before the holidays with all of the issues being found. So it buys us a little more time to see if the newer SOAP::Lite gets released.

dkl

Comment 11

[David Lawrence [:dkl]]

(In reply to David Lawrence [:dkl] from comment #10) 
> Ok. Doesn't seem like 5.0RC will be out before the holidays with all of the
> issues being found. So it buys us a little more time to see if the newer
> SOAP::Lite gets released.

Thinking about this more and also was discussed in one of my recent meetings, maybe we will just leave in the
customization (hack) for RC1 so we can go ahead and get the RC out. In theory the customization would not interfere
with the new version of SOAP::Lite when it is released, and we would also not need to blacklist 1.12 in the module requirements either.

glob, thoughts?

dkl

Comment 12

[Frédéric Buclin]

1.12 is clearly bogus (see also https://rt.cpan.org/Public/Bug/Display.html?id=100885) and upstream maintainers are aware of the problem. It should be blacklisted. It's never a good idea to override an upstream method when that's not needed.

Comment 13

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #11)
> Thinking about this more and also was discussed in one of my recent
> meetings, maybe we will just leave in the
> customization (hack) for RC1 so we can go ahead and get the RC out. In
> theory the customization would not interfere
> with the new version of SOAP::Lite when it is released, and we would also
> not need to blacklist 1.12 in the module requirements either.

(In reply to Frédéric Buclin from comment #12)
> 1.12 is clearly bogus (see also
> https://rt.cpan.org/Public/Bug/Display.html?id=100885) and upstream
> maintainers are aware of the problem. It should be blacklisted. It's never a
> good idea to override an upstream method when that's not needed.

i'm not concerned about the subclassing of 'new', but setting internal 'debug_logger' variable is definitely a hack.


let's go with the current hack to address this issue.  once 1.13 is released we need to blacklist 1.12 and remove the workaround.  please file a new bug to track the blacklisting of 1.12.

Comment 14

[David Lawrence [:dkl]]

Comment on attachment 8534593 [details] [diff] [review]
1108809_1.patch

I agree for now. I do not yet see the pull request on github being merged yet even so it will still be a while before the new version shows up on CPAN. I will put the change in trunk/5.0 and then we can back it out and blacklist 1.12 when the new one is out. Hopefully before 5.0 final.

dkl

Comment 15

[:glob ✱]

Comment on attachment 8534593 [details] [diff] [review]
1108809_1.patch

Review of attachment 8534593 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob

Comment 16

[David Lawrence [:dkl]]

To ssh://[e-mail]/bugzilla/bugzilla.git
   0598a0a..1365815  master -> master

To ssh://[e-mail]/bugzilla/bugzilla.git
   c76e2cc..9e89b78  5.0 -> 5.0