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

Apply global middleware also to Saloon requests #15

Merged
merged 6 commits into from
Dec 1, 2023

Conversation

pascalbaljet
Copy link
Contributor

In Laravel 10.14, Global HTTP middleware was introduced (PR #47525). You may use this to globally set a default User Agent. The Middleware array is passed from the HTTP Factory to a new PendingRequest:

https://github.com/laravel/framework/blob/fc8f819bef5b4843baadda393c51320ceeb41a70/src/Illuminate/Http/Client/Factory.php#L396-L404

In the createLaravelPendingRequest() method of HttpSender, a new PendingRequest is instantiated but without the global middleware. This PR fixes that.

One caveat, though: there's no functionality to grab the middleware array from the Factory. So as a workaround, I added a macro. After opening this PR, I'll submit another PR to the Laravel repository to do this more elegantly.

@pascalbaljet
Copy link
Contributor Author

Here's the Laravel PR: laravel/framework#48950

@Sammyjo20
Copy link
Member

Oh wow, this is huge! Thank you so much @pascalbaljet for PR'ing a fix in the Laravel framework and Saloon 🤯 Will this introduce a breaking change in this library because of the minimum requirement of Laravel 10? Or could we remove the requirement for Laravel 9 and I could tag v2.1 of this library? I assume then Composer will automatically keep people on Laravel 9 using 2.0 and then anyone using >10 will go up to your version here/

@Sammyjo20
Copy link
Member

Sammyjo20 commented Nov 8, 2023

Also @pascalbaljet I assume with access to global middleware, things like Http::fake() will work?

@pascalbaljet
Copy link
Contributor Author

pascalbaljet commented Nov 9, 2023

Oh wow, this is huge! Thank you so much @pascalbaljet for PR'ing a fix in the Laravel framework and Saloon 🤯 Will this introduce a breaking change in this library because of the minimum requirement of Laravel 10? Or could we remove the requirement for Laravel 9 and I could tag v2.1 of this library? I assume then Composer will automatically keep people on Laravel 9 using 2.0 and then anyone using >10 will go up to your version here/

The Laravel PR is already merged, so the easiest way would be to require next week's release as a minimum requirement. It's fine to do that in a minor release, Spatie drops support for Laravel and PHP versions in minor releases as well.

Otherwise, you could ditch the macro and check if the method is available. Something like this:

/** @var Factory $httpFactory */
$httpFactory = resolve(Factory::class);

$middleware = method_exists($httpFactory, 'getGlobalMiddleware')
    ? $httpFactory->getGlobalMiddleware()
    : [];

$httpPendingRequest = new HttpPendingRequest($httpFactory, $middleware);

@pascalbaljet
Copy link
Contributor Author

Also @pascalbaljet I assume with access to global middleware, things like Http::fake() will work?

No, I don't think so. In the Factory, the stub callbacks and $preventStrayRequests value is set on a PendingRequest:

https://github.com/laravel/framework/blob/2703f3e85511d8a6eb70752fc5d5e2d9287d0712/src/Illuminate/Http/Client/Factory.php#L440

So when the PendingRequest is instantiated in createLaravelPendingRequest(), the stubs should be passed. That may require another Laravel PR for getStubs() and shouldPreventStrayRequests() methods on the Factory.

@Sammyjo20
Copy link
Member

The Laravel PR is already merged, so the easiest way would be to require next week's release as a minimum requirement.

I agree @pascalbaljet - would you mind including this minimum version in this PR and then also remove the macro please? After that, I'll get this PR merged

@pascalbaljet
Copy link
Contributor Author

@Sammyjo20 I've updated the PR :)

@onlime
Copy link

onlime commented Nov 29, 2023

Hey @Sammyjo20 Are you planning to release a v2.1 with this merged anytime soon? No pressure! I just keep on patching my vendor code over and over again when switching branches and re-installing laravel-http-sender, so would need to fork this project in the meantime.

Please let us know if this PR needs further refinement. Thanks.

@Sammyjo20
Copy link
Member

Hey @onlime @pascalbaljet absolutely! Apologies I've been really busy with work but I'm currently going through issues/PRs so I'll get this one actioned soon :)

Copy link
Member

@Sammyjo20 Sammyjo20 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One tiny question!

src/HttpPendingRequest.php Outdated Show resolved Hide resolved
Copy link
Member

@Sammyjo20 Sammyjo20 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the great PR @pascalbaljet and your help @onlime

@Sammyjo20 Sammyjo20 merged commit 22d0ecb into saloonphp:v2 Dec 1, 2023
10 checks passed
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.

3 participants