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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use HasAttributes::originalIsEquivalent() for attribute change detection #971

Open
Gummibeer opened this issue Oct 20, 2021 · 3 comments
Open
Labels
enhancement hacktoberfest https://hacktoberfest.digitalocean.com help wanted

Comments

@Gummibeer
Copy link
Collaborator

Gummibeer commented Oct 20, 2021

Found the \Illuminate\Database\Eloquent\Concerns\HasAttributes::originalIsEquivalent() method during code diving and it should be usable for our trait! 馃帀
This PR would be mainly reasearch when that method was added, if it's compatible with our version support rules and switching our own change detection logic with the core one.

@Gummibeer Gummibeer added enhancement hacktoberfest https://hacktoberfest.digitalocean.com help wanted labels Oct 20, 2021
@kallepyorala
Copy link

Does that work with attribute casting as well? I was just playing with state attribute which is casted to ModelState and got it working by adding this to LogsActivity trait's attributeValuesToBeLogged function:

if ($old instanceof Spatie\ModelStates\State && $new instanceof Spatie\ModelStates\State) {
    return $old->equals($new) ? 0 : 1;
}

I think there should be some generic way to handle all casted attributes.

@Gummibeer
Copy link
Collaborator Author

@azurinspire no, it has a lot of cases but none for custom casts - these will fall back to strcmp.
https://github.com/laravel/framework/blob/95c86075d80daf54ba370db8a991c378cba1ebcd/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php#L1646-L1680

I think it would help if we move the current diff-check callback to its own method and write it that you can easily override and extend it.

function ($new, $old) {
// Strict check for php's weird behaviors
if ($old === null || $new === null) {
return $new === $old ? 0 : 1;
}
// Handels Date intervels comparsons since php cannot use spaceship
// Operator to compare them and will throw ErrorException.
if ($old instanceof DateInterval) {
return CarbonInterval::make($old)->equalTo($new) ? 0 : 1;
} elseif ($new instanceof DateInterval) {
return CarbonInterval::make($new)->equalTo($old) ? 0 : 1;
}
return $new <=> $old;
}

public function foobar(): int
{
    $foo = parent::foobar();

    if ($old instanceof Spatie\ModelStates\State && $new instanceof Spatie\ModelStates\State) {
        return $old->equals($new) ? 0 : 1;
    }

    return $foo;
}

(intentionally used foobar as I didn't have time now to imagine a proper name)

But such a change would help allow you to adjust the value comparison without overriding the whole surrounding method!?

@kallepyorala
Copy link

@Gummibeer sounds good solution to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement hacktoberfest https://hacktoberfest.digitalocean.com help wanted
Projects
None yet
Development

No branches or pull requests

2 participants