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

Fix casting issue #2653

Merged
merged 29 commits into from Oct 30, 2023
Merged

Fix casting issue #2653

merged 29 commits into from Oct 30, 2023

Conversation

hans-thomas
Copy link
Contributor

Hello friends,
If you had a look at this repository, you would find that one of the most reported issues is failing to cast attributes on creating/saving data on the database. There are some issues that have been opened since 2018: #1580, #2199, #2257, #2393, #2651.
I applied #2257 PR and specifically wrote tests for each data type that might had an impact.
In this PR, I had to override asDecimal and fromJson methods from the Illuminate\Database\Eloquent\Concerns\HasAttributes trait.

@codecov-commenter

This comment was marked as off-topic.

@hans-thomas hans-thomas marked this pull request as ready for review October 29, 2023 12:35
Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for reviewing these issues and proposing a fix. I have a few remarks, and I still need to test your branch. But overall it looks great!
Big up for all the tests.

src/Eloquent/Model.php Show resolved Hide resolved
src/Eloquent/Model.php Show resolved Hide resolved
tests/Casts/BooleanTest.php Outdated Show resolved Hide resolved
tests/Casts/BooleanTest.php Outdated Show resolved Hide resolved
tests/Casts/CollectionTest.php Outdated Show resolved Hide resolved
tests/Casts/StringTest.php Outdated Show resolved Hide resolved
Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think there is some error cases that could be added? Like data that can’t be casted.

tests/Models/Casting.php Show resolved Hide resolved
protected $casts = [
'intNumber' => 'int',
'floatNumber' => 'float',
'decimalNumber' => 'decimal:2',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a case with decimal without the prevision part?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in the #2146 issue, an error will be thrown.

tests/Casts/CollectionTest.php Outdated Show resolved Hide resolved
tests/Casts/IntegerTest.php Show resolved Hide resolved
tests/Casts/JsonTest.php Show resolved Hide resolved
tests/Casts/DateTest.php Show resolved Hide resolved
@hans-thomas
Copy link
Contributor Author

Do you think there is some error cases that could be added? Like data that can’t be casted.

I tried my best to cover any possible failure in casting values and nothing new came to my mind.

Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@GromNaN GromNaN changed the base branch from 4.0 to 4.1 October 30, 2023 19:08
@GromNaN GromNaN merged commit bc209f7 into mongodb:4.1 Oct 30, 2023
12 of 13 checks passed
@GromNaN
Copy link
Member

GromNaN commented Oct 30, 2023

Thank you @hans-thomas. I merged into 4.1 because the behavior changes and some users may rely on the current behavior.

@apeisa
Copy link
Contributor

apeisa commented Oct 31, 2023

Thanks @hans-thomas for this! It works, but not in all cases. When using date casts, you can also define the format in the string format, like this (docs):

   protected $casts = [
        'mydate' => 'datetime:j.n.Y H:i',
    ];

When using cast like that, it doesn't work. Without formatting set, it does work as expected:

   protected $casts = [
        'mydate' => 'datetime',
    ];

@hans-thomas
Copy link
Contributor Author

Hello @apeisa, I had some tests on datetime cast with custom formatting, It's not from my changes because obviously, I did not change a thing on date casting. Please open a new issue and I will try to fix that. Thank you for your feedback.

@apeisa
Copy link
Contributor

apeisa commented Oct 31, 2023

Sure thing, here you are #2655

@jmikola
Copy link
Member

jmikola commented Nov 1, 2023

I merged into 4.1 because the behavior changes and some users may rely on the current behavior.

@GromNaN: Did you create a PHPORM issue to track this? If not, is it worth doing so for generating release notes down the line, or is that not necessary for this project?

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

5 participants