Storable is used in HTTP::Headers for cloning [rt.cpan.org #120310]

[oalders]

Migrated from rt.cpan.org#120310 (status was 'new')

Requestors:

• [email protected]

From [email protected] on 2017-02-21 00:00:46:

Storable is used in HTTP::Headers and another internal fallback method is provided for the method 'clone'
Two minor issues there:
1 - there are not a single unit test which guarantee that the second clone method (without Storable) is working in the same way
2 - the method setting and check should probably happen at BEGIN time rather than run time.
3 - maybe we only need one method there, the faster, and we can probably assume that Storable is available on all System and if not simply list it as a dependencies for old perl versions

if (eval { require Storable; 1 }) {
     *clone = \&Storable::dclone;
 } else {
     *clone = sub {
     my $self = shift;
     my $clone = HTTP::Headers->new;
     $self->scan(sub { $clone->push_header(@_);} );
     $clone;
     };
 }

[neilb]

Storable is no longer used:

• it was used in 6.22
• you can see that it was no longer used in 6.24, replaced with Clone

The meat of the issue is still relevant: if the person releasing has Clone installed, then they're testing against that, and there's no explicit test for the fallback code.

Clone is a suggests dependency rather than a required. You could make it required, but looking at Clone on CPAN Testers, it supports 5.8.8+ and HTTP-Message declares 5.8.1.

Two options:

1. The fallback code could be made a _fallback_clone method, and then the fallback would be *clone = *_fallback_clone and there could be explicit tests for _fallback_clone.
2. Or simpler would be to bump the min-perl to 5.8.8, and simplify this code to always use Clone::clone.

5.8.8 was released in 2006, so would you accept a PR for (2)? This would basically be removing the code which decides on which Clone to use, and adding at the top:

use parent 'Clone';

And updating dist.ini to require Clone 0.45

Clone has other dists putting it up the river, so I don't think that should be a concern.

[oalders]

Option 2 sounds fine to me. Let's get at least one other admin to chime in on this.

[genio]

My preference is to bump the Perl prereq to 5.8.8.

[oalders]

@neilb green light!

[charsbar]

Hi. Hard requirement of a non-core XS module like Clone makes it difficult to bundle HTTP-Message in an application such as Movable Type that users may upload to a restricted server where they are not allowed to make modules by themselves. Could it be back to optional?

[oalders]

@neilb thoughts on this? ^^

[neilb]

I'm surprised that Clone was the first non-core XS dependency - is that right? Anyway ...

There's a Clone::PP on CPAN, but I'm not sure if it's a drop-in replacement (the doc says its interface was based on Clone, but it's had way fewer releases). If it is, then we could fall-back on Clone::PP if Clone isn't available.

I'll test it with Clone's testsuite, and see how it does. It's going to be a lot slower, but I'll look HTTP::Header's usage to see if that's would be a problem.

[vanHoesel]

When reading @neilb his response, it immediately beckons for Clone::MaybeXS, but then discovered there is Clone::Any.

I am not sure if we need the speed that XS gives us, I did not look into the code to check what data structures we need to clone. If those are not too big, I would be okay to only use Clone::PP and not bother about Any other.

[neilb]

I've tried Clone::PP with Clone's testsuite: half the test files pass cleanly, one needs minor work, but a couple need more investigation. So for now, I'd say Clone::PP is not a drop-in replacement for Clone. I've opened an issue on Clone::PP. Not sure when I'll get to it.

It may be that Clone::PP is enough for HTTP::Headers, but I'd feel better if it were Clone-equivalent. I may have a play with it over the xmas break, but I've got plenty of things on my list for then :-)