use decimal precision for model casting

[pxpm]

Hey @jasonmccreary hope to find you well. I maybe missing something obvious here, so forgive me if that's the case. 🙏

The idea I got from the docs is that the definition for a decimal column is decimal:precision,scale. https://blueprint.laravelshift.com/docs/model-data-types/

On Laravel Docs for casts, the allowed configuration is precision https://laravel.com/docs/11.x/eloquent-mutators#attribute-casting.

From my understanding the attributes array is: [0 => precision, 1 => scale], so I assume we are passing the scale to the casts instead of the precision.

Let me know what you think. 👍

Cheers

Edit: tests are failing probably due to asserting against the "scale" value and not the "precision" value, if you think this approach is correct I will update the tests accordingly.

[jasonmccreary]

Thanks. This PR is a bit noisy with all the links and failing tests. If you believe this is a bug and your code change is correct, then you would also need to either update the exiting fixtures so the tests pass or add a new test to demonstrate the error.

[pxpm]

Hey @jasonmccreary thanks for taking time looking at this issue, and sorry if the links create too much noise, I just wanted to point out from where I got the information I am working with 🙏

I've just updated the tests to reflect what I think it's the correct approach.

In tests, the decimal column is defined as: base_pay:10,2, here: https://github.com/laravel-shift/blueprint/blob/4485f6b3491eaeca6c9d59114b5c1dd349f914d0/tests/fixtures/drafts/model-with-ulid-id.yaml#L5 and https://github.com/laravel-shift/blueprint/blob/4485f6b3491eaeca6c9d59114b5c1dd349f914d0/tests/fixtures/drafts/model-with-uuid-id.yaml#L5

From my understanding, that definition means: 10 precision, 2 scale. When parsing the definition to an array, we get an array of: [0 => 10, 1 => 2]. Before this PR blueprint would use the 1 key in the array to write the model decimal casts, and that would incorrectly set the precision in the cast to be 2 instead of the expected 10.

The actual change in the model is like bellow:

protected $casts = [
-    'base_pay' => 'decimal:2',
+    'base_pay' => 'decimal:10',
];

I don't know why macos tests are failing, but I am 100% sure that there is nothing in this PR that would pass on windows/linux and fail on macos.

Let me know if I can help you with something else. 👍

Cheers

[jasonmccreary]

Thanks again. However, I am not convinced this is a bug. This YAML is based off the decimal column format, not Eloquent casts.

So I believe it is behaving accordingly. If not, I'd like to think someone would have complained by now. If you believe it is an issue. Please open an issue with the draft YAML and the incorrect generated code.

[pxpm]

Hey @jasonmccreary , this was unexpected 😞

There is nothing wrong with the yaml file nor the generation of the database columns. The issue here is that instead of using "total (precision)", blueprint is adding the "places (scale)" when generating the model class with the casts.

Following your example (the one from laravel docs you pointed), $table->decimal('amount', total: 8, places: 2);, what value would you use when building the model casts ?

protected $casts = [
    'amount' => 'decimal:8',
];

// or

protected $casts = [
    'amount' => 'decimal:2',
];

I believe the correct one is decimal:8, but without this PR blueprint is generating decimal:2.

We already have the draft yamls in the tests, I pointed to two different yaml files in the tests and we didn't need to touch those files as they are working as expected when generating the columns. Blueprint correctly interprets the decimal definition, but when generating the model casts it uses the wrong value.

Do you really think it's necessary to open a separate issue ? You can test with this yaml:

models:
  User:
    amount: decimal:10,2

Risking repeating myself, this will generate the following cast in the model:

protected $casts = [
    'amount' => 'decimal:2',
];

I expect it to generate:

protected $casts = [
    'amount' => 'decimal:10',
];

Please let me know what I need to do to move this issue further. 👍

[jasonmccreary]

Sorry. It's not personal. I am simply not convinced Blueprint is behaving incorrectly. There is no reported issue other than your suggested PR. I'm relying (maybe naively) on the fact that I and hundreds of others have used Blueprint and not noticed a problem with the generated code for decimal casts.

In the end, I think the code is correct and maybe the wording (or interpretation of the wording) is incorrect. That is to say the decimal:total,places syntax translates to an Eloquent cast of decimal:places. Which is correct. I think the docs for Eloquent may be using "precision" in a different context.

Again, if you can demonstrate an actual bug, as in generated code from Blueprint is resulting in an incorrect Eloquent casted value, I will gladly reconsider.