-
Notifications
You must be signed in to change notification settings - Fork 11.2k
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] Allow brick/math 0.11 also #45762
Conversation
Allows brick/math version 0.11 besides version 0.10
Can you update the other brick requirements in other composer.json files as well? |
It doesn't seem to be added to the other packages, but I think it probably should be? |
@@ -17,6 +17,7 @@ | |||
"require": { | |||
"php": "^8.0.2", | |||
"ext-json": "*", | |||
"brick/math": "^0.10.2|^0.11", |
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 don't think this should have been added here. Only as a suggest.
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.
This would have been a breaking change if it was only a suggest. Anyone currently using the decimal
model cast would have had issues on update.
@@ -16,6 +16,7 @@ | |||
"require": { | |||
"php": "^8.0.2", | |||
"ext-json": "*", | |||
"brick/math": "^0.10.2|^0.11", |
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 don't think this should have been added here. Only as a suggest.
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.
This would have been a breaking change if it was only a suggest. Anyone currently using the multiple_of
validation rule would have had issues on update.
Yes it contains. Today I updated from v9.48.0 to v9.49.0, and I got this error from Brick Math: I got this error inside a FormRequest and to solve this error I had to change
to
Not a big deal, and idk if the space was a bug to begin with. |
@Temepest74 I don't think we can consider that a valid use case sorry. |
Yes, we had the same error here @Temepest74 . Luckily like with your solution, we had to remove the space and it started working again. |
Maybe I've got a valid use case. I ahve a test that is validating the value of a field, which should be a number of course. But the test is failing since the update of The validation from the 'year_of_birth' => [
'required',
'numeric',
'min:'.$yearOfBirthMin,
'max:'.$yearOfBirthMax,
], The data that's sent using a POST request: ['year_of_birth' => ''] I could remove the test, but that's not what I want. For now I've configured If there is anyway I can help, please let me know. |
We fixed the spacing issue btw. @timacdonald ^ |
@rolfdenhartog could you add a very simple example with rules as well as values that the validator now fails for? |
This should have been fixed with #45912, however it has not been tagged. If you can provide a reproduction case that still fails after that fix, I'll dig into it and get it sorted. |
@driesvints @timacdonald Okay! Please give some time to provide as much as possible info. #kids 😬 I will also try to give some code to reproduce. |
My apologies! I had a very small bug in my code. Which was visible thanks to the update. I owe you guys a beer in Australia or Belgium. Australia will probably take a while, but in two months I'll be in Belgium 🍺 |
For your information: The application I'm maintaining contains almost 700 (feature) tests. I had to fix a few tests and everything is 🟢 again. I've tested it with the latest version 9 release and also the |
That is great to hear. |
Allows brick/math version 0.11 besides version 0.10
See https://github.com/brick/math/releases/tag/0.11.0
Doesn't seem to contain breaking changes for Laravel.
0.10 is required for uuid, but will also allow 0.11 later: ramsey/uuid#488 (if merged)