[12.x] Return unprocessable response when parameter is not a valid UniqueStringId

[cosmastech]

This changes the behavior of the HasUniqueStringIds::handleInvalidUniqueId() method so that it returns a semantically correct HTTP response that can help callers determine the root cause of a failing response.

If I pass an incorrectly shaped UUID (1234-th!s-is-not-A-uuid-#^@b) to a route that expects a UUID, returning a 404 is misleading. While it is true the model probably does not exist in the database and therefore is not found, the reality is that the request is failing validation. It is easy to assume that the response returned is the result of a DB query. (For instance, I tried to optimize routes by skipping route model binding because I didn't understand that the database was NOT being queried in this case).

When HasUniqueStringIds::isValidUniqueId() indicates the key invalid, we throw a new custom exception InvalidIdFormatException. It has been added to the Handler::$internalDontReport array, and when raised in the application during an HTTP request, gets converted to a 422 response.

I targeted master because I believe this behavior change will break tests for users and therefore is not appropriate for a patch release.

---

Alternatives

• Modify the ModelNotFoundException. I'd be happy to pivot back to this if it makes more sense for the framework. It seems very muddy, but the new InvalidIdFormatException duplicates much of what exists in that class.
• Throw a ValidationException instead. Something like ValidationException::withMessages([$field => '???']). I'm not sure what the message should be in that case.

[shaedrich]

You could use \Symfony\Component\HttpFoundation\Request::HTTP_NOT_FOUND here

Suggested change

	return $this->status === 404;
	return $this->status === Request::HTTP_NOT_FOUND;

[shaedrich]

Wouldn't it make sense to extend the exception from \Symfony\Component\HttpKernel\Exception\HttpException then?

[shaedrich]

You could use \Symfony\Component\HttpFoundation\Request::HTTP_UNPROCESSABLE_ENTITY here

Suggested change

	$e instanceof InvalidIdFormatException => new HttpException(422, $e->getMessage(), $e),
	return $this->status === Request::HTTP_UNPROCESSABLE_ENTITY;

[shaedrich]

This can be simplified by omitting the function call and comparing it to an empty scalar value:

Suggested change

	if (count($this->ids) > 0) {
	if ($this->ids !== []) {

[shaedrich]

If you type-hint $ids, you can simply do:

Suggested change

	public function setModel($model, $ids = [], ?string $field = null)
	{
	$this->model = $model;
	$this->ids = Arr::wrap($ids);
	public function setModel($model, array|int|string $ids = [], ?string $field = null)
	{
	$this->model = $model;
	$this->ids = (array) $ids;

[shaedrich]

Wouldn't it make sense to extend the exception from \Symfony\Component\HttpKernel\Exception\HttpException then?

Suggested change

	/**
	* @template TModel of \Illuminate\Database\Eloquent\Model
	*/
	class InvalidIdFormatException extends \RuntimeException
	use Symfony\Component\HttpKernel\Exception\HttpException;
	/**
	* @template TModel of \Illuminate\Database\Eloquent\Model
	*/
	class InvalidIdFormatException extends HttpException

[taylorotwell]

Thanks for your pull request to Laravel!

Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include.

If applicable, please consider releasing your code as a package so that the community can still take advantage of your contributions!