Add http_download function to simplify downloading files

[tigitz]

Continuation of #293

It narrows the scope to a simple http_download() function for now. Separates http_upload() into its own future PR due to its unique complexities and diverse HTTP upload methods.

Feature details are in the documentation.

[TheoD02]

Why not use on_progress directly ? https://symfony.com/doc/current/http_client.html#progress-callback
In that case the progress will be possible when $stream is true or false.

The only point that can be not good idea is if the user override the on_progress, it will will loose the default behavior purposed by castor

WDYT about add a progress bar as progress indicator for no verbose case ? This might be useful on large files or for those want to show progress anyway

[lyrixx]

I didn't review the download implementation yet, but it's a good start

[tigitz]

@TheoD02

Why not use on_progress directly ? https://symfony.com/doc/current/http_client.html#progress-callback
In that case the progress will be possible when $stream is true or false.

The only point that can be not good idea is if the user override the on_progress, it will will loose the default behavior purposed by castor

I initially dismissed this idea for the reason you highlighted. However, upon reflection, I could wrap the user's on_progress callback with our implementation.

WDYT about add a progress bar as progress indicator for no verbose case ? This might be useful on large files or for those want to show progress anyway

I considered this too but opted to avoid cluttering the standard output by default since, as far as I know, few native Castor functions currently output anything. I aimed to maintain consistency by making the progress display an "opt-in" mechanism, leveraging the existing verbose feature for this purpose.

I'm open to either approach. Let's hear what others have to suggest.

[lyrixx]

I considered this too but opted to avoid cluttering the standard output by default since, as far as I know, few native Castor functions currently output anything. I aimed to maintain consistency by making the progress display an "opt-in" mechanism, leveraging the existing verbose feature for this purpose.

yes, let's be silent by default 👍🏼

[tigitz]

@lyrixx Updated according to your suggestions, I left the HttpRequester->httpClient() for now as it's not clear to me what's expected.

@TheoD02 I switched to the onprogress option as discussed. As expected, download progress code is now simpler and also work for non stream downloads. 🙂

[lyrixx]

except nitpicking, it looks good 👍🏼

[lyrixx]

Hi, for the record, I have finished this PR. I fixed all my comments, and I also rebased.

[pyrech]

Just a small question in the doc, but looks go to me otherwise. Thanks to both of you :)

[pyrech]

Should we mention the default value?

[tigitz]

@lyrixx Ah, now I understand! You wanted me to keep the HttpClientInterface alongside the newly introduced HttpDownloader instead of merging all HTTP responsibilities into a single HttpRequester class.

Thank you for completing the work! 👍 I hope it will be useful for others.

[lyrixx]

yes, Single Responsibility for the win 👍🏼