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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add extra fields and indices to audit tables, based on configuration #61

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

WhatsOn
Copy link

@WhatsOn WhatsOn commented Nov 17, 2021

This pull request allows users to create extra columns in their audit tables. This is useful when You need to group audit events with an external parameter for easy retrieval later on.

Adds the options "extra_fields" and "extra_indices" to the Configuration object:

new Configuration([
    'table_prefix' => '',
    'table_suffix' => '_audit',
    'ignored_columns' => [],
    'entities' => [],
    'viewer' => true,
    'extra_fields' => [
        'extra_column' => [
            'type' => 'string',
            'options' => ['notnull' => true]
        ]
    ],
    'extra_indices' => ['extra_column' => null]
]);

Adds the ability to query the audit table on the configured extra_indices via filters, eg.

$query->addFilter(new SimpleFilter('extra_column', '123abc'));

The extra column values can be set using an EventSubscriber:

class AuditEventSubscriber implements EventSubscriberInterface
{
    public static function getSubscribedEvents(): array
    {
        return [
            LifecycleEvent::class => 'onAuditEvent',
        ];
    }

    public function onAuditEvent(LifecycleEvent $event): LifecycleEvent
    {
        $payload = $event->getPayload();
        $entity = $this->entityManager->getRepository($payload['entity'])->find($payload['object_id']);

        $payload['extra_column'] = "fancy_value_123";

        $event->setPayload($payload);
        return $event;
    }
}

@DamienHarper
Copy link
Owner

@WhatsOn Thanks for the contribution! I'll have a closer look at it asap.

@Swahjak
Copy link

Swahjak commented Apr 4, 2022

@DamienHarper do you still plan on taking a look at this? Would be greatly appreciated. For example; we're looking to use this to let users also add an (optional) comment as to why a certain change was made.

@DamienHarper
Copy link
Owner

@Swahjak @WhatsOn sure, it's still on my roadmap but I have to figure out some "issues" or edge cases before merging this feature (eg. if a user can add new "columns" from config file, he probably can also remove them, so how does the audit:schema:update command should behave? what to do with the data previously recorded in these new "old" columns?)

@Swahjak
Copy link

Swahjak commented Apr 5, 2022

@DamienHarper completely understand and not trying to rush you. Just wanted to know if this is still something that you would consider merging (at some point).

You make some valid points about the updating / removing of fields. My initial thought would be to only allow columns to be added through configuration and update / remove would be something the user has to take care of through custom migrations. Although doctrine would / might be able to manage this there would always be that edge case. Maybe this shouldn't be a configuration at all, but just a 'hook' that allows others to fiddle with the columns (and inserts), that way the responsibility does not lie with the library / bundle but with the user.

@Alarich
Copy link

Alarich commented Jun 17, 2022

@Swahjak @WhatsOn sure, it's still on my roadmap but I have to figure out some "issues" or edge cases before merging this feature (eg. if a user can add new "columns" from config file, he probably can also remove them, so how does the audit:schema:update command should behave? what to do with the data previously recorded in these new "old" columns?)

Suggestion: Never remove a field that was not part of the core auditor field.
Developers can handle the removal of their own custom fields. It would just be a disclaimer in the documentation for custom fields.

This feature is way too useful to not include.

@kasatria-fong
Copy link

Hi, a dumb question: this pull is not inside master yet, right ?

@Alarich
Copy link

Alarich commented Jun 29, 2022

Hi, a dumb question: this pull is not inside master yet, right ?

It is not.
We're using the feature in a separate fork, so far no issues.

@etiennecotin
Copy link

Hello, have you plan to merge this feature soon ?

@DamienHarper
Copy link
Owner

@etiennecotin Well, it depends on what you consider as "soon" :) It won't happen until 2023 though.

@etiennecotin
Copy link

@DamienHarper Thx for the answer :)
I will try to find a solution until the release of this feature

@joerndyherrn
Copy link
Contributor

@DamienHarper could you devote yourself to this again? :)

@CaptainPerox
Copy link

@DamienHarper Is there an update for this feature? :-)

@mcapinha-mxp
Copy link

+1 on this one as we were discussing extending the auditor in this way.
Our use case is for adding additional context, as we have a multi-tenant DB, and recording relantionship information (as some records can be deleted and its hard do keep track of those).

@Izakun
Copy link

Izakun commented Nov 8, 2023

@DamienHarper Hi, have you a plan to merge this feature soon :-) ? We are in 2023 ^^

It won't happen until 2023 though.

Thank's

@wouter-toppy
Copy link

@WhatsOn @Swahjak @DamienHarper See #187

wouter-toppy added a commit to wouter-toppy/auditor-bundle that referenced this pull request Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.