Audit Log IP addresses

[acornforth]

we have been running a forked version of this plugin since april 2019, We copied the code to our local repo and made our mods to allow logging of IP. Now we need to update to main repo version to receive updates. But want to keep IP logging.

Since this is likely to be useful to other users I'm submitting this PR for your consideration.

Supplied code has been running successfully on our production systems for the last 3 years.

[natewiebe13]

Thanks for your contribution. The v0 series is planned to only receive bugfixes moving forward. v1.0 includes a number of changes and would be best to use as a target for this PR.

Can you rebase on v1.0 and make the necessary changes?

Other notes I can give right away:

• This should be behind a configuration, disabled by default, as this information is considered personal identifiable information
• If you'd be willing to, I think storing the user agent string (behind same config) would be awesome as well
• Please add tests (there's only a basic one at the moment, let me know if you run into issues with them running)

[acornforth]

Hi @natewiebe13 ,
Yes, i can rebase onto 1.0 and tweak my submission based on your excellent notes. Also happy to add tests and UA field as part of this PR.

There is one small sticking point though, I wont actually be able to use any of these recent versions myself, since support for dbal < 3 is dropped, unfortunately we are using this in a Sylius project that has nailed it's doctrine/dbal dependency to ^2.7.

In theory it's not too big of a deal for us. We can continue to maintain our fork from an earlier version of this plugin.
I have to wonder if dropping support for dbal 2.x was a little premature though, since Doctrine is continuing to support 2.12 and many projects are still using it. Is it feasible to re-add support for dbal 2.12 in time for v1.0 release ?

[natewiebe13]

I'll take a look at adding dbal 2 support back in. I can't remember the specifics, but I think there was some added complexity with supporting both. But considering Sylius is still on v2, I'm good with reassessing. Thanks!

[acornforth]

@natewiebe13 It's rebased ok, but haven't managed to add any tests yet.

I have added logging of UA string, as requested, but I'm not convinced it's a good thing to encourage this. At least it is not the default behavior. It currently truncates to 255 chars.

I have marked the PR as draft for the moment as i plan to install this fork into a project and test it in a real world application. We have an impetus to start using this development version as a soon as we can as it will enable us to get some other important updates installed. I can revisit the phpUnit tests after that. (i didn't have a problem getting the basic test to run)

[acornforth]

I noticed too that you re-added support for dbal ^2.9, that is very appreciated,

If i can be of any help getting over the finish line with your 1.0 release. I'd be happy to do so.

[acornforth]

I think if any more configuration keys are added, it may be better to group them into arrayNodes and reduce the number of setter methods needed in AuditSubscriber, or inject them as a constructor arg... something for another PR though i think .

[acornforth]

found an issue when pulling this branch in to a project, Im pretty sure this has nothing to do with my submitted changes, looks like an unintended consequence of the mapping changes in previous commits, and might only affect postgres .

I can open a separate issue for this if you think it's appropriate?

(https://user-images.githubusercontent.com/4586381/157653353-4d1724d1-9249-46e6-aa12-ce9234b9ba86.png) exception

UPDATE:

I checked out the 1.0.x-dev branch and confirmed that this issue is still present, therefore not related to this PR. Maybe you are already aware of this ?

[natewiebe13]

I think if any more configuration keys are added, it may be better to group them into arrayNodes and reduce the number of setter methods needed in AuditSubscriber, or inject them as a constructor arg... something for another PR though i think .

Agreed. This is actually something I was planning on tackling in a future PR as well.

[acornforth]

@natewiebe13

I changed AUTO strategy to IDENTITY, (should work on both postgres and mysql. and on Postgres doesn't cause a migration to be created that drops the DEFAULT NEXTVAL('audit_logs_id_seq'). (Fixes #95)

I have tested this installed into a real project, and it all appears to work. Only issue I've found is that decorating the audit subscriber seems to prevent the config setters in AuditSubscriber.php from being called. Maybe i was doing something wrong, but with no decorator, this all works well.