chore: throw an exception when no serializer is provided to the controller

[SychO9]

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

• Has the problem that is being solved here been clearly explained?
• If applicable, have various options for solving this problem been considered?
• For core PRs, does this need to be in core, or could it be in an extension?
• Are we willing to maintain this for years / potentially forever?

Confirmed

• Frontend changes: tested on a local Flarum installation.
• Backend changes: tests are green (run composer test).
• Core developer confirmed locally this works as intended.
• Tests have been added, or are not appropriate here.

[askvortsov1]

Seems reasonable to me. Perhaps a unit test would be nice, to ensure that the error message looks like we want it to?

[luceos]

Doesn't container make already throw an error and the lies more with flarum adding json to the response using its internal client?

[SychO9]

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.

[luceos]

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 :)

[SychO9]

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.

[luceos]

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.

[SychO9]

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 👍🏼

[luceos]

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 👴

[davwheat]

I think developer QoL is important, and helping developers to specifically identify what a forum-breaking issues are is a good idea.

Similar things happen when an error occurs in an API controller that is run for the preloaded API document: the forum frontend partially loads, making the error seem like a frontend issue, and the only clue being an obscurely written Javascript console error, or diving into the raw page HTML and inspecting the multi-KB JSON payload for the stringified PHP exception.

We've introduced dev QoL changes before to help developers identify when they have made an error (i.e., look at the Tooltip component:

framework/framework/core/js/src/common/components/Tooltip.tsx

Lines 138 to 166 in 62ffe5f

	// We'll try our best to detect any issues created by devs before they cause any weird effects.
	// Throwing an error will prevent the forum rendering, but will be better at alerting devs to
	// an issue.
	if (typeof children === 'undefined') {
	throw new Error(
	`Tooltip component was provided with no direct child DOM element. Tooltips must contain a single direct DOM node to attach to.`
	);
	}
	if (children.length !== 1) {
	throw new Error(
	`Tooltip component was either passed more than one or no child node.\n\nPlease wrap multiple children in another element, such as a <div> or <span>.`
	);
	}
	const firstChild = children[0];
	if (typeof firstChild !== 'object' || Array.isArray(firstChild) || firstChild === null) {
	throw new Error(
	`Tooltip component was provided with no direct child DOM element. Tooltips must contain a single direct DOM node to attach to.`
	);
	}
	if (typeof firstChild.tag === 'string' && ['#', '[', '<'].includes(firstChild.tag)) {
	throw new Error(
	`Tooltip component with provided with a vnode with tag "${firstChild.tag}". This is not a DOM element, so is not a valid child element. Please wrap this vnode in another element, such as a <div> or <span>.`
	);
	}

), so I don't see how this is any different, personally.