flarum
chore: throw an exception when no serializer is provided to the controller
Changes proposed in this pull request: When developing and you forget to provide a serializer, the forum crashes but does not indicate what the problem is, even in the flarum logs. We 100% expect a serializer anyway, so this PR throws an explicit exception when non is provided.
Necessity
- [x] Has the problem that is being solved here been clearly explained?
- [x] If applicable, have various options for solving this problem been considered?
- [x] For core PRs, does this need to be in core, or could it be in an extension?
- [x] Are we willing to maintain this for years / potentially forever?
Confirmed
- [x] Frontend changes: tested on a local Flarum installation.
- [x] Backend changes: tests are green (run
composer test). - [x] Core developer confirmed locally this works as intended.
- [ ] Tests have been added, or are not appropriate here.
Doesn't container make already throw an error and the lies more with flarum adding json to the response using its internal client?
Doesn't container make already throw an error
It's very hard to immediately tell what the problem actually is from the error thrown from that. This is more sane/clear.
and the lies more with flarum adding json to the response using its internal client?
Sorry I don't follow, the internal client still catches errors (including this exception) and renders JSON.
The reason there's confusion is because the Container throws an error and the API does not handle that error properly. IMO on the frontend you see this weird error that the output is already generated while using the internal api client for the preloaded document. It would make more sense to me to capture the Container Exception thrown and cast it into a more sensible one that can be understood by anyone, like "Flarum tried resolving a class but failed: <ClassName>". In addition the internal api client should throw exceptions when failing with 5xx status and process them better (if we aren't already).
Fixing just a missing serializer, in my opinion, seems like a way to fix one isolated problem where any binding could fail in any part of Flarum (but presumably in an extension).
I hope this makes more sense :)
This isn't about the internal API though, that's a separate issue because with or without this change you still won't understand what the problem is frontend-wise.
Even if we catch container exceptions and instead throw an error with "Flarum tried resolving a class but failed: <ClassName>" that still doesn't make much of a difference from ReflectionException: Class <ClassName> does not exist in, I don't think we need to do anything about binding resolution exceptions.
This is merely to be straightforward about this one case, which is that the serializer might not be specified before we try to resolve it.
I still don't think this is the right solution. This really seems like a QoL improvement for one specific part of Flarum for developers which they should be easy to identify while developing. Even if we agreed to merge this, it doesn't check whether the class exists so if the developer deleted it you would run into the container exception anyhow.
To me, this feels like an unnecessary feature that bloats the framework. An exception is already thrown when the property isn't set, the container throws an error when it cannot be resolved, simply making it clearer seems unneeded as the trace should also show that.
In my honest opinion, I would hate to introduce this change when the maintainer can identify the root cause quite easily by inspecting the API endpoint in debug mode.
I still don't think this is the right solution. This really seems like a QoL improvement for one specific part of Flarum for developers
I don't see what the harm in that is :/. All we are doing here is making sure that a nullable string property is not actually null when passing it to a method expecting a string. We're making sure what we're using is how we expect it.
The same could be said about when we ensure that a serializer is only used on its actual model, a dev could realize that without us checking, but it's saner to do so. https://github.com/flarum/framework/blob/main/framework/core/src/Api/Serializer/BasicDiscussionSerializer.php#L39-L45
I guess we agree to disagree 👍🏼
It's okay to agree to disagree, that's why we're a team. I do think we should wait for the input of the others @flarum/core before making a final decision and my apologies for being such a stubborn old man 👴