Skip to content

Conversation

@benbjurstrom
Copy link
Contributor

This pull request overrides the resolveRouteBinding method in the HasUuids trait to ensure the given URI segment is a valid UUID before passing it to the database.

This check is necessary when using a Postgres database with the UUID datatype as passing anything other then a valid UUID will throw a QueryException. This check is also useful more generally to avoid unnecessary database queries.

Copy link
Member

@driesvints driesvints left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea 👍

@driesvints
Copy link
Member

We might want to do the same for the ULID trait?

@michaelnabil230
Copy link
Contributor

We might want to do the same for the ULID trait?

I created it
#44995

@taylorotwell
Copy link
Member

I don't think this properly addresses child route bindings.

@taylorotwell taylorotwell marked this pull request as draft November 17, 2022 21:03
@benbjurstrom
Copy link
Contributor Author

It looks like moving the check down to resolveRouteBindingQuery catches the child bindings as well. I updated the pull request. Let me know if you'd like to see any other changes.

@benbjurstrom benbjurstrom marked this pull request as ready for review November 17, 2022 22:06
@taylorotwell
Copy link
Member

taylorotwell commented Nov 17, 2022

Don't you actually need to check and use the $field parameter if it's present?

@benbjurstrom
Copy link
Contributor Author

I'm sure you're right. I'll take another look at it. Thanks!

@benbjurstrom benbjurstrom marked this pull request as draft November 17, 2022 22:37

$this->beforeApplicationDestroyed(function () {
Schema::dropIfExists('users');
Schema::dropIfExists('posts');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the drop method of the table comments in beforeApplicationDestroyed

@benbjurstrom benbjurstrom marked this pull request as ready for review November 18, 2022 20:34
@taylorotwell taylorotwell merged commit 1d18fae into laravel:9.x Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants