Updating the logging code to not throw away all class information #1493
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We ran into a problem described in this PR: ubccr#1490 After some research I found that the root of the problem is this function json_encode'ing `$record` when the contents of `$record` may contain objects that do not support `JsonSerializable` but that do implement `toString()`. After a conversation w/ Joe it's been decided that our Logging API will be to support the logging of arrays w/ objects that implement a `toString()` function. The eagle-eyed may notice that this solution differs from the one I put forth in the above PR. The reason being that the original solution only supported the use case of having an array w/ top level value objects and would not work as expected if a nested array is supplied with an object that implements `toString()`.
ryanrath
added
bug
|
Loks good. Please can you add a description of what this logger supports at the top of the file (so we don't forgret in future). Something like: |
|
@jpwhite4 Absolutely, I'll add that and push it up momentarily. |
Per @jpwhite4, it's important that we document that our Logger supports logging both strings and arrays, and if an array is provided that contains objects, those objects will be converted to strings via the `__toString` function.
jpwhite4
approved these changes
Feb 10, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
We ran into a problem described in this PR: #1490 After some research I found that the
root of the problem is this function json_encode'ing
$recordwhen the contentsof
$recordmay contain objects that do not supportJsonSerializablebut thatdo implement
toString(). After a conversation w/ Joe it's been decided thatour Logging API will be to support the logging of arrays w/ objects that
implement a
toString()function. The eagle-eyed may notice that this solutiondiffers from the one I put forth in the above PR. The reason being that the
original solution only supported the use case of having an array w/ top level
value objects and would not work as expected if a nested array is supplied with
an object that implements
toString().Motivation and Context
It's nice to have a logging api that works as expected.
Tests performed
All automated tests.
Checklist: