Add support for LWP as an HTTP protocol_class

[preaction]

HTTP::Tiny doesn't support authenticating proxies, but LWP::UserAgent does. This patch adds support for other protocol_class possibilities to the HTTP repository, and adds HTTP::Response object support to the API methods.

[mohawk2]

@jberger, is this module being developed much? There are PRs from 20 months ago?

[mohawk2]

@preaction, I'd prefer you not subdir http.t, which would mean the recursive_test_files change to Build.PL could go, along with the MANIFEST change. Seems you'll also need to rebase your change since the codebase has moved along a little!

[plicease]

My preference would not to add a dependency on Class::Load.

[mohawk2]

Good call. (I'm partly noting the changes I'd like in case I have to take over and finish this PR)

[plicease]

Also I agree with not renaming http.t I am not sure I understand the purpose of that.

[preaction]

Yeah, I abandoned my effort to use Alien-Base after I figured out exactly how horrible and evil the library I was trying to download was (I should write a rant about it, but let's start off with "reinvents autotools" and end with "litters GNUMakefile in at least 2 levels of parent directory, which takes precedence in GNU Make over just 'Makefile'").

That said, would Module::Runtime be okay? or just a simple eval?

What's wrong with recursive_test_files? Older versions of Perl+EUMM support?

I'm glad I write better commit messages now, because I'm no longer sure why I renamed http.t. From the new name, my guess is going to be I was going to add other ways of determining the package to download. I know for my purposes, I needed a full, direct URL to the tarball. Looks like #26 might be where that got fixed.

Let me clean this PR up a bit. I can't do any more work at the moment, but I can at least address these issues quick.

[preaction]

http.t rename removed and rebased from master.

[plicease]

I am fine with eval.

However, if we are going to be doing a lot of dynamic module then a convenience of some sort is desirable. Module::Runtime has itself fewer dependencies and seems to reliably install as far back as 5.8.1. I'd like to hear what others think.

[mohawk2]

Let's just do: eval { require Whatever::Module } - no overpowering reason to get extra modules involved for this.

[preaction]

Done.

[mohawk2]

Is it possible to make the download be fake rather than real? This is an issue raised also on #33, so please don't duplicate effort! Also, I'd like to see a test for this great new functionality.

[preaction]

I don't know what you mean about faking the download. Do you mean making a mock protocol_class that can be used for testing?

I'll write some tests, at least. Since it doesn't look like the tests are fully complete, I may not be able to cover everything.

[mohawk2]

I mean something like in the .t file, making a fake LWP::Simple, eg (non-tested example):

{ package LWP::Simple; $INC{'LWP/Simple.pm'} = 1; sub get { "some pre-cooked data" } }

So that when you call the relevant AB functionality, it will use that and not the real module.

[plicease]

I would favor using a file:// url in a test rather than mocking LWP. That way it tests that it works with the real LWP.

Not a strong preference though.

[mohawk2]

That's an extremely good idea, do that instead ;-)

[preaction]

I will probably do both. If the user already has LWP installed, it will test the operation of LWP integration. Either way, we also test a custom connection object (that just happens to be a mock object).

I've got tests for connection() written, working on get_file() now.

[preaction]

Looks like I can't test find_links well without making "file://" act the same as "http://". I should probably make the starting tests a different pull request, then I can write better tests for this pull request on top of that one.

[mohawk2]

Speaking for myself, I like this direction. If the PR expands a little beyond the initial concept, I'm fine with that. If others aren't, we can always split some off to a separate PR. For now, carry on with this good work!

[preaction]

Okay, this is what I got now.

I can't test HTTP::Tiny via file:// URLs, it does not like it. But LWP works with file://, and indeed the tests revealed a problem in my code which is now fixed.

[plicease]

I was working on a test module that adds file support to HTTP::Tiny here:

https://github.com/plicease/App-cpangitify/blob/master/inc/Test/HTTPTinyFile.pm

It is incomplete, but might be useful here (?), and it would be great to drive development of it.

It might also be too Rube Goldberg.

[plicease]

if using subtest make Test::More 0.94 a testing prereq.

I am all for using subtest I think it makes the output more readable.

[plicease]

Should LWP be the default, if installed?

I don't want LWP to be a prereq, but making it the default if it is installed lets any Alien::Foo package that uses AB to work as expected in environments that require it, without forcing the prepreq on environments that don't need it. I think that does a good job of addressing the motivation of this PR.

I think it is still useful to allow an Alien::Foo author to require LWP, there may be situations where this is appropriate too.

[preaction]

I added the Test::More requirement as a separate commit, because subtest was already being used in t/build_flags.t.

As for making LWP the default without making it prescribed, HTTP::Tiny purports to now have support for the proxy strategy that $work uses (chansen/p5-http-tiny@30c5500), so in theory this entire PR is unnecessary. I could add a prereq on version 0.048 of HTTP::Tiny.

I don't think that the author of the Alien::Foo module should need to make a decision as to which protocol_class to use, but it just worked out that way. In any case, it's not the author of Alien::Foo that could even make that decision, it's the user trying to install the module that would need to make the choice.

For me, making an internal Alien::Foo module, I can choose a protocol_class and be certain that (a) it is installed and (b) it solves the problem of navigating the internal network. That might be a valid enough reason to continue this work. My build script is a lot simpler when everything, even external native libraries, are CPAN-style distributions.

[plicease]

Ah I figured that you were wanting something more sophisticated than basic auth (I reviewed the HTTP::Tiny doco when reviewing this PR), didn't realize that was a new feature for HTTP::Tiny.

I think LWP support is still worthwhile, since it has more features than HTTP::Tiny, but supporting both isn't entirely free. It will require extra maintenance and testing. I'd like to see some the others weigh in.

[preaction]

No, my initial pull request seems (I can't remember) completely incorrect. The real problem $work has is proxying https through an http proxy via CONNECT requests, which has struggled (there were open bug reports on LWP for years) and finally seems to be pretty well-supported throughout the various modules now (is this another sign of the Perl Revival/Renaissance?)

[mohawk2]

I'm favourable towards this too (for what it's worth), so if we can finish this off (once various other merges are done), that would be great!

[plicease]

I've rebased and updated it so that headers are available in either mode:

https://github.com/plicease/Alien-Base/commits/lwp

I vote on this version: aye.

[mohawk2]

Aye. @zmughal?

[zmughal]

@plicease's branch looks good to me.

Merge: Aye