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 cast issue #2651

Closed
rbruhn opened this issue Oct 24, 2023 · 9 comments
Closed

Fix cast issue #2651

rbruhn opened this issue Oct 24, 2023 · 9 comments

Comments

@rbruhn
Copy link

rbruhn commented Oct 24, 2023

As reported in #2257 , #2199 , #1580 there is an issue with casting and saving to the database. @alcaeus asked for a test example in a new issue. A sample test was provided by @hans-thomas but I figured I would add one here that's more inline with current tests.
The pull request from #2257 works.

First, change User model to make age and integer. This should not effect any other tests as an integer is used in those tests.

protected $casts            = [
    'birthday' => 'datetime',
    'entry.date' => 'datetime',
    'member_status' => MemberStatus::class,
    'age' => 'int'
];

In ModelTest, add the following test. Here we set age as string. It will fail because the cast type is not being saved to the database.

public function testCasting(): void
{
    $user        = new User();
    $user->name  = 'John Doe';
    $user->title = 'admin';
    $user->age   = '35';

    $user->save();

    $this->assertTrue($user->exists);
    $this->assertEquals(1, User::count());

    $raw = $user->getAttributes();
    $this->assertIsInt($raw['age']);
}
@hans-thomas
Copy link
Contributor

To merge #2257 PR, I think we might need to override some methods used in castAttribute method and writing tests to cover every data type declared in castAttribute method.

@rbruhn
Copy link
Author

rbruhn commented Oct 25, 2023

@hans-thomas Or, if the parent::setAttribute is not casting (which it's not) perhaps it's the wrong method to extend.

@hans-thomas
Copy link
Contributor

The setAttribute method casts attributes on retrieving from DB. Because of that, attributes were not cast in creating or updating actions. That's why we need this PR #2257. However, as I said, we need to write tests to cover all supported data types.

@rbruhn
Copy link
Author

rbruhn commented Oct 25, 2023

@hans-thomas I did some reading. Learn something new everyday. Some info states casts are used used in saving/updating. Some info states the opposite. I know this: When testing an eloquent model, castAttribute() is never called when saving or updating.

This changes my mind on this issue. If Laravel doesn't cast when saving/updating to the database, neither should this package. It should be left up to the user to use Attribute (which I originally did to solve this issue).

Since I opened this issue, I'm now closing it. If someone wants this functionality changed, they can open a new issue.

@rbruhn rbruhn closed this as completed Oct 25, 2023
@hans-thomas
Copy link
Contributor

hans-thomas commented Oct 26, 2023

@rbruhn Laravel doesn't support schemaless databases but the purpose of this package is to add a schemaless database (MongoDB) support to Laravel. So, that's why we need to override some methods or add new methods.

I guess this casting is done in PDO or database layer in SQL-based databases because we determined the column data type before any saving/updating actions.

It should be left up to the user to use Attribute (which I originally did to solve this issue).

I don't disagree with you, but this problem doesn't have to exist to solve.

@apeisa
Copy link
Contributor

apeisa commented Oct 27, 2023

I recently hit to this problem. Something to do also with upgrading to Laravel 10 I believe. They dropped the separate $dates property and date casting is now done in $casts property. I have of course made the needed changes on my codebase, but still issues. For some reason in some occasions date casting doesn't seem to work with mongo.

Here is example of the change I needed to do because of this (it used to work before update):
image

@hans-thomas are you working for a PR for casting? All these issues and PRs seems to be closed, although there clearly is some problems.

Strange thing is that in some parts of my codebase date casting seems to be working normally.

@coreycoburn
Copy link

coreycoburn commented Oct 27, 2023

I am experiencing this same issue.

I am trying to mutate/cast the attribute when storing in the database. Although I'd rather use some of the built-in casts, I did find that creating a custom cast did solve the use-case for me.

This didn't work for me (just like the OP stated:

protected $casts = [
    'age' => 'int',
];

However, this DID work, making a custom reusable integer cast:

// MyModel.php
protected $casts = [
    'age' => IntegerCast::class,
];

// IntegerCast.php
<?php

namespace App\Casts;

use Illuminate\Contracts\Database\Eloquent\CastsInboundAttributes;

class IntegerCast implements CastsInboundAttributes
{
    public function set($model, string $key, mixed $value, array $attributes): int
    {
        return (int) $value;
    }
}

Or, for a one-off cast, this worked as well for me:

protected function age(): Attribute
{
    return Attribute::make(set: static fn (string $age): int => (int) $age);
}

@GromNaN GromNaN reopened this Oct 27, 2023
@hans-thomas
Copy link
Contributor

@apeisa I will check this issue ASAP but if you find a use case that breaks date casting, it can help a lot to fix this issue faster.

Strange thing is that in some parts of my codebase date casting seems to be working normally.

Can you check that when it's working fine? On creating/updating or retrieving? I guess it's working correctly on retrieving.

@hans-thomas
Copy link
Contributor

@coreycoburn class casting is done by default in setAttribute. The problem is casting an attribute to a native type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants