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

Allow changes to the attribute data to log per attribute. #801

Conversation

indykoning
Copy link

@indykoning indykoning commented Oct 19, 2020

This is a simple solution to what's happening in #647
This small change allows anyone to change the data that gets logged per attribute without having to override any classes.
As an example the fix to #647 is to have a function as simple as

    public function attributeDataToRecord($attribute)
    {
        if ($this->isTranslatableAttribute($attribute)) {
            return $this->getTranslations($attribute);
        }
    }

exist on any model that uses the translatable trait.
Of course this function could also be set in a trait to apply it to a broader set of models.

Any feedback or other ways of making this easier to maintain with multiple packages would be more than welcome. 馃槃

@Gummibeer Gummibeer self-requested a review October 21, 2020 10:12
@Gummibeer Gummibeer added enhancement hacktoberfest https://hacktoberfest.digitalocean.com labels Oct 21, 2020
@Gummibeer
Copy link
Collaborator

Hey,

thanks for your work! 馃檪
I like the idea but am not comfortable with the proposed naming. A method attributeDataToRecord() is too generic and could conflict with other packages or app logic.

I'm thinking about something better right now - naming is hard.^^
I would like something that has log and/or activity in it and possibly even matches the existing methods somehow - so like getAttribute().

@brendt @freekmurze @AlexVanderbist @Nielsvanpach @sebastiandedeyne does anyone of you have a great idea? 馃

On top I'm thinking about something hookable to make it easier for packages to provide a bridge. 馃

One idea would be something similar to Laravel Resource when().

$value = $this->translatablePackageActivityLogBridge($attribute);

if(! ($value instanceof MissingValue)) {
    return $value;
}

// next

All this could be done in a nicer way by using pipeline/collection - but the idea. We have to use a dedicated n/a class/object as null and every other value would be possible values that should be logged.

But all this would be part of a second PR as it's not important for the hook at all that's implemented in this PR - it's only a feature on top that will make it easier for packages to incorporate with activitylog package.

@indykoning
Copy link
Author

I 100% agree, often the naming of things is actually more difficult than most things 馃槢
Maybe something along the line of getActivityLogAttribute()

The idea of being able to add multiple hooks is great, especially since it would allow other packages to change the attribute value without conflict.
Maybe add this to a backlog or an issue/feature request.
If i have the time i might try picking it up as well, but in another PR of course.

@Gummibeer
Copy link
Collaborator

Your PR should be valid for hacktoberfest without merging now.
So I would like to wait for feedback. This can take some time - I hope you understand it.
Don't worry, I will keep an eye on it so it won't get lost. But simply bumping is also not useful.
https://twitter.com/freekmurze/status/1318230638072385538

Thanks again for your work and the idea in general! 馃帀馃殌馃憣

@Gummibeer
Copy link
Collaborator

This one is replaced/solved by v4 #787 new pipeline logic.

@Gummibeer Gummibeer closed this Apr 27, 2021
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 hacktoberfest-accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants