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] Allow brick/math 0.11 also #45762

Merged
merged 2 commits into from
Jan 22, 2023
Merged

[9.x] Allow brick/math 0.11 also #45762

merged 2 commits into from
Jan 22, 2023

Conversation

barryvdh
Copy link
Contributor

@barryvdh barryvdh commented Jan 22, 2023

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)

Allows brick/math version 0.11 besides version 0.10
@driesvints
Copy link
Member

Can you update the other brick requirements in other composer.json files as well?

@barryvdh
Copy link
Contributor Author

It doesn't seem to be added to the other packages, but I think it probably should be?
I've added it the the Database and Validation package. cc @timacdonald as this is an addition to #45729

@taylorotwell taylorotwell merged commit 2f62992 into laravel:9.x Jan 22, 2023
@@ -17,6 +17,7 @@
"require": {
"php": "^8.0.2",
"ext-json": "*",
"brick/math": "^0.10.2|^0.11",
Copy link
Member

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.

Copy link
Member

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",
Copy link
Member

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.

Copy link
Member

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.

@timacdonald
Copy link
Member

Thanks @barryvdh! I added these in #45693 but didn't realise I left it in draft, so it has just been sitting there 🙈

@Temepest74
Copy link

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)

Yes it contains. Today I updated from v9.48.0 to v9.49.0, and I got this error from Brick Math:
The given value " 15" does not represent a valid number. {"userId":1,"exception":"[object] (Brick\Math\Exception\NumberFormatException(code: 0): The given value " 15" does not represent a valid number. at /var/www/html/vendor/brick/math/src/BigNumber.php:71)

I got this error inside a FormRequest and to solve this error I had to change

                    'numeric',
                    'between:1, 15',

to

                    'numeric',
                    'between:1,15',

Not a big deal, and idk if the space was a bug to begin with.

@driesvints
Copy link
Member

@Temepest74 I don't think we can consider that a valid use case sorry.

@RikSomers
Copy link

Yes, we had the same error here @Temepest74 . Luckily like with your solution, we had to remove the space and it started working again.

@rolfdenhartog
Copy link

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 brick/math. I've included the most important parts.

The validation from the FormRequest class:

'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 conflicts in composer.json.

If there is anyway I can help, please let me know.

@driesvints
Copy link
Member

We fixed the spacing issue btw. @timacdonald ^

@driesvints
Copy link
Member

@rolfdenhartog could you add a very simple example with rules as well as values that the validator now fails for?

@timacdonald
Copy link
Member

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.

@rolfdenhartog
Copy link

rolfdenhartog commented Feb 4, 2023

@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.

@rolfdenhartog
Copy link

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 🍺

@rolfdenhartog
Copy link

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 9.x branch.

@timacdonald
Copy link
Member

That is great to hear.

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

8 participants