-
Notifications
You must be signed in to change notification settings - Fork 11.6k
[12.x] Add Http::batch #56946
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 12.x
Are you sure you want to change the base?
[12.x] Add Http::batch #56946
Conversation
I wonder if it would be possible/better to have a syntax similar to Http::pool(fn (Pool $pool) => [
$pool->get('http://localhost/first'),
$pool->get('http://localhost/second'),
$pool->get('http://localhost/third'),
])->before(function (Pool $pool) {
// Logic here...
})->progress(function (Pool $pool, int|string $key, Response $response) {
// Logic here...
})->then(function (Pool $pool, array $results) {
// Logic here...
})->catch(function (Pool $pool, int|string $key, Response|RequestException $response) {
// Logic here...
})->finally(function (Pool $pool, array $results) {
// Logic here...
}); |
@hafezdivandari I thought about that. |
@taylorotwell an alternative approach to this implementation, that wouldn't create a breaking change can be creating a different method like Http::lazyPool(fn (LazyPool $pool) => [
$pool->get('http://localhost/first'),
$pool->get('http://localhost/second'),
$pool->get('http://localhost/third'),
])->before(function (LazyPool $pool) {
// Logic here...
})->progress(function (LazyPool $pool, int|string $key, Response $response) {
// Logic here...
})->then(function (LazyPool $pool, array $results) {
// Logic here...
})->catch(function (LazyPool $pool, int|string $key, Response|RequestException $response) {
// Logic here...
})->finally(function (LazyPool $pool, array $results) {
// Logic here...
})->dispatch(); If you prefer this approach, I can update this PR towards this approach. |
Maybe |
@hafezdivandari I like the names. |
You could probably just call it Please mark as ready for review when the requested changes have been made. 🫡 |
Thanks @taylorotwell |
Hey there @taylorotwell I updated the implementation and also updated the PR description to reflect the changes in the implementation and the use case that was faced, which I think this implementation allows an elegant and simple way to solve it. |
Pull requests like this 🫶 Great work @WendellAdriel 👌 |
Thank you so much @negoziator 🫶🏼 |
Can the batch be added to dynamically later? Or is it that once it's created, the batch size must stay the same and a new batch may be created? |
Since you have access to the Batch in each of the hooks, you can add more calls by calling like: $batch->get(...); |
Not sure about this one but since Laravel 12 requires PHP 8.2 wouldn't it be good if the public properties would be read-only? And have type declarations as close as possible? |
That's a good question I had when creating the code. |
@WendellAdriel In job batches, |
@taylorotwell yeah, I saw that. |
Does progress work as expected? I just tried this and found weird results. Http::batch(fn ($batch) => [
$batch->get('https://httpbin.org/delay/1'),
$batch->get('https://httpbin.org/delay/10'),
])->before(function () {
echo 'Before: '.date('Y-m-d H:i:s').PHP_EOL;
})->progress(function () {
echo 'Progress: '.date('Y-m-d H:i:s').PHP_EOL;
})->send(); I'd expect the output the be something like:
But instead I consistently see something like:
I believe there is a problem in the foreach loop where the promises have already been combined into a single promise. Maybe I'm missing something, though. |
And even if we were to fix that, I am wondering is the Imagine I have the following, where I run a slow and a fast request. Http::batch(fn ($batch) => [
$batch->get('https://httpbin.org/delay/10'),
$batch->get('https://httpbin.org/delay/1'),
])->before(function () {
echo 'Before: '.date('Y-m-d H:i:s').PHP_EOL;
})->progress(function () {
echo 'Progress: '.date('Y-m-d H:i:s').PHP_EOL;
})->send(); Due to PHP, I assume we would be resolving these sync, so the progress closure for the fast request would not trigger until the long request has finished? I understand that curl multi is likely processing both of the requests under the hood at the same time, but when it comes to iterating over them in PHP, what output would we expect see? I assume we would see:
But I wonder if a user of the method would expect:
Due to the issue I raised above, I can't test this myself. Once that is resolved I'm happy to play with it and find out. |
Oh, that's a good catch @timacdonald |
…el-framework into feat/http-pool-hooks
@timacdonald @taylorotwell But with this approach, we have two things that I want to check if it's ok and we can move on like this or if we should dig more into. 1 - For making the progress hook work correctly, the way it handles the promises, there's no way to cancel the batch because it will resolve things in a non-standard order + it will dispatch all promises at the same time. Is it ok if I remove the cancel feature? 2 - Also because of this, adding new calls to the batch won't work as expected as far as I could check. I didn't find a way to make these two things work with this new approach. |
@WendellAdriel, Taylor might not see this while it is in draft. I would recommend opening it to get his feedback. |
Thanks @timacdonald! @taylorotwell I opened again if you want to take a look on what I said above. This seems to work good, but we would need to take out the cancel mechanism and adding new calls to the already initiated batch. Let me know how you want me to proceed, and I'll be happy to work on any needed changes! 🔥 |
Overview
Inspired by the hooks from the
Bus::batch
, this PR adds a newHttp::batch
with the same hooks that are available in theBus::batch
.Use Case
Imagine that your application needs to connect to 3 different 3rd-party services to setup configurations for your customers, and you want to track the progress of the overall and also take actions after each one of these calls finish (handling success and error cases) and after everything finished you want to send a notification to the client.
With the current
Http::pool
, this is not possible. But with the newHttp::batch
, you can do that easily by setting up hooks that can run to cover all of these needs.Example
Helper Methods/Properties
Besides the Hooks, the
\Illuminate\Http\Client\Batch
class also provides some helper methods and properties to check details from the batch of requests:$batch->totalRequests
gives you the count of requests in the batch.$batch->pendingRequests
gives you the count of pending requests in the batch.$batch->failedRequests
gives you the count of failed requests in the batch.$batch->createdAt
gives you the datetime that the batch was created.$batch->cancelledAt
gives you the datetime that the batch was cancelled.$batch->finishedAt
gives you the datetime that the batch finished to execute all the HTTP requests.$batch->processedRequests()
gives you the total number of requests that have been processed by the batch.$batch->completion()
gives you the percentage of requests that have been processed (between 0-100).$batch->hasFailures()
returns true if the batch has at least one failed request.$batch->finished()
returns true if the batch has finished executing.$batch->cancel()
to cancel the batch (this will stop the batch to execute the pending requests).$batch->cancelled()
returns true if the batch was cancelled.