-
Notifications
You must be signed in to change notification settings - Fork 7
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
Reorder conversion methods in ParamConverter and binary check for logging #58
Conversation
This commit changes the order of value conversion in the ParamConverter class. It now first tries to convert values using the ToScalarInterface, and then, if that's not possible, it falls back to using the __toString method. This can potentially lead to better, more expected conversion results under certain scenarios.
The commit adds a check for UTF-8 encoding on variables within the log function in the MediaQueryLogger. If a variable does not meet this encoding, it will be converted to base 64, ensuring error-free JSON encoding.
This commit adds a check to ensure the value submitted is a string before checking for UTF-8 encoding in MediaQueryLogger. This reduces the risk of potential errors that
WalkthroughThe updates involve adding support for the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
The mbstring extension has been added as
@coderabbitai review |
Actions PerformedReview triggered.
|
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.
Actionable comments posted: 1
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- composer.json (1 hunks)
- src/MediaQueryLogger.php (2 hunks)
- src/ParamConverter.php (1 hunks)
Additional comments not posted (4)
src/MediaQueryLogger.php (2)
9-13
: Ensure the newly imported functions are used appropriately throughout the code.
33-41
: The logic to handle non-UTF-8 strings by encoding them in base64 is a good solution to the binary data issue in logs.src/ParamConverter.php (1)
38-44
: Changing the order of type checks to prioritize scalar conversion over string conversion is a good practice for data integrity.composer.json (1)
14-14
: Addingext-mbstring
to handle multi-byte strings is necessary for the new functionality in logging binary data.
@toby-griffiths What do you think? |
@koriym Looks good to me. I think the AI bot has a point about the readabilty of the comment. It's suggested reads better to me than the original. |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@NaokiTsuchiya merged! |
@toby-griffiths Do you think this interface change is still necessary? public function log(string $queryId, array $values, array $originalValues = []): void; |
Close #57
BC Berak
This PR breaks backwards compatibility ( even though it is minor) as it changes the order of processing in ParamConverter. The release is incremented by the minor version.
Summary by CodeRabbit
ext-mbstring
PHP extension for better multilingual support.