Laravel 7 native casts

[atymic]

• Added or updated tests
• Added or updated the README.md
• Detailed changes in the CHANGELOG.md unreleased section

WIP PR for implementing native eloquent casting.

Current issues:

• Currently, we let laravel do native casting, then we apply the enum cast after.
  Ie. '6' cast by laravel -> 6 cast by us -> EnumInstance

With the new native casts, this isn't possible. I'm thinking we'll need to pass an additional attribute to the constructor to apply a native cast if required.
Kind of a bit messy, but not sure if there's a better way?
Fixed in the latest commit :)

Also, it seems that natively cast attributes are not used in the toArray() which is a breaking change, so we need to see if we can return the object.

Edit: also need some way of only running this test on L7 in CI. Should be easy to do via ENV :)

[BenSampo]

Worth taking a look at this PR for this feature:
laravel/framework#32129

[atymic]

I'll revisit this shortly :)

[BenSampo]

Another relevant PR to to the framework:
laravel/framework#32225

[cerbero90]

Thanks for this proposal @atymic, this feature would be great to have!

Do you think it is possible to:

• let Enum implement Illuminate\Contracts\Database\Eloquent\Castable
• return EnumCast in castUsing()

Introducing this feature may imply that a major version bump is needed for this package as custom casts are only supported in Laravel 7.

Previous Laravel applications working with versions ^1.0 of this package would fail as they wouldn't recognise the new interface.

Dependencies in composer.json and builds in .travis.yml should be updated accordingly too, or am I wrong @BenSampo?

[atymic]

@cerbero90

Yep, I agree it's worth a version bump for this feature.
@BenSampo i've updated the PR to use the Castable interface.

I've also added a way to do native casting (by adding a protected static property on the enum, with the native cast type)

Enum ends up looking like so:

final class UserType extends Enum
{
    protected static $nativeType = 'integer';

    const Administrator = 0;
    const Moderator = 1;
    const Subscriber = 2;
    const SuperAdministrator = 3;
}

and therefore, model can just be:

class NativeCastModel extends Model
{
    protected $casts = [
        'user_type' => UserType::class,
    ];

    protected $fillable = [
        'user_type',
    ];
}

[cerbero90]

Thanks for your work @atymic and for raising this issue in all my favourite Laravel packages (cknow/laravel-money#48) 😄

These are just my two cents and feel free to correct me, but would native casting be avoidable if the $strict flag in hasValue() defaulted to false?

This might be part of the breaking changes in the major version bump if @BenSampo thinks it's worth it.

It should be a curious edge case to have enums with

const FOO = 1;
const BAR = '1';

and that case may be solved by calling hasValue(1, true)

[atymic]

These are just my two cents and feel free to correct me, but would native casting be avoidable if the $strict flag in hasValue() defaulted to false?

I hate the PHP type coercion, I think it's much better to be explicit.

I don't think you should ever have an enum with 1 in both int and string forms. That's something I don't think is worth protecting from.

[atymic]

cc @spawnia who always has good thoughts :)

[spawnia]

How about we do native casting by a simple, overwriteable function on the enum class?

/**
 * Cast values before constructing an enum from them.
 *
 * You may need to overwrite this when using string values that are returned
 * from a raw database query or a similar data source.
 *
 * @param  mixed  $value  A raw value that may have any native type
 * @return mixed  The value cast into the type this enum expects
 */
public static function castNative($value)
{
    return $value;
}

Less overhead, less string-based magic.

[spawnia]

I don't think you should ever have an enum with 1 in both int and string forms. That's something I don't think is worth protecting from.

I agree, Enum's should be monomorphic. We should mention that explicitly as part of the docs.

[cerbero90]

I don't think you should ever have an enum with 1 in both int and string forms. That's something I don't think is worth protecting from.

Agreed, what I meant is that my previous suggestion to default the flag to false would be problematic in presence of the edge case of values 1 and '1', which is not recommended.

Would be great if we didn't have to specify a native cast but that would only be possible with type coercion.

[cerbero90]

What do you guys think about introducing the following method in EnumCast:

/**
 * Retrieve the value that can be casted into Enum
 *
 * @param mixed $value
 * @return mixed
 */
protected function getCastableValue($value)
{
    // if the value exists in the enum, return it
    if ($this->enumClass::hasValue($value)) {
        return $value;
    }

    // if the value doesn't exist in the enum, return it and let it fail later
    if (!$this->enumClass::hasValue($value, false)) {
        return $value;
    }

    // return the value in the enum with the correct type
    foreach ($this->enumClass::getValues() as $enumValue) {
        if ($value == $enumValue) {
            return $enumValue;
        }
    }
}

So that castEnum() can call it to have the correct value to cast into Enum:

protected function castEnum($value): ?Enum
{
    $enum = $this->enumClass;

    if ($value === null || $value instanceof Enum) {
        return $value;
    }

    $value = $this->getCastableValue($value);

    return $enum::getInstance($value);
}

This would let us:

• avoid defining a native type
• prevent performing native cast
• have an easier way to cast enums by providing the enum class name only

[spawnia]

@cerbero90 so basically drop explicit casts in favor of implicit PHP style type coercion?

[cerbero90]

@spawnia correct, the underlying logic remains the same but we implicitly provide the right type without asking the user

[spawnia]

That is not guaranteed to produce a correct result. Let's suppose the Enum class has two possible values, which are returned as:

[true, false]

In the database the colums is defined as ENUM ('true','false').

PHP's magic coercion would make it so that every value returned from the database would result in an enum with the value true being instantiated.

>>> true == 'true'
=> true                                                                                                                                                                                                                                       
>>> true == 'false'
=> true

[cerbero90]

Good point @spawnia. I can be wrong but to me that sounds like an edge case, nonetheless I agree that we should be able to handle it regardless.

What about using both solutions:

protected function getCastableValue($value)
{
    // optionally cast the value into the expected type
    $value = $this->enumClass::castNative($value);

    // if the value exists in the enum, return it
    if ($this->enumClass::hasValue($value)) {
        return $value;
    }

    // if the value doesn't exist in the enum, return it and let it fail later
    if (!$this->enumClass::hasValue($value, false)) {
        return $value;
    }

    // return the value in the enum with the correct type
    foreach ($this->enumClass::getValues() as $enumValue) {
        if ($value == $enumValue) {
            return $enumValue;
        }
    }
}

That would solve the use case you mentioned by overriding your castNative() method with something like:

public static function castNative($value)
{
    return $value === 'true';
}

[spawnia]

@cerbero90 i like it, seems like a good compromise between convenience and offering control to the user.

To be honest, while trying to poke holes into PHP's type coercion, it behaves pretty reasonable for the kind of scenarios that would be encountered when coercing strings from a database to enum values. As shown, there can be some edge cases where it will not work cleanly, but overriding castNative() gives an escape hatch.

[atymic]

Awesome, thanks for the input @spawnia and @cerbero90

I've pushed up that strategy and all the tests pass 🎉

[cerbero90]

Good stuff guys 👍

@atymic do you mind changing the required PHP version from ~7.2.5 to ^7.2.5?
This would let us support PHP >= 7.2.5 and < 8.0.0 instead of PHP >= 7.2.5 and < 7.3.0

[spawnia]

Docs, Changelog and Upgrade guide are important for this PR. The upgrade guide should give concrete example of how to convert the casts, e.g.:

class MyModel extends Model
{
-   protected $enumCasts = [
+   protected $casts = [
        'foo' => Foo::class,
    ];

[atymic]

I guess if we're doing a major version bump, maybe we should drop the existing casting method completely since it's replaced by custom casts?

Or we can leave it (and deprecate it)

[spawnia]

@atymic i think we should try and have a release that contains both in order to allow incremental adoption. Then, we can remove $enumCasts after a grace period that allows for possible bugs or edge cases in the new native casts to be fixed. Maybe a few weeks, then we could cut a v3 that cleans up.

[netpok]

I would say if it's not hurting anything else then it should be only dropped when the LTS is no longer supported or there is an other change that actively requires the version bump.

Actually it needs a version bump and can not be merged to the current major version as the Castable interface is required.

[cerbero90]

Hey guys, any updates on this feature? Let me know if you need help

[shaffe-fr]

Hey, I can help if you need help.
I've been using this PR on my project for a few weeks and I encounter no problems.

[atymic]

I'll clean this up and get it ready to merge shortly :)

If you'd like to help, could you write the UPGRADE.md doc?

Thanks @shaffe-fr

[atymic]

Ok, have added docs and made the required changes. Thanks @shaffe-fr but no help needed :)

[atymic]

@BenSampo should be good to go for review now :)

[spawnia]

Nicely done @atymic, looking forward to using this. This approach is more powerful and simpler at the same time.

[shaffe-fr]

I've been using for a while this with a custom composer repo, it's very smooth and clean. Only problem I got was caused by a problem in laravel that will be fixed next release.
Great contribution 😊

[atymic]

@BenSampo sorry to bother you mate, let me know if there's any changes you'd like :)

[BenSampo]

All looks good. Awesome job on this @atymic and well supported by @spawnia and @cerbero90 - cheers guys.