-
-
Notifications
You must be signed in to change notification settings - Fork 39
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 logging to Hub on update publish #110
Conversation
a7c6abb
to
21fb2fa
Compare
@dunglas CI is failing because of a coding standards check ( |
21fb2fa
to
dec5814
Compare
@@ -46,6 +46,7 @@ | |||
}, | |||
"require-dev": { | |||
"lcobucci/jwt": "^3.4|^4.0|^5.0", | |||
"psr/log": "^1.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"psr/log": "^1.0", | |
"psr/log": "^1.0 || ^2.0 || ^3.0", |
Thanks! PR to fix the the CS is welcome. |
'auth_bearer' => $jwt, | ||
'headers' => [ | ||
'Content-Type' => 'application/x-www-form-urlencoded', | ||
], | ||
'body' => Internal\QueryBuilder::build($postData), | ||
])->getContent(); | ||
|
||
$this->logUpdate($update); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would move the null check here to prevent a useless function call in case the logger isn't injected (this code may be in the hot path if the app sends a lof of updates):
$this->logUpdate($update); | |
if (null !== $this->logger) { | |
$this->logUpdate($update); | |
} |
$this->logger->info( | ||
sprintf( | ||
'Published update with ID \'%s\' on topic(s) [%s]', | ||
$update->getId(), | ||
implode(', ', $update->getTopics()) | ||
), | ||
[ | ||
'id' => $update->getId(), | ||
'topic' => $update->getTopics(), | ||
] | ||
); | ||
|
||
$this->logger->debug( | ||
sprintf('Debug details for update \'%s\'', $update->getId()), | ||
$this->updateToArray($update) | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO it would be better to have only one entry in the logs instead of two, even in debug mode. What do you think about like that:
$this->logger->info( | |
sprintf( | |
'Published update with ID \'%s\' on topic(s) [%s]', | |
$update->getId(), | |
implode(', ', $update->getTopics()) | |
), | |
[ | |
'id' => $update->getId(), | |
'topic' => $update->getTopics(), | |
] | |
); | |
$this->logger->debug( | |
sprintf('Debug details for update \'%s\'', $update->getId()), | |
$this->updateToArray($update) | |
); | |
} | |
$this->logger->info( | |
sprintf( | |
'Published update with ID "%s" on topic(s) [%s]', | |
$update->getId(), | |
implode(', ', $update->getTopics()) | |
), | |
$this->debug ? $this->updateToArray($update) : [ | |
'id' => $update->getId(), | |
'topic' => $update->getTopics(), | |
] : | |
); | |
} |
$this->debug
will be a new promoted property defaulting to false in the component, and set to %kernel.debug%
by the bundle.
This approach would also save 2 function calls in non-debug mode.
In favour of #86 #SymfonyHackday