getstore does not work sanely with Path::Tiny objects (or any other object)

[autarch]

This manifests as silently not writing the content to a file.

I think this is really part of a core bug in LWP, where LWP::Protocol->collect is probably dying but that die is being discarded.

It seems like collect should maybe check if the object in $arg can be stringified and then use that as a file path. And yes, that's broken and dumb, but so is Perl 5's overloading.

But it also seems like a bug that if collect is dying (which it has to be) that getstore doesn't catch that.

[oalders]

Do you have a quick code sample to illustrate this?

[autarch]

perl -e 'use Path::Tiny; use LWP::Simple qw( getstore ); my $p = path(q{./LWP::Simple}); getstore(q{https://metacpan.org/pod/LWP::Simple}, $p)'

[genio]

perl -e 'use Path::Tiny; use LWP::Simple qw( getstore ); my $p = path(q{./LWP::Simple}); getstore(q{https://metacpan.org/pod/LWP::Simple}, "$p")'

Stringify the path. Worked here.

[autarch]

Yes, I know stringification works. But it shouldn't fail silently and return a 200 when given an object like it currently does.

[genio]

Gah. Apologies. I completely missed the point of the issue. Failure to read on my part. Agreed, this shouldn't fail silently.

[autarch]

Heh, no apology needed. I think I've done this once or twice (or ten thousand times).

[genio]

After tracing it through (using http://checkip.amazonaws.com/ so that LWP::Protocol::https doesn't get involved), it seems you're right about where the error's occurring. LWP::Protocol->collect() fails since $arg fails the ref checks. When this happens, the x-died and client-aborted headers are set.

I'll look a bit longer to see where that error should be captured. I tend to agree with you at this point that it's a silent failure.

[oalders]

If I clean up this PR would that fix the silent failure? libwww-perl/HTTP-Message#25

[autarch]

The warning in that PR is better than nothing, but I really think this needs a deeper fix. Maybe getstore need multiple return values or something so it can signal that the request worked but something else failed. Or maybe it should throw an exception on a bad path to store in. I dunno.

[oalders]

Or maybe it should throw an exception on a bad path to store in

👍