Opened 8 years ago
Closed 8 years ago
#27386 closed Cleanup/optimization (fixed)
Readonly callable field is unconditionally wrapped inside <p>...</p>, which might create invalid HTML
Reported by: | Jacob Rief | Owned by: | nobody |
---|---|---|---|
Component: | contrib.admin | Version: | dev |
Severity: | Normal | Keywords: | callable field is_readonly <p> |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
(Pseudo)-Fields in classes inheriting from django.contrib.admin.ModelsAdmin
which are callables, must be listed in readonly_fields
. This implies that in admin/includes/fieldset.html
(line 17) and admin/edit_inline/tabular.html
(line 55) the content of this field is wrapped inside a paragraph <p>{{ field.contents }}</p>
.
However, a <p>...</p>
is not suitable to accept every kind of HTML element. Therefore when using a "callable" field, which renders it's content in HTML, one might get a surprising result.
Since the author of a callable field may wrap it's content into whatever (s)he likes, there should be a way to avoid these wrapping paragraphs.
My proposal is to check if field.contents
is safe text, and if so then leave it as-is, or otherwise wrap it into <p>..</p>
as we do it right now.
Change History (9)
comment:1 by , 8 years ago
Keywords: | is_readonly <p> added |
---|
comment:2 by , 8 years ago
Keywords: | callable added |
---|
comment:3 by , 8 years ago
Description: | modified (diff) |
---|---|
Summary: | Callable field is wrapped inside <p>...</p> → Readonly callable field is unconditionally wrapped inside <p>...</p>, which might create invalid HTML |
Triage Stage: | Unreviewed → Accepted |
comment:4 by , 8 years ago
Yes, changing this could add a minor backwards compatibility problem; something probably not really noticeable and if, easy to fix.
I looked at the code. We presumably should add another tag to django.contrib.admin.helpers.AdminReadonlyField.__init__
named is_callable
which is True
for callable fields. Then in the templates, we'd have to add another {% if field.is_callable %}
and only wrap the content inside <p>..</p>
it that's false.
If this proposal is accepted, I will implement it right now.
comment:5 by , 8 years ago
I'm not sure. It doesn't seem obviously correct to assume that all callables might have HTML.
comment:6 by , 8 years ago
Has patch: | set |
---|---|
Needs tests: | set |
Resolution: | → fixed |
Status: | new → closed |
PR https://github.com/django/django/pull/7477 to fix this ticket is pending.
comment:7 by , 8 years ago
Needs tests: | unset |
---|---|
Resolution: | fixed |
Status: | closed → new |
comment:8 by , 8 years ago
Patch needs improvement: | set |
---|
Some comments for improvement are on the PR.
I'm not sure if the proposal is completely backwards-compatible, but the problem seems real.