owen-it / laravel-auditing

Record the change log from models in Laravel
https://laravel-auditing.com
MIT License
3.05k stars 390 forks source link

Not logging Affected Relations? #58

Closed bbredewold closed 2 years ago

bbredewold commented 8 years ago

Is it correct that any updated relations are not being logged in the database? I'm using eloquent's sync() function a lot, for updating many-to-many relations...

Any ideas how to do that?

camih commented 8 years ago

The auditing currently uses events, but in case of pivots, there are no events being triggered. There are many issues reported regarding this. See for example here: https://github.com/laravel/framework/issues/8452 . I also need this and currently did not find a very nice way to handle it.

robclancy commented 8 years ago

Can you not just override those methods like sync, attach and detach? Or even better would be a chain on the relationship in some way, will explain more if I come up with a way of doing this. Trying out this package and I think we will need it to work for this and we can work around it easy enough, if a workaround is good enough we will make a PR.

anteriovieira commented 8 years ago

Sorry for the delay.

Hello @robclancy , let's think of something. I'll keep this open to give continuity to this improvement.

fly-studio commented 7 years ago

yes, 'sync attach detach' donot log, 4.x support it?

quetzyg commented 7 years ago

@fly-studio, as soon as version 4.0 is out the door, we'll try to work on many-to-many audit support for 4.1.

projct1 commented 7 years ago

When will release 4 version? 😃

quetzyg commented 7 years ago

@rorc, when it's done (i.e. when I finish adding tests)

Meanwhile, you already can use "owen-it/laravel-auditing": "4.0.x-dev" in your composer.json. The v4 documentation is available here.

projct1 commented 7 years ago

@quetzyg Ok, thanks 😄

jschlies commented 7 years ago

I strongly vote for this feature. I'm currently working on a custom auditor that would record (to a seperate table) the foreign key relationships relations to a separate table, then I extend $Model->getAuditArr() to pull it all together the way we want. Lots of DB hits and in general, not a perfect solution. Will switch off this when this feature is implemented.

projct1 commented 7 years ago

How progress here? 😀

od3n commented 7 years ago

+1 still waiting for the best solution

ghost commented 7 years ago

considering using this package but I would need pivot auditing

neylsongularte commented 7 years ago

tenho o mesmo problema

neylsongularte commented 7 years ago

Experimental package for events in many to many relations:

https://github.com/neylsongularte/eloquent-extra-events

My temporary solution:

// Add on your AppServiceProvider
$saveManyToManyAudit = function ($event, $eventData) {
            $audit = new Audit;
            $audit->user_id = Auth::check() ? Auth::user()->getAuthIdentifier() : null;
            $audit->event = $event;
            $audit->auditable_id = $eventData['parent_id'];
            $audit->auditable_type = $eventData['parent_model'];
            $audit->old_values = [];
            unset($eventData['parent_id']);
            unset($eventData['parent_model']);
            $audit->new_values = $eventData;
            $audit->url = App::runningInConsole() ? 'console' : Request::fullUrl();
            $audit->ip_address = Request::ip();
            $audit->user_agent = Request::header('User-Agent');
            $audit->created_at = Carbon::now();
            $audit->save();
        };

        $this->app['events']->listen('eloquent.synced*', function ($eventData) use ($saveManyToManyAudit) {
            if($eventData['changes']['attached'] || $eventData['changes']['detached'] || $eventData['changes']['updated']) {
                $saveManyToManyAudit('synced', $eventData);
            }
        });

        $this->app['events']->listen('eloquent.attached*', function ($eventData) use ($saveManyToManyAudit) {
            $saveManyToManyAudit('attached', $eventData);
        });

        $this->app['events']->listen('eloquent.detached*', function ($eventData) use ($saveManyToManyAudit) {
            $saveManyToManyAudit('detached', $eventData);
        });

@anteriovieira

neylsongularte commented 7 years ago

https://github.com/laravel/framework/pull/14988

manniL commented 7 years ago

Any update concerning this problem?

quetzyg commented 7 years ago

Hi everyone,

I've been thinking in a few ways to achieve this, and I'm inclided to leave it up to the developer to implement part of the functionality.

The package can already handle other events besides created, updated,deleted and restored, and like Taylor suggests, the attach/detach/sync logic should be handled in a repository, in which the proper event is fired after the DB operation happens.

So, the idea is to add three new AuditableObserver methods: attached(), detached() and synced()

The developer would be responsible for firing/dispatching said events and pass the data they wish to audit/log.

From that point on, the appropriate AuditableObserver method would kick in and handle things.

I think this brings a good balance between new feature and code complexity.

Thoughts?

manniL commented 7 years ago

@quetzyg Sounds good to me! Because there won't be an official support concerning the events, I think that's a very good way, especially because the developers are now "unrestricted".

But please provide an example for the implementation in the docs 😛

quetzyg commented 7 years ago

@manniL, yeah, the idea is to keep it flexible, because I have no idea how developers want to store changes, specially when we're talking about sync events, which may affect more than one row in a pivot table. Do they want to create an audit with all the row changes or do they want to audit each row individually? There's no one-size-fits-all in this case.

And don't worry, the docs will be updated once this gets implemented.

manniL commented 7 years ago

@quetzyg That's a good plan!

Well, in my case I'd simply store the changes (e.g. User A Was linked to posts 1,2,3 and is now linked to Post 4,6,7, I'd just change the stored numbers). If the relation models itself were changed, it'd be fine if there would be an extra audit model for that.

quetzyg commented 7 years ago

For a simple case like the one you described it's fine to use just one audit record, but when you have extra data in the pivot or when you detach hundreds of relations in one go, things would get out of hand very quickly and stuffing all that data in just one record would make it hard to track changes.

But again, I'll leave that decision to the developer.

manniL commented 7 years ago

@quetzyg Yes, this can become very hairy in situations like the one you described. But as the developers are flexible, everyone will adapt it to their needs :)

jschlies commented 7 years ago

I think I have a solution for issue. Are you accepting pull requests??? It has, I admin, some of the data issues stated above. I can have it done in acceptable form in a couple of Saturdays

quetzyg commented 7 years ago

@jschlies, sure. Feel free to send a PR and we'll review it.

manniL commented 7 years ago

I'm very curious! Good luck @jschlies and hit me up when u need help :)

neylsongularte commented 7 years ago

:+1:

neylsongularte commented 7 years ago

Hey guys, any progress so far?

quetzyg commented 7 years ago

I have done some work in this regard, but there's no ETA. You'll have to be patient 😄

sarah-brittan commented 7 years ago

I'm actively needing auditing for detach(), save() and sync() too at the moment, so I'm really keen to see what you are working on @quetzyg :)

jschlies commented 7 years ago

See PR https://github.com/owen-it/laravel-auditing/pull/289 I'm going to try to give this some of my time. I hope I have a 'merge-able' branch in a few months. This PR is my work so far.

Please let me know if I'm on the right track.

Going forward, I want to:

jschlies commented 7 years ago

@quetzyg

I'm not a fan of using a second audit table. You also talk about a convention to limit what gets audited and what doesn't. I also don't like that idea. Read my comment about letting the developer decide which data should be audited.

Regarding "convention to limit what gets audited ", I plan on making , per object, either an entry in the config file OR a method call to that object. Completely configurable by developer.

I think I can do without a second table. The second table was easier when I was just coding for our own needs. I just need to fight through the key constraints and such. After some progress, I'll get back to you.

jschlies commented 7 years ago

FYI, I think I'm not half way.

See latest:

jschlies commented 7 years ago

I've added a migration to add related_id. I notice that there is no migration for updated_at. I have added this to my migration for now. Suggestions as to how to handle this?????

quetzyg commented 7 years ago

@jschlies, please keep the conversation about your PR in the actual PR comment section.

aheinzm commented 7 years ago

:+1:

projct1 commented 7 years ago

So, the idea is to add three new AuditableObserver methods: attached(), detached() and synced()

How we can now extend AuditableObserver for add this methods?

But, if i dont want add new audit record for relation sync? Before save my model, i try to add relation, but plugin ignoring new key (relation_name):

$model->new_values = collect($model->new_values)->put('relation_name', [1, 2, 3]);
$model->save();
anteriovieira commented 7 years ago

@GrahamCampbell, can you give us guidance on this? :pensive:

ghost commented 7 years ago

Hey everyone,

so after reading the comments and ideas in here, I set out to get this working for me.

What I did was, I got a generic piece of code (call it a repository if you will) which handles attaching, detaching and updating pivots for any many-to-many relationship (ie. for any model pair that has a many-to-many relationship between them). In there, I start by storing the related models before any operation, do the operation and then get the updated related models. Finally, I fire up an event named eloquent.many2many.attached, eloquent.many2many.detached or eloquent.many2many.updatedPivots with these 2 arrays (the initial and updated related models) as data.

I defined an Event listener for eloquent.many2many.* (which allows me to catch only "my" events) where I store the initial and updated related models in the model being audited itself (ie. the "parent" mode whose relationships are being updated), call Auditor::execute() and in the auditAttachedAttributes, auditDetachedAttributes and auditUpdatedPivotsAttributes methods I simply do:

$old = $this->initialRelated;
$new = $this->updatedRelated;

Which allows me to get the wanted audit rows as I want them, ie. one per attach/detach/pivot update with the before and after related models. So far so good.

My problem is that when looking at a certain auditable model's audits (for "my" events) I have no way of knowing what related models those audits refer to. I have the before and after related models, but I don't know the type of those models.

I understand I could get this piece of information by having different events for different related models (eg. attachedModelA and attachedModelB), where the event name itself would tell me the related model's type, but that would force my generic many-to-many audit handling not to be generic anymore, since I would have to implement the audit*Attributes methods for all the possible events, ie. auditAttachedModelAAttributes, auditAttachedModelBAttributes, etc. This would indeed defeat the purpose of a generic approach as the one I'm hoping to achieve.

For a proposed solution... Maybe the $auditableEvents parameter could accept * like Laravel's Event listeners themselves accept? This, together with my suggestion in #320 would solve my problem, as I could define attached*, detached* and updatedPivots* as auditable events and make them all be handled by a generic method such as the one I have now (while still allowing different event names).

ghost commented 7 years ago

I did my 2 suggestions in https://github.com/owen-it/laravel-auditing/pull/324 - they do solve my problem!

jschlies commented 7 years ago

@daniel-gomes-sociomantic , I think I get the above approach and I suggest it has merit. My comments are:

ghost commented 7 years ago

You seem to be limited to Eloquent relations. Our need included 'relations' that were outside the standard Eloquent relation.

Technically, since I'm firing the events manually, it would be possibly to do so for non-eloquent relations, but not sure how the auditing package itself would handle them..

I'm don't see how you limit which relations to ausit. I can foresee circumstances where you're not interested in auditing ALL many-to-many. Or is this logic found in auditAttachedModelBAttributes????

As I said, I made all attaching/detaching/pivot updating be handled by a generic piece of code (a trait, to be specific) which is then used to update the many-to-many relationships I want audited. It would be simple enough to adapt the trait to allow handling the many-to-many relationship without firing the audit events.

Would you logic work if a particular model was attached - detached - RE-attached???

The way I implemented it, that would cause 3 audits, one for the attaching, one for the detaching and one for the re-attaching, each with their initial and updated states.

jschlies commented 7 years ago

Thanks @daniel-gomes-sociomantic , I'm not sure I'm following 100% percent but it sounds like you've thought through the pitfalls we experienced. I still believe in the efficacy of https://github.com/owen-it/laravel-auditing/pull/321 and I leave things to the group.

strangetopology commented 7 years ago

Would definitely like to be able to integrate auditing of attach/detach/sync events for related models into a project I'm currently taking over. I'm relatively new to laravel (and modern PHP, in general) and so far, laravel-auditing is one of my favorite libraries. This one additional (albeit non-trivial) functionality would elevate it to epic.

ctf0 commented 6 years ago

any news on this ?

quetzyg commented 6 years ago

Not yet.

garbinmarcelo commented 6 years ago

Any news or release date?

quetzyg commented 6 years ago

Hi @marcelogarbin, thanks for your interest on the project!

Unfortunately, I cannot commit to a release date for this feature. There were a couple of PR's made, but to be honest, I didn't like the logic or the code.

I also have an internal branch I'm keeping up to date with my own implementation, but it still isn't exactly the way I like.

In case someone wants to sponsor this feature, I'll work exclusively on it, instead of doing it on my spare time, because Family > Work > Open Source.

We'll keep this open.

Thank you all!

garbinmarcelo commented 6 years ago

Thanks for the quick feedback. I started using the package today and I just noticed it for now. I still have no deep knowledge of Laravel and the PHP testing part to be more consistent in helping you. However, I'm at your disposal.

johnpaulmedina commented 6 years ago

@quetzyg Since you are working on one behind the senes, what would you say your completion rate is thus far and how much more time would be needed?

quetzyg commented 6 years ago

@johnpaulmedina, the development of this feature is currently stalled and I have no idea when I'm gonna pick it up. If working exclusively, it could probably take me a month to have it finished (with planning + experimenting, development, documentation and tests).

But like I said a few comments back, priorities :)