Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[9.x] Validate uuid before route binding query #44945

Merged
merged 8 commits into from Nov 21, 2022
Merged

[9.x] Validate uuid before route binding query #44945

merged 8 commits into from Nov 21, 2022

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
$table->integer('user_id');
$table->timestamps();
});

$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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants