Do not add an extra % when escaping in SQLCommenter#631
Conversation
The formatter for SQLCommenter comments was calling `urlencode` to perform percent-escaping, and then adding an _extra_ % to each escaped value with that idea that you need a second % to escape the percent symbols themselves for SQL. But this isn't necessary, because it's inside a SQL comment (not a literal in a SQL query), and % has no special meaning there. This was noted in the old Google SQLCommenter repo: google/sqlcommenter#168 ... which specifically called out the PHP implementation as out-of-spec. And the spec also makes it clear that the extra % character is not required: https://google.github.io/sqlcommenter/spec/#key-value-format I've removed the Util method that was doing the extra % escaping, and switched `formatComments` to just call `urlencode` directly.
|
Thanks for opening your first pull request! If you haven't yet signed our Contributor License Agreement (CLA), then please do so that we can accept your contribution. A link should appear shortly in this PR if you have not already signed one. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #631 +/- ##
============================================
- Coverage 80.46% 80.45% -0.01%
+ Complexity 1669 1668 -1
============================================
Files 116 116
Lines 6265 6262 -3
============================================
- Hits 5041 5038 -3
Misses 1224 1224 Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
@Westacular there's some tests that are failing after this change. Can you look to update those too please? 👍 |
|
@ChrisLightfootWild Oh, yes, oops. Done. |
|
@jerrytfleung would you mind taking a look at these changes please? |
2 participants
The formatter for SQLCommenter comments was calling
urlencodeto perform percent-escaping, and then adding an extra % to each escaped value with that idea that you need a second % to escape the percent symbols themselves for SQL.But this isn't necessary, because it's inside a SQL comment (not a literal in a SQL query), and % has no special meaning there.
This was noted in the old Google SQLCommenter repo: google/sqlcommenter#168
... which specifically called out the PHP implementation as out-of-spec.
And the spec also makes it clear that the extra % character is not required: https://google.github.io/sqlcommenter/spec/#key-value-format
I've removed the Util method that was doing the extra % escaping, and switched
formatCommentsto just callurlencodedirectly.