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

Adding event context to EventLog service provider #1092

Closed
interworks-morr opened this issue Apr 3, 2024 · 2 comments
Closed

Adding event context to EventLog service provider #1092

interworks-morr opened this issue Apr 3, 2024 · 2 comments

Comments

@interworks-morr
Copy link
Contributor

Package targeted

Winter CMS

Description

The MessageLogged event listener in the ServiceProvider class excludes the event context instead of placing it in the details column (by passing it in as the details argument to EventLog::add). I have dynamically overridden this event locally for a while now with no ill effects and the stock output renders quite nicely as-is. I would like to submit a pull request to make this change upstream.

Additionally, a recent change added a type declaration to the EventLog::add function for the details parameter. The type declaration was set to ?string when the function assumes and even explicitly casts it to an array. The model has the details attribute set to JSONable, so I believe this type declaration is incorrect and causes needless double-encoding to JSON when using this function. I believe the type declaration should have been ?array instead.

Will this change be backwards-compatible?

I have had this event dynamically overridden in a custom plugin since version 1.0.x with no negative consequences, so it should be backward compatible.

@interworks-morr
Copy link
Contributor Author

Here's a preview of the intended change

diff --git a/modules/system/ServiceProvider.php b/modules/system/ServiceProvider.php
index 3dfd76c7a..771971f18 100644
--- a/modules/system/ServiceProvider.php
+++ b/modules/system/ServiceProvider.php
@@ -315,7 +315,8 @@ protected function registerLogging()
     {
         Event::listen(\Illuminate\Log\Events\MessageLogged::class, function ($event) {
             if (EventLog::useLogging()) {
-                EventLog::add($event->message, $event->level);
+                $details = $event->context ?? null;
+                EventLog::add($event->message, $event->level, $details);
             }
         });
     }
diff --git a/modules/system/models/EventLog.php b/modules/system/models/EventLog.php
index f72c4f459..2d2efc358 100644
--- a/modules/system/models/EventLog.php
+++ b/modules/system/models/EventLog.php
@@ -40,7 +40,7 @@ class_exists('Model') &&
     /**
      * Creates a log record
      */
-    public static function add(string $message, string $level = 'info', ?string $details = null): self
+    public static function add(string $message, string $level = 'info', ?array $details = null): self
     {
         $record = new static;
         $record->message = $message;

@LukeTowers
Copy link
Member

@interworks-morr sounds good, submit the PR please!

interworks-morr pushed a commit to interworks-morr/winter that referenced this issue Apr 9, 2024
Also fixes an issue with the type declaration of the EventLog::add
function.

Fixes issue wintercms#1092
(c.f. wintercms#1092)
@LukeTowers LukeTowers added this to the 1.2.6 milestone Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants