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

[7.x] Add HasCasterClass interface #32129

Merged
merged 3 commits into from
Mar 31, 2020
Merged

Conversation

brendt
Copy link
Contributor

@brendt brendt commented Mar 27, 2020

This PR adds an interface which allows castable types to specify their caster class. This approach has two advantages:

  • Packages can provide caster classes with less configuration required from the user
  • The casting configuration on models can now tell what class a property will be casted to, instead of what caster will be used.

As an example:

// Instead of this
class ModelX extends Model
{
    protected $casts = [
        'data' => CastToDTO::class . ':' . MyDTO::class,
    ];
}
// You could do this
class ModelY extends Model
{
    protected $casts = [
        'data' => MyDTO::class,
    ];
}

The underlying MyDTO class would look like this:

class MyDTO implements HasCasterClass
{
    public static function getCasterClass()
    {
        return CastToDTO::class . ':' . static::class;
    }
}

As you can tell from the example, my use case is with our DTO package. I would prefer our users to specify what type of DTO they want to cast to, instead of having to manually specify CastToDTO::class . ':' . MyDTO::class for each cast. The getCasterClass would be done in our abstract DataTransferObject implementation, and the user wouldn't have to worry about it.

@Gummibeer
Copy link
Contributor

I love this! But for https://github.com/spatie/laravel-enum we would need to be able to pass additional information too.

MyEnum::class.':nullable'
EnumCaster::class.':'.MyEnum::class.',nullable'

So the interface method should accept additional arguments.

@brendt
Copy link
Contributor Author

brendt commented Mar 27, 2020

@Gummibeer you'll return whatever string you like from getCasterClass, so that wouldn't be a problem I believe.

I might send a new PR after this one, which would allow to pass pre-configured instances of casters, instead of their class names. But that's after this.

@brendt
Copy link
Contributor Author

brendt commented Mar 27, 2020

Hmm, thinking about it, that might not be a solution for your problem…

@Gummibeer
Copy link
Contributor

I can return it - but it should also be possible to pass them in. So the method should accept all additional arguments, like the caster constructor.
I think that an array or the spread operator should be sufficient for this case!?

@brendt
Copy link
Contributor Author

brendt commented Mar 27, 2020

Gotcha. I will make sure $arguments are passed too, but I'll await Taylors feedback first, if he asks for additional changes, I can tackle them at once.

@taylorotwell
Copy link
Member

Yeah I think the additional arguments would definitely be needed in cases. Other than that I don't see any major issues.

@brendt
Copy link
Contributor Author

brendt commented Mar 27, 2020

I'll make these changes next week, I'm a little swamped right now with client work :)

@brendt
Copy link
Contributor Author

brendt commented Mar 27, 2020

I found some spare time. Attributes that are given to a HasCasterClass object are automatically passed to the underlying caster class.

@slavarazum
Copy link
Contributor

slavarazum commented Mar 28, 2020

I think we should have an ability to work with custom attributes in better way.

Class instances suits much better here:
Custom casts

Array from casts method can be merged with array from $casts property. So, we can choose from 2 options or use both.

What do you think?

@brendt
Copy link
Contributor Author

brendt commented Mar 31, 2020

@slavarazum I think it's a good feature, but outside the scope of this PR. I plan on looking it this after this feature is merged

@brendt
Copy link
Contributor Author

brendt commented Mar 31, 2020

@taylorotwell could you take a look at my final changes? Any more remarks?

@taylorotwell
Copy link
Member

Renamed HasCasterClass to Castable and getCasterClass to castUsing.

@taylorotwell taylorotwell merged commit a81614f into laravel:7.x Mar 31, 2020
@taylorotwell
Copy link
Member

May want to PR the docs for this.

@brendt
Copy link
Contributor Author

brendt commented Apr 1, 2020

Docs PR: laravel/docs#5934

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.

5 participants