ApiSerializer Extender

[SychO9]

Part A of #1624
Part of #1891

Changes proposed in this pull request:

• Deprecate the Serializing event.
• Deprecate the GetApiRelationshipEvent event.
• Add attributes extenders.
• Add relationships extenders.
• Add Tests for all the new extenders.

Reviewers should focus on:

• Are the tests on point.

Confirmed

• Backend changes: tests are green (run composer test).

Bundled extensions to update:

• flarum/likes: Use new extenders likes#25
• flarum/mentions: Use new extenders mentions#58
• flarum/tags: Use new extenders tags#102
• flarum/flags: Use new extenders flags#34
• flarum/suspend: Use new extenders suspend#31
• flarum/approval: Use new extenders approval#23
• flarum/sticky: Use new extenders sticky#22
• flarum/subscriptions: Use new extenders subscriptions#36
• flarum/lock: Use new extenders lock#26
• flarum/pusher: Use new extenders pusher#30

[SychO9]

I think I'll split this into two PRs, one for adding the ApiSerializer Extender (this one) and one for the ApiController Extender.

[clarkwinkelmann]

Just looking at the code looks good.

I just find it annoying sometimes to not have access to the actor in some contexts, but that was already from before this PR.

My only worry is that by using callbacks instead of event class, it makes it harder to add properties like $actor or $request later on without introducing a bunch of breaking changes...

[SychO9]

wouldn't you still be able to do that if you used invokable classes ?

EDIT: nvm, I don't think I fully understand what the difference is between using an event class and a callable when it comes to using the actor, seems like you'd still be able to do $serializer->getActor() just like in the event listeners.

[askvortsov1]

I think Clark is referring to the old approach of bundling all properties passed to the callable under a single POPO event object. That's a fair point, but are there arguments you're expecting to need to pass in, in the near future? The way I see it, the only thing that could prompt a change like that is if we switched to https://github.com/tobyzerner/json-api-server, but that's a long way away and would likely break other things too

[clarkwinkelmann]

seems like you'd still be able to do $serializer->getActor() just like in the event listeners.

True, those additional parameters could be accessed from objects that are passed to the callback sometimes, so there wouldn't be an issue with adding more parameters to the callback here.

are there arguments you're expecting to need to pass in, in the near future?

Except from actor/request for some contexts where none of the arguments allow to access them, I'm not seeing anything particular. I'm just seeing this as a potential issue for the future. Adding properties to an event class is easy, removing one only affects those that were using it. Removing properties from a callback will break every implementation that uses one of the arguments that follows.

Luckily PHP allows callback to accept less arguments than passed. Otherwise this would be a guaranteed breaking change anytime we add a new parameter, even if nobody wanted to use it.

[SychO9]

Ah I see, that's a good point yeah 🤔

[askvortsov1]

We'll merge this a bit later in case @franzliedke has any comments / suggestions

[SychO9]

a couple of things I thought about while updating bundled extensions.

[SychO9]

I noticed that most of the time when adding a single attribute through the attribute method of the extender, the first parameter being $attributes is rarely the one needed, so it's useless most of the time, do we want to push it to the end, or would we rather keep it consistent with the callback signature of the mutate method ?

[askvortsov1]

I think consistency is better here; that being said, we could push it to the end for both mutate and attribute.