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

[8.x] Custom casts can implement increment/decrement logic #34964

Merged
merged 3 commits into from
Oct 28, 2020
Merged

[8.x] Custom casts can implement increment/decrement logic #34964

merged 3 commits into from
Oct 28, 2020

Conversation

daniser
Copy link
Contributor

@daniser daniser commented Oct 25, 2020

This PR allows developers to add support of increment and decrement operations on their custom castable attributes. There are no breaking change, new functionality is covered with tests.

Reasoning

Consider a custom caster that casts the value to a value object, for example Money.
As we all know, PHP doesn't support operator overloading, and even if it once will - there's no guarantee that it will be implemented in value object implementation.
Which leads us to the following problem when trying to increment or decrement castable attribute:
Object of class Money\Money could not be converted to number in Model::incrementOrDecrement method:

$this->{$column} = $this->{$column} + ($method === 'increment' ? $amount : $amount * -1);

Why do we ever need this?

  • Consistency of value objects with simple numeric types;
  • Transparent usage when increments/decrements needed on multiple fields - no workaround logic needed;
  • Increment and decrement operations are atomic at database level - no explicit transactions required;
  • There are no excuse to dropping support of increment/decrement operations for custom castable attributes.

Naming problem
I've used word deviate as aggregate term for both increment and decrement. For example:

  • IncrementsAndDecrementsCastableAttributes -> DeviatesCastableAttributes;
  • incrementOrDecrementClassCastableAttribute -> deviateClassCastableAttribute;
  • isClassIncrementableAndDecrementable -> isClassDeviable.

It looks much more clean and, in my opinion, still perfectly describes logic behind it.
I'm welcome to adopt a better alternative if any.

Conclusion

This is my first PR ever, so please don't judge too much.
For my test I've reused Decimal value object class and DecimalCaster from PR #34702.
Also mentioned PR was good starting point and reference for my work.

@taylorotwell
Copy link
Member

taylorotwell commented Oct 27, 2020

Do you need to use increment and decrement at all? Can you just do:

$model->fill([
    'money' => $model->money->add(500),
])->save();

@daniser
Copy link
Contributor Author

daniser commented Oct 27, 2020

Say we have a model:

/**
 * @property Foo $foo
 * @property Bar $bar
 * @property Baz $baz
 */
class FooBarBazTest extends Model
{
    protected $casts = [
        'foo' => FooCast::class,
        'bar' => BarCast::class,
        'baz' => BazCast::class,
    ];
}

Here we have many different casts, but while they support increment/decrement functionality, incrementing them at once is as simple as that:

$model = FooBarBazTest::findOrFail(1);
$attributesToIncrement = ['foo', 'bar', 'baz'];
$incrementAmount = 100;
foreach ($attributesToIncrement as $attribute) {
    $model->increment($attribute, $incrementAmount);
}

... just keep in mind that $incrementAmount is a raw value that will actually be incremented at the database level.

Without using increment/decrement code will look like that:

$model = FooBarBazTest::findOrFail(1);
$attributesToIncrement = ['foo', 'bar', 'baz'];
$incrementAmount = 100;
foreach ($attributesToIncrement as $attribute) {
    switch (true) {

        case $model->{$attribute} instanceof Foo:
            $model->{$attribute} = $model->{$attribute}->add($incrementAmount);
            break;

        case $model->{$attribute} instanceof Bar:
            $model->{$attribute} = $model->{$attribute}->increment(new Bar($incrementAmount));
            break;

        case $model->{$attribute} instanceof Baz:
            $model->{$attribute} = $model->{$attribute}->increase(Baz::fromRawValue($incrementAmount));
            break;
    }
}
$model->save();

@daniser
Copy link
Contributor Author

daniser commented Oct 27, 2020

Also, when converting from primitive attributes to casted ones, I want to preserve functionality that have already worked. Why do we need increment and decrement in the first place?

@taylorotwell taylorotwell merged commit 1a55e3c into laravel:8.x Oct 28, 2020
@daniser daniser deleted the feature/deviable-casts branch October 28, 2020 20:12
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

2 participants