refactor: JSON:API #3971
refactor: JSON:API #3971
Conversation
# Conflicts: # extensions/tags/src/TagValidator.php # framework/core/locale/validation.yml # framework/core/src/User/Command/RegisterUserHandler.php
There was a problem hiding this comment.
This is based off an initial reading from core changes, no extensions or implementation has been done.
For my understanding:
- serializers have been replaced with resources, and resources contain not just endpoint instruction, but also field instructions; I really like that
- many of the controllers have been simplified or removed entirely because of this refactor, which is also nice. I do think it will need some getting used to, to easily find where what is declared when looking things up.
- Can we somehow get rid of all the Command/Handlers and move them to the respective controllers where possible or would that better be done in a follow up PR?
Overall this looks good, I will start doing some implementation attempts for the federation extension so that I can fully understand how things work etc.
| public function getId(object $model, \Tobyz\JsonApiServer\Context $context): string | ||
| { | ||
| return '1'; | ||
| } | ||
|
|
||
| public function id(\Tobyz\JsonApiServer\Context $context): ?string | ||
| { | ||
| return '1'; | ||
| } |
There was a problem hiding this comment.
For multi tenant communities, it would be helpful if this value could actually be set. So that if any json is visible to the world, it provides a human readable identifier. An option here would be to take that ID from the config.php if set, or fallback to 1. Unless this is extensible, it feels like a hardcoded value that might byte us in the future..
There was a problem hiding this comment.
That's a good idea, but not really tied to this PR. It was hardcoded as 1 before this aswell in the ForumSerializer.
Let's do that in a separate issue to not go out of scope 👍🏼
Yes, and columns. Filters have been disabled since we have our own driver-based filter implementation (which we can always improve in terms of boilerplate as well).
Keep in mind, you can still create a separate controller the old fashion way, only there are non of the previous parent serializer classes. So the result of it cannot be the serialization of a resource. You can also just use a custom endpoint (see user resource delete avatar).
They are still useful reusable domain logic, for example check the delete avatar custom endpoint in the user resource class. Now that most controllers have been removed, command handlers aren't actually bad to have in terms of boilerplate. An additional improvement to them would be to make them one class rather than requiring two classes imo. |
|
documentation added: flarum/docs#472 |
|
There's two things I'd love to be able to do:
Overall using this is very nice, it reminds me a lot of the filament resources logic. I have had no issue declaring the resources, their endpoints and their schema. So 👍 |
That would mean adding a route to the forum rather than the API, it's quite messy and would require quite a lot of changes. This is an edge case where I would rather you use a normal controller. Does this endpoint require the api resourceat all? if it returns custom json structure that isn't actually of the JSON:API spec for resources, all the more reason to use a normal controller instead. If it does return valid json for a resource, then checkout the
You can use the |
| (new Extend\ApiController(ListPostsController::class)) | ||
| ->addInclude(['flags', 'flags.user']), | ||
| (new Extend\ApiResource(Resource\DiscussionResource::class)) | ||
| ->endpoint(Endpoint\Show::class, function (Endpoint\Show $endpoint) { |
There was a problem hiding this comment.
Maybe in the docs it would be nice to have some guidelines for when to use default includes vs not.
There was a problem hiding this comment.
docs actually recommend avoiding default includes whenever possible. Otherwise endpoints become unnecessarily bloated which easily harms performance. I have noticed for example some extensions adding default includes to endpoint A in order to display a relation in frontend page X, only for frontend page B to now unnecessarily have an api call that includes a relation it does not need (which often leads to more issues when you consider lack of eager loading).
With multiple extensions, it all builds up to a pretty bad effect.
There was a problem hiding this comment.
Yeah this makes sense. I meant more like "default includes are almost always bad, here's why, here's what might qualify as an exception but seriously please don't"
| return Flag::class; | ||
| } | ||
|
|
||
| public function query(Context $context): object |
There was a problem hiding this comment.
We need to override this to add the group by?
There was a problem hiding this comment.
yes, would be better to have something like this wouldn't it
Endpoint\Index::make()
->constrain(fn ($query) => $query->groupBy('post_id')),| public function __invoke(): array | ||
| { | ||
| return [ | ||
| Schema\Boolean::make('canTag') |
There was a problem hiding this comment.
I wonder if it would make sense to have a Schema\Permission
There was a problem hiding this comment.
Why not, the best part of this library is how much we can add on top that adds to the developer experience.
There was a problem hiding this comment.
Amazing job, this new API is such a great transformation!
Left some more comments. Some of these are clarifications / stuff I've gotten a bit rusty one, and some more might be better noted and saved for a future feature.
Thanks for putting this together, it's really exciting to see!
| }), | ||
|
|
||
| Schema\DateTime::make('markedAllAsReadAt') | ||
| ->visible(fn (User $user, Context $context) => ($context->collection instanceof self || $context->collection instanceof ForumResource) && $context->getActor()->id === $user->id) |
There was a problem hiding this comment.
Can we factor this logic out into a helper function?
|
|
||
| class PopulateWithActor implements MiddlewareInterface | ||
| { | ||
| public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface |
There was a problem hiding this comment.
This (and iirc translation locales) are one of the few global bits of per-request state that make using octane difficult. Maybe in another pr, it would make sense to store all them in some centralized place so it's easier to keep track of and eventually eliminate?
# Conflicts: # extensions/likes/tests/integration/api/ListPostsTest.php # extensions/mentions/tests/integration/api/ListPostsTest.php # framework/core/src/Api/routes.php # framework/core/src/Discussion/UserState.php # framework/core/src/Extend/Settings.php # framework/core/tests/integration/extenders/ApiControllerTest.php # framework/core/tests/integration/extenders/ModelTest.php # framework/core/tests/integration/policy/DiscussionPolicyTest.php
|
Will merge after the DB PRs are merged. |
Closes #3964
Changes proposed in this pull request:
This is a difficult review, I couldn't really find a way to split this since it can only be all done together, I would recommend looking at some specific resources, for example, comparing the main branch Discussion model JSON API with this one. The controllers and serializer class and command handlers vs the DiscussionResource class.
Reading the issue itself will help you a lot in getting context about the changes here. Other than that, some key things to think about:
Some notable changes
validation.ymlfile has been updated.SortColumnon the DiscussionResource can receive anascendingAliasanddescendngAliaswhich can them easily be accessed in the content classes, this also means extensions can add to the sortmap by just the sorts they add themselves in the same manner.Todo