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

Run API Client requests through middleware #2783

Merged
merged 27 commits into from
May 10, 2021
Merged

Conversation

askvortsov1
Copy link
Member

@askvortsov1 askvortsov1 commented Apr 12, 2021

Fixes #2474

Changes proposed in this pull request:

  • Add integration tests for login and registration; realized that this change might break them and they are currently very missing.
  • API client now has a fluent API, and sends via request methods using a path.

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).

src/Api/Client.php Outdated Show resolved Hide resolved
@askvortsov1
Copy link
Member Author

Moved the pipe construction into the service provider, this feels a lot cleaner to me.

@askvortsov1
Copy link
Member Author

@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 session as an argument, but that would require a lot of code changes, and as you mentioned, wouldn't scale well to other changes.

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?

@askvortsov1
Copy link
Member Author

Tests are failing due to the default route setting in DefaultRouteTest

Copy link
Member

@clarkwinkelmann clarkwinkelmann left a 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?

src/Api/Client.php Show resolved Hide resolved
src/Api/Middleware/ResolveRouteFromName.php Outdated Show resolved Hide resolved
@askvortsov1
Copy link
Member Author

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)

Reverted, not sure why I changed that. Although tbh, the entire Content system needs an extensibility review.

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?

Agreed, changed. Much nicer, and clearer to read.

Copy link
Member

@clarkwinkelmann clarkwinkelmann left a 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.

src/Api/Client.php Outdated Show resolved Hide resolved
src/Api/Client.php Outdated Show resolved Hide resolved
src/Forum/Content/Discussion.php Outdated Show resolved Hide resolved
src/Api/Client.php Show resolved Hide resolved
Copy link
Member

@clarkwinkelmann clarkwinkelmann left a 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.

src/Api/Client.php Outdated Show resolved Hide resolved
Copy link
Member

@SychO9 SychO9 left a 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!

@askvortsov1 askvortsov1 merged commit 104a31b into master May 10, 2021
@askvortsov1 askvortsov1 deleted the as/api-client-middleware branch May 10, 2021 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants