-
-
Notifications
You must be signed in to change notification settings - Fork 834
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
Run API Client requests through middleware #2783
Conversation
dbf984e
to
df1db76
Compare
Moved the pipe construction into the service provider, this feels a lot cleaner to me. |
@clarkwinkelmann I'm a bit conflicted on how to proceed here with regards to #2800. Just adding the "Start Session" middleware won't fix it, because the session that would get started is different from the one from the original request. We could add One idea I had was allowing the entire "parent" request to be passed in as an optional argument after $actor, and then ApiClient could transfer over certain attributes (like session). Actor could still be provided, but if it wasn't, maybe we could read that from the parent request too? |
Tests are failing due to the default route setting in |
[ci skip] [skip ci]
[ci skip] [skip ci]
[ci skip] [skip ci]
[ci skip] [skip ci]
This makes modification easier.
[ci skip] [skip ci]
084f0b5
to
4cae57b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tested the changes, but here are a few comments based on the code.
I'm not particularly a fan of private
statements as I have said before. I'm sure this change will break multiple of my extensions where I will have to copy the whole file instead of just re-implementing getApiDocument
(when it's private, not only it cannot be called, but re-implementing it doesn't work without also re-implementing the method that calls it)
Couldn't a method chain maybe make for a cleaner client API? Like $client->withParentRequest($request)->withParams($params)->get('discussions.show')
to avoid all the null
parameters?
[ci skip] [skip ci]
Reverted, not sure why I changed that. Although tbh, the entire Content system needs an extensibility review.
Agreed, changed. Much nicer, and clearer to read. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still not tested locally, but it looks really good like this now!
Just one suggestion below.
…essary skipped middleware
[ci skip] [skip ci]
We already have CSRF checks on all our routes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Just one more thing below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love how much cleaner it is now!
Fixes #2474
Changes proposed in this pull request:
Confirmed
composer test
).