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

Add cast support #2257

Closed
wants to merge 1 commit into from
Closed

Add cast support #2257

wants to merge 1 commit into from

Conversation

Christophvh
Copy link

@Christophvh Christophvh commented May 19, 2021

Fixes #2199 , #1580

@divine
Copy link
Contributor

divine commented May 19, 2021

Hello,

Can you please add a tests?

Thanks!

@sts-ryan-holton
Copy link

@divine @Christophvh I really need this feature merged ASAP, I've created an issue regarding this which is likely related and after manually taking the small if statement from the attached verified commit and manually adding it to my code, I can indeed see that it's casting my field to an integer. Any idea when this can be merged?

@divine
Copy link
Contributor

divine commented May 26, 2022

@sts-ryan-holton honestly, probably never.

@sts-ryan-holton
Copy link

@divine how can I get this into my project? It's just a few lines of code and seems to work for me

@divine
Copy link
Contributor

divine commented May 26, 2022

@divine how can I get this into my project? It's just a few lines of code and seems to work for me

It's just two lines without a test that could break any application.

How you can get into this project? Well, it's currently in stale mode as I can't merge and release PRs that were created back in March.

Thanks!

@Giacomo92
Copy link

Hi @sts-ryan-holton , could you try to add some tests ? You can open a new PR

@hans-thomas
Copy link
Contributor

I guess it is fixed and this issue no longer exists. If it still exists, please let me know how I can produce it.
@Giacomo92 @sts-ryan-holton

@rbruhn
Copy link

rbruhn commented Oct 19, 2023

Ran into this today and still isn't working.
Using v4.0.

Pulling a row of data from a csv file and simply running an updateOrCreate() on the model. The subject_id and subject_expeditionId are still entered into the document as strings.

/**
 * The attributes that should be cast.
 *
 * @var array
 */
protected $casts = [
    'subject_id'           => 'integer',
    'subject_expeditionId' => 'integer',
    'created_at'           => 'datetime',
    'updated_at'           => 'datetime',
];

Handling it by adding mutators in my models for every place I need it. For example:

/**
 * Set subjectId to int.
 */
protected function subjectId(): Attribute
{
    return Attribute::make(
        set: fn (string $value) => (int)$value,
    );
}

@alcaeus
Copy link
Member

alcaeus commented Oct 20, 2023

@rbruhn can you please create a new issue? I just ran a quick test and it worked just fine. A small test case would help us tremendously. You can use some other tests in ModelTest as guidance on how to create them.

I'm closing this pull request as setAttribute handles a bit of special logic (casting for identifiers and access to nested fields), then defers to the default implementation in the Eloquent model, so anything that works in Eloquent by default should also work here just the same.

@alcaeus alcaeus closed this Oct 20, 2023
@rbruhn
Copy link

rbruhn commented Oct 20, 2023

parent::setAttribute() doesn't handle normal casting. If you look closely, it only handles mutators, dates, enums, classes, json, encrypted, and hashed. The code @Christophvh added fixes the problem.

I did a few quick tests of my own. They come back correct, (assertIsInt() and assertIsString()) but still stored in the database incorrectly. I believe the test results are showing the getAttribute but not setting correctly before inserting in the database.

@hans-thomas
Copy link
Contributor

hans-thomas commented Oct 21, 2023

@alcaeus I was checking this problem a couple of days ago. It's really not working. As mentioned in #2393, if you don't cast your values before storing them in the DB, they will be stored as their original data type. To produce this issue, you can test int or bool data type for casting like this:

public function testCastingAttributes(): void {
    Birthday::truncate();
    Birthday::query()->create(['name' => 'Hans','age' => '1']);
    
    self::assertIsBool(Birthday::query()->first()->age); // pass
    self::assertIsBool(DB::collection('birthday')->first()['age']); // fail
}

Don't forget to add age attribute as bool data type in $casting

But with this PR, it's working fine.
If you agree, I will work on this issue and write some tests to ensure everything is okay and all data types casting works fine.

@rbruhn rbruhn mentioned this pull request Oct 24, 2023
@hans-thomas hans-thomas mentioned this pull request Oct 29, 2023
@GromNaN GromNaN reopened this Oct 30, 2023
@GromNaN GromNaN closed this Oct 30, 2023
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.

Add cast to Model class[Feature Request]
8 participants