Added (and set as default) a cURL-based method for fetching HTTP content

[julienCXX]

Implements proposal in #531.

[EDIT 2016-08-10] Relates to sebsauvage#114 (virtualtam)

[ArthurHoaro]

Thanks!

A few remarks:

• unit tests are failing because PHP changed CURLOPT_PROGRESSFUNCTION in v5.5. Not sure what's the best way to handle this. Ping Drop PHP 5.3/5.4 compatibility #599.

5.5.0 Added the cURL resource as the first argument to the CURLOPT_PROGRESSFUNCTION callback.

• the documentation should be updated in Github's wiki, it'll be included in the repo at the next release.
• it looks like you're downloading the remote page in every case, while the original function only download it if it find a 200 HTTP code. That's not a bad idea, because Shaarli will only make 1 request and users don't usually shaare error pages, however both method should have the same behaviour.

[julienCXX]

I think it is fixed now, for the first remark.

For the latter one, I can return an empty body on 40x HTTP codes, even though the body would be still downloaded.
Alternatively, I can make cURL stop immediately when it encounters one of the said codes. But the original headers would not be returned.
Finally, I could send 2 requests, in the fashion of the fallback method, but that would make the code less readable.

[ArthurHoaro]

Nah, I like what you did, removing the or code = 200 here is a better idea, IMHO.

[ArthurHoaro]

If the arguments list is too long, we usually use one line per argument, as stated in PSR-2.

[julienCXX]

Nah, I like what you did, removing the or code = 200 here is a better idea, IMHO.

OK to do that, but I would remove the condition on line 159 too.

[ArthurHoaro]

No. We make an extra request changing one parameter if we don't get a HTTP 200. However, if we already have a valid response, the extra request is not necessary.

[julienCXX]

OK, I made all the changes you stated. And also:

• in cURL method: put received headers with the same name in a sub-array, like in the fallback method
• in fallback method: found a way to send Accept and Accept-Language headers, like in the cURL method
  Thus, with the fallback method, http://services.gisgraphy.com/ now works and title of http://android.izzysoft.de/ appears in English.

I also noticed that get_headers() follows redirects by default, thus get_redirected_headers() might be useless. The trick here is that it mixes-up headers from the original page with all its redirects, which happened to me when building the cURL method (as I explained it in #531). And it explains the weird headers you got with https://fr.atlassian.com/git/tutorials/comparing-workflows/forking-workflow/.

[ArthurHoaro]

I also noticed that get_headers() follows redirects by default, thus get_redirected_headers() might be useless.

You're right, I don't know how I didn't see that. A bit of clean up could be done later.

I'll run a few tests but I'm OK with the current PR. Could you squash your commits and update the wiki?

[julienCXX]

Could you squash your commits and update the wiki?

Yes, I can. Which commit structure do you want?

[ArthurHoaro]

You changed one file for one feature, I'd say one commit is fine.

[ArthurHoaro]

Tested, it works fine.

[virtualtam]

*amount

[virtualtam]

+1 for merging, PR tested with a few known-to-be-problematic web sites listed in #531
(sorry, had to find a small typo somewhere :D )

Btw, this list of problematic websites could be used to add functional (vs. unitary) test suites, to check Shaarli code against real-world conditions. Let me know if there's interest, this would be quite easy to add through PHPUnit's configuration.

[ArthurHoaro]

@virtualtam It would be interesting indeed. However, aren't functional tests supposed to test the application a whole, meaning fire up a web server, login, etc., instead of components?

@julienCXX Thanks again!

[virtualtam]

Well, you can test both; usually functional tests cover a whole feature in real-world conditions, but can also be used to check features that cannot be covered through unitary tests, e.g. accessing external services. We actually do have such tests, I think most of them are related to HttpUtils.