Laravel-activitylog: Opposite of $ignoreChangedAttributes?

[bluec]

May be useful to have a reverse of this, something like $ignoreChangedAttributesExcept or $onlyChangedAttributes?

i.e. we list only the attributes that we wish to trigger an activity being logged.

Does such a thing exist? Would anyone else find it useful? Have I overlooked another way to do this?

[Gummibeer]

There were already a lot of variables that allow to customize the logged attributes. A new one will just make this package a lot more complicated. I would recommend you, if you need this, to override the shouldLogEvent() method, which uses the $ignoreChangedAttributes.
https://github.com/spatie/laravel-activitylog/blob/c1dd4932d21216a3d0b23778c867ee1000eb87c7/src/Traits/LogsActivity.php#L107-L125
On the last line you could add your logic and use array_only() or you change attributesToBeIgnored() method and inverse your whitelist via the available attributes to the already existing blacklist. This could also be done on boot() in best case via trait-boot-method https://laravel-news.com/booting-eloquent-model-traits - this way you won't touch any of the package logic but could get your request done in your own trait.
If you got the trait work you are free to create a gist with it and link it here for others with this request. Keep in mind that $ignoreChangedAttributes does not remove the attributes from the log entry but just is used to decide if the entry has to be logged or not.

[bluec]

Hi @Gummibeer thanks for the response.

I believe this should be included. I think in Laravel we're all very familiar with whitelist/blacklist functionality like fillable/guarded. This is only a similar thing.

You say it makes the package a lot more complicated but in fact the required code changes are minimal so there is no added complexity there. What I think might be the problem is that there are already a lot of variables and this _seems_ complicated, but I would politely suggest this is down to the documentation and explanation of each variable/property rather than the variables themselves or their implemented logic. I have personally found it confusing to get to grips with so I'm only speaking from my own experience.

I think I saw there has been some refactoring recently to make variable names more friendly and this could probably benefit from something like this:

    /**
     * Update activity won't be logged if only these attributes changed.
     *
     * @var array
     */
    protected static $dontLogUpdateFor = [];

    /**
     * Update activity will only be logged if any of these attributes changed.
     *
     * @var array
     */
    protected static $onlyLogUpdateFor = [];

Note: you may only use $dontLogUpdateFor or $onlyLogUpdateFor

What do you think?

[Gummibeer]

Thanks for your input!
The complexity isn't only about the needed code - yes this isn't much. But primary to test, document and keep in mind.

I agree that there are a lot of static variables and the documentation isn't perfect. But to rework this and make these more readable we would need a v4 and it's just a month since we did the step to v3.
The next thing is the need: until now no one has requested this (except you) - so could be that they are just silent and have solved it. Or it it's really not needed.
My major problem with white-/blacklist is to define the priority.

The last decision is up to @freekmurze but he want's to keep everything as simple as possible.

My idea/offer would be to bring all the static variables and possible opposites into discussion for v4. I also see some optimization room in this area and would think about ways to list them in some way not only in the code which use it. Ideas would be to switch to non-static or even abstract methods and/or interface which forces the developer to see them all and think about what/how to use.

But so far I don't see a good reason to add more complexity if it's easy for the developer to do it himself. And like I said I would be happy about a gist for the others how to do it. If we decide to implement it we could use your gist as base.

For the future I would also like to think about an Observer approach and not the traits.

[bluec]

I think those are some good points !

I think to rework doesn't need to be v4 immediately. Firstly implementing this doesn't _need_ a rework of the names, I just offered it as a suggestion because $ignoreChangedAttributes is confusing! Also it's possible to depricate but keep active some names so that this wouldn't be a breaking change. E.g. the new $dontLogUpdateFor is just an alias of existing $ignoreChangedAttributes with exact same functionality. This is what Laravel does when it renames things.

Demand-wise, I understand, but I can't help feel that implementing one without the other feels unbalanced. I am sure people would benefit from having both options available. Also I would argue that if only one was offered it should be the opposite of what you've currently included because whitelists are inherently safer, easier, and more comprehensible, as they are explicitly defined.

In terms of priority for whitelist/blacklist, just pick one that wins and document it. I would say the whitelist should always win since it is safer to rely on an explicitly defined list rather than a list of exclusions.

I think this can be acheived within v3 and it would be a really useful addition. I'll send @freekmurze a postcard if he agrees 😀

[Gummibeer]

Ok, adding these new things within v3 and keep the old ones as alias but deprecated would be an option. But the problem here is that the only way to mark them as deprecated would be to put it in the docs. There is no way to use the @deprecated php-doc flag. 😕
One of my goals for a rewrite would be to find a way to have them defined with default values to remove all the isset(static::$abc) calls and be able to use php-type-hins, php-doc and so on. This would help a lot if you use an IDE like PhpStorm.
I agree that the pure addition of the new variable (whitelist) wouldn't introduce a BC. We already have a whitelist for the list of logged attributes attributesToBeLogged().
https://github.com/spatie/laravel-activitylog/blob/ae7558db4d0f926b2c02dfec50c0e927858d00a0/src/Traits/DetectsChanges.php#L26-L51

So yeah we also already have a rule how to handle whitelist and blacklist at the same time and decided the other way. An attribute listed in the blacklist will never be logged - independent of the whitelist. The reason for this was that the list of attributes could also be filled by dynamic lists (fillable & guarded).

My major issue with the current available variables is that we have two parts that have configuration for what they do:

1. what will trigger an activity entry (which events, which attributes if changed and so on)
2. what will be logged if an activity is created

Primary in the attributes case this seems pretty similar and the variables aren't clear enough there. This has caused a lot of issues in the past and even I have to open the code and look which does what.
Adding a new one will just increase this problem f it's not done right. The alias-idea could be a solution! (thanks for this) For myself I don't like to migrate this way because you have to keep code that isn't needed - I like hard breaks more, no room for deprecated usage. But I would let @freekmurze decide this, if he says a new major-release in, I would say about a month is needed, isn't an option and we should wait for more major-issues I would be fine with aliases.

To make it clear: I'm 100% with you and don't want to close this issue without a solution. But as a package collaborator I also have to keep the others in mind and with ~700k downloads atm there are a lot of them.

I would end this discussion between us two for the moment and wait what freekmurze says. We have cleared out our standing, solutions, problems and ideas but aren't able/allowed to decide how to do it.

[freekmurze]

Currently I don't want to invest time to do this. There aren't many people asking for this feature. I think it could be part of a rewrite for a new major version.

Currently a new major version of this package isn't high on my planning. If other good breaking feature ideas come in, I'll revisit this issue.

[kbond]

I also required this functionality so I extended the LogsActivity trait:

use Spatie\Activitylog\Traits\LogsActivity as BaseLogsActivity;

trait LogsActivity
{
    use BaseLogsActivity {
        attributesToBeIgnored as protected baseAttributesToBeIgnored;
    }

    protected static $logOnlyChangedAttributes = true;

    public function attributesToBeIgnored(): array
    {
        if (self::$logOnlyChangedAttributes) {
            return array_keys(array_except($this->getAttributes(), $this->attributesToBeLogged()));
        }

        return $this->baseAttributesToBeIgnored();
    }
}

[bluec]

Currently I don't want to invest time to do this. There aren't many people asking for this feature. I think it could be part of a rewrite for a new major version.

Currently a new major version of this package isn't high on my planning. If other good breaking feature ideas come in, I'll revisit this issue.

Hi @freekmurze it feels like maybe this issue got misunderstood with all the talk of changing variable names which was really a side-issue. The original request is not a BC or a major investment of time nor does it require a major release so it would be great if you could reconsider including this small change that would really help usability of the extension.