Log only dirty fields doesn't behave as expected when used with scope

[istiak-tridip]

I'm updating a video status by using this code

$video = Video::identifier()->addSelect("status")->where("id", "15")->first();
$newStatus = ($video->status != "active" ? "hidden" : "active");     

$video->update(["status" => $newStatus]);

In the properties column of the activity_log table I'm getting this

{
  "attributes": {
    "slug": "MPnMwgXCU48YogB",
    "title": "Video Tile Placeholder",
    "duration": 1574664899,
    "status": "hidden",
  },
  "old": {
    "slug": null,
    "title": null,
    "duration": null,
    "status": "active",
  }
}

instead of expected

{
  "attributes": {
    "status": "active"
  },
  "old": {
    "status": "hidden"
  }
}

If I remove the scopeIdentifier it works expectedly.

I'm using:

• Laravel: 5.8.10
• laravel-activitylog: 3.4.0

[Gummibeer]

Hey,
could you provide your settings in the model?
I expect you have static::$logOnlyDirty = true!? But what have you set static::$logFillable, static::$logUnguarded, static::$logAttributes and static::$logAttributesToIgnore to?

I would like to reproduce it in a unittest and after this try to solve the issue.

[istiak-tridip]

I am using these settings:

protected static $logOnlyDirty = true;
protected static $logUnguarded = true;

I think the problem is with the $model->fresh() method. The fresh method reloads without the scopes & selected columns. It's just a guess though.

[istiak-tridip]

Hey @Gummibeer, were you able to reproduce the problem?

[Gummibeer]

@istiak-tridip sorry, not yet and I have a lot of tasks for the next two weeks.
So I think that I won't find time to debug, fix, release or discuss anything.

I will pass the ball to @spatie team.

I hope you understand it - for sure feel free to fix it yourself and send a PR.

[nagi1]

I am using these settings:

protected static $logOnlyDirty = true;
protected static $logUnguarded = true;

I think the problem is with the $model->fresh() method. The fresh method reloads without the scopes & selected columns. It's just a guess though.

Please provide a steps to reproduce this error... cant manage to reproduce it!

[Gummibeer]

@istiak-tridip in best case a PR with a failing Testcase. This way we have a real goal and way to check if we've solved it.

[nagi1]

$model->fresh()

Thanks for the quick response and the PR.

I'm currently working on solving this but since I'm kinda new to the codebase I wish that you enlighten me more about why we use fresh in this situation, particularly in this

DetectsCahnges.php:97

        $properties['attributes'] = static::logChanges(
            $processingEvent == 'retrieved'
                ? $this
                : (
                    $this->exists
                        ? $this->fresh() ?? $this
                        : $this
                )
        )

@Gummibeer, if you can spare a minute it would be kind if you explained above piece of code.

[Gummibeer]

Answer: this is one of the oldest parts of this package - sometimes re-arranged but never changed.
It was part of this package before I've joined in. So it's in since Laravel 5 versions.
I would remove it and check if/which tests fail. The names of the failing tests will/should explain it more.^^
If nothing fails: it should be safe to remove it.
I think that this was added to get database default values - as they wouldn't be part of the saved model instance.
If this has to be changed I will reject supporting attribute subsets as, for me, it's the less common case of both we have to decide between and it's also the one that would require breaking code changes.

Personally I've never used attribute subsets on model queries and primary not for manipulating the model instance. The only way I could think about to solve this would be to access, if available, the attribute subset of the original select() query and pass it down to the fresh() one or to a Arr::only() call. Possibly even worth an issue to the laravel framework itself that the limited attribute selection isn't removed during a fresh() call.

[nagi1]

I tend to agree more with @Gummibeer since I spent more than 5 hours across multiple days fiddling around with this issue.

I tried to remove

        $properties['attributes'] = static::logChanges(
            $processingEvent == 'retrieved'
                ? $this
                : (
                    $this->exists
                        ? $this->fresh() ?? $this
                        : $this
                )
        )

and replace it with

        $properties['attributes'] = static::logChanges(
                    $this 
                )
        )

8 test have failed, the common failure between them was that the package was relaying on fresh() to fetch all columns from database then filter them based on static attributes (options) and what not.

I think this is an edge case that only 1% of people will fall into because local scopes isn't designed to handle filtering columns on the query.

Plus Laravel doesnt support passing scopes nor selected attributes to the fresh method, based on Mohamed Said's response on this issue on laravel repo.

Fresh doesn't use scopes by default, this is by design, if you want a different behaviour you can add your own version of fresh in a base model.

To me that scope you're adding is not the best approach, using am eloquent mutator is a better approach.

Based on all of the above, we may close this issue considering it an edge case that will probably wont affect users who use scopes as they intended. (no offence @istiak-tridip 😄).

[istiak-tridip]

I agree with you, @nagi1.
I only needed this use case for a specific project and I bypassed this issue by creating a fork (with failing tests 😳).

You guys can go ahead and close this at your convenience.

[Gummibeer]

@nagi1 THANKS A LOT for your work on this! 🤩🥳
🙏🚀🎉

You did a great job in verifying my opinion.^^