-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix django unicode error #2217
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
base: main
Are you sure you want to change the base?
Fix django unicode error #2217
Changes from all commits
aee0f95
b9e9922
57541b7
f35646a
02d5c70
8fd6bbe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,4 +1,4 @@ | ||||||
from django.utils.encoding import force_str | ||||||
from django.utils.encoding import DjangoUnicodeDecodeError, force_str | ||||||
from django.utils.translation import gettext_lazy as _ | ||||||
from django.views.debug import get_default_exception_reporter_filter | ||||||
|
||||||
|
@@ -24,10 +24,16 @@ def title(self): | |||||
) | ||||||
|
||||||
def generate_stats(self, request, response): | ||||||
def catch_force_errors(force_function, value): | ||||||
try: | ||||||
return force_function(value) | ||||||
except DjangoUnicodeDecodeError: | ||||||
return "Debug toolbar was unable to parse value" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
nitpick: We use the full name in other messaging presented to users. |
||||||
|
||||||
self.record_stats( | ||||||
{ | ||||||
"settings": { | ||||||
key: force_str(value) | ||||||
key: catch_force_errors(force_str, value) | ||||||
for key, value in sorted(get_safe_settings().items()) | ||||||
} | ||||||
} | ||||||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -13,6 +13,8 @@ Pending | |||||||||
class instance, regardless if any data was generated. | ||||||||||
* Fixed selenium tests for CI by using psycopg for Python 3.13 runs. | ||||||||||
* Added ``CommunityPanel`` containing links to documentation and resources. | ||||||||||
* Fixed force_str to catch error and give out default string if value is not | ||||||||||
serializable. | ||||||||||
Comment on lines
+16
to
+17
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I think we're better off trying to communicate what is going to change from a developer experience here rather than what technically changed. A user may try to figure out why they are getting this message and our documentation may not explain it. Having it here in the change log will help them realize it's intentional and be able to track what the issue/PR is to see why it was changed. |
||||||||||
|
||||||||||
6.0.0 (2025-07-22) | ||||||||||
------------------ | ||||||||||
|
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.
There are a few places we use
force_str
and I suspect we may want to swap all of them out with this new function. Perhaps we should create asanitize.py
file and put this in there with a doc string explaining the interface and what it does. Then use that everywhere we've been usingforce_str
. I'd consider keeping the name asforce_str
too, but that's just my preference. We'd want to have an explicit test covering the functionality of this new function too.There are some other sanitizing functions in
utils.py
that can be moved over too, though that should be a separate commit. Or I can do that.