#19577 closed Bug (fixed)
admin doc may encourage bad practices
| Reported by: | Owned by: | nobody | |
|---|---|---|---|
| Component: | Documentation | Version: | 1.4 |
| Severity: | Normal | Keywords: | |
| Cc: | Florian Apolloner | Triage Stage: | Ready for checkin |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
On https://docs.djangoproject.com/en/1.4/ref/contrib/admin/ two places example code demonstrate allow_tags=True in order to return HTML fragments containing <span> tags. Inside these tags data (self.first_name and self.last_name) is not escaped/quoted.
As such example code often is copy/pasted it probably should reflect "best practices"?
Attachments (2)
Change History (11)
comment:1 by , 12 years ago
| Cc: | added |
|---|---|
| Triage Stage: | Unreviewed → Accepted |
by , 12 years ago
| Attachment: | 19577.diff added |
|---|
comment:2 by , 12 years ago
| Has patch: | set |
|---|
comment:3 by , 12 years ago
The indendation is wrong at https://code.djangoproject.com/attachment/ticket/19577/19577.diff#L45 and you don't need format_html at https://code.djangoproject.com/attachment/ticket/19577/19577.diff#L63 (there is no allow_tags=True) -- otherwise it looks good to me.
comment:4 by , 12 years ago
| Patch needs improvement: | set |
|---|
by , 12 years ago
| Attachment: | 19577.2.diff added |
|---|
comment:5 by , 12 years ago
| Patch needs improvement: | unset |
|---|
Thanks for taking a look. I don't think the example is meant to assume that get_full_address() returns a safe string, but maybe I interpreted it differently. I added a comment to try to explain what I thought the intent of the example was in addition to the other fixes you mentioned (plus documenting format_html_join).
comment:7 by , 12 years ago
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
Yes, this should definitely use format_html (https://docs.djangoproject.com/en/dev/ref/utils/#django.utils.html.format_html). Do you think you can wipe up a patch for that?