Skip to content
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

[10.x] Fix Http global middleware for queue, octane, and dependecy injection #47915

Merged
merged 3 commits into from
Aug 1, 2023

Conversation

timacdonald
Copy link
Member

@timacdonald timacdonald commented Aug 1, 2023

fixes #47738

We added the ability to add global middleware to the Http client.

Http::globalRequestMiddleware(fn ($request) => $request->withHeader(
    'User-Agent', 'Example Application/1.0'
));

Because Facades are cached for the duration of a request, this works as expected for each Http request.

On the queue / in octane however, these middleware are not applied.

This is because the underlying Factory is not a singleton, so the Factory instance is refreshed on each job execution / octane request. The Factory instance is where the middleware is stored.

This PR binds the Factory as a singleton. Excluding testing infrastructure, the only state the Factory maintains is the global middleware and the event service provider. As the event service provider is also a singleton, we shouldn't need to refresh it manually for Octane.

In addition to the queue and Octane fixes, this fix also means that the requests made by the NotPwnedVerifier will now pass through the global middleware, which I believe is the expected behaviour.

This is because the NotPwnedVerifier injects the Factory via the constructor, rather than accessing the Facade.

This also applies to any user-land code that is injecting the Factory class, which I also believe is the expected behaviour.

@timacdonald timacdonald changed the title [10.x] Fix Http global middleware on queue and octane [10.x] Fix Http global middleware for queue, octane, and dependecy injection Aug 1, 2023
@taylorotwell taylorotwell merged commit 4e0716b into laravel:10.x Aug 1, 2023
19 of 20 checks passed
@timacdonald timacdonald deleted the http-middleware-queue branch August 1, 2023 23:15
@bernardwiesner
Copy link
Contributor

Just found this bug today... was about to open an issue. Thanks for the fix!

@timacdonald
Copy link
Member Author

No troubles!

@StSarc
Copy link

StSarc commented Dec 15, 2023

This breaks the testing though!

I am faking an http response of an URL in a test. This fake is written in a base test class. This is used by multiple other test cases. Some of these test cases implement another fake http response of the same URL.
Calling the fake method second time in these test cases does not work. It gives the response set by the first fake method. It does not overwrite the response with the new fake data. The first fake call is always taking the precedence.

Simplified code to reproduce:

<?php

use Illuminate\Support\Facades\Http;

Route::get('/', function (){
  Http::fake([
    'https://laravel.com' => Http::response('OK', 200)
  ]);

  dump(Http::get('https://laravel.com')->body());

  Http::fake([
    'https://laravel.com' => Http::response('Internal Server Error', 500)
  ]);

  dump(Http::get('https://laravel.com')->body());
});

On the Laravel Playground: https://laravelplayground.com/#/snippets/0f8268bf-57d5-4709-ac54-e24965ec4c22

The same issue is being faced by others as well: https://laracasts.com/discuss/channels/laravel/overwrite-fake-response-in-http-client?page=1&replyId=762080

The solution suggested (swap) - it used to work until this PR got merged. That solution broke with this PR.

The test case (with multiple http calls faked) are run in a single request. Singletons cause an issue in testing because of that.

@timacdonald
Copy link
Member Author

Responded in #49398

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Global HTTP client middleware being forgotten after every queue job
4 participants