-
Notifications
You must be signed in to change notification settings - Fork 780
Add support for UUID validation using ramsey/uuid to support all UUID… #1542
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
base: 2.4
Are you sure you want to change the base?
Conversation
… versions As suggested in Respect#1471 Defaults to the original behavior if ramsey/uuid is not installed or explicitly disabled.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 2.4 #1542 +/- ##
======================================
Coverage ? 93.91%
Complexity ? 462
======================================
Files ? 99
Lines ? 1265
Branches ? 0
======================================
Hits ? 1188
Misses ? 77
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
{ | ||
if ($useRamseyUuid && !$this->ramseyUuidIsLoaded()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's really awesome, thank you so much! I would say though, we could aways use RamseyUuid for this rule, we just need to require it to be install. In that case, this branch should be based on main
, to be released with the next major version. What do you say?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying not to introduce any BC breaks and allow RamseyUuid to be optional.
But if you are good with making RamseyUuid required, it would clean up the code loads.
I was targeting this at 2.4 so it could be used quicker, has I dont know how far version 3.0 is from release.
I will make the chages to make RamseyUuid a required and clean up the code 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, just looking at the main
version, I will create a new version for the main
and leave the PR here if you want to merge this into 2.4
… versions
As suggested in #1471
Defaults to the original behavior if ramsey/uuid is not installed or explicitly disabled.