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

Fixed handler stack #112

Merged
merged 1 commit into from
Nov 23, 2022
Merged

Fixed handler stack #112

merged 1 commit into from
Nov 23, 2022

Conversation

Sammyjo20
Copy link
Collaborator

@Sammyjo20 Sammyjo20 commented Nov 23, 2022

@Gummibeer I think you mentioned that Guzzle wasn't throwing exceptions on error requests, you were right - my old way of defining the handler stack meant the default middleware wasn't being applied. I've moved the logic to use the same as v1, and it works again now!

Copy link
Collaborator Author

@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.

Self review

@Sammyjo20 Sammyjo20 marked this pull request as ready for review November 23, 2022 22:30
@Sammyjo20 Sammyjo20 merged commit 9845216 into v2 Nov 23, 2022
@Gummibeer
Copy link
Contributor

Not sure if this is fixed now - my problem was that I had a request that returned a 401 response, I got the exception (from my sdk response) but the fixture file wasn't created. So the test would do a real request every single time and needs a correct access token. Which isn't the sense of fixtures and wasn't like that in v1.

@Sammyjo20
Copy link
Collaborator Author

This is what I mentioned in issue #64 - I think we need to have a middleware priority or the ability to add a middleware to the top of the stack - like the fixture recording middleware, so that it can still record the response and then throw the exception.

@Gummibeer
Copy link
Contributor

Sounds good - like request middlewares in Laravel.

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.

None yet

2 participants