Refs #28526 -- Provided URLResolver namespace in technical 404 template. #19880
Conversation
sarahboyce
changed the title
django/views/debug.py
Outdated
| if isinstance(inner_tried, URLResolver): | ||
| inner_tried.debug_key_val = TriedPatternDebugInfo( | ||
| key="namespace", val=inner_tried.namespace | ||
| ) | ||
| else: | ||
| inner_tried.debug_key_val = TriedPatternDebugInfo( | ||
| key="name", val=inner_tried.name | ||
| ) |
There was a problem hiding this comment.
| if isinstance(inner_tried, URLResolver): | |
| inner_tried.debug_key_val = TriedPatternDebugInfo( | |
| key="namespace", val=inner_tried.namespace | |
| ) | |
| else: | |
| inner_tried.debug_key_val = TriedPatternDebugInfo( | |
| key="name", val=inner_tried.name | |
| ) | |
| if isinstance(inner_tried, URLResolver): | |
| inner_tried.debug_key_val = { | |
| "key": "namespace", | |
| "val": inner_tried.namespace, | |
| } | |
| else: | |
| inner_tried.debug_key_val = {"key": "name", "val": inner_tried.name} |
I wouldn't add a namedtuple unless we had to
There was a problem hiding this comment.
Agree, I think it's pretty idiomatic to pass dicts into templates.
There was a problem hiding this comment.
Terrific. Haven't written a template in a long time and simply forgot this was possible!
There was a problem hiding this comment.
I don't love that I'm polluting the underlying object, will look into adding wrappers. Would have been a lot easier if I could have registered a filter?
There was a problem hiding this comment.
Removed pollution in latest.
tests/view_tests/tests/test_debug.py
Outdated
| @override_settings(ROOT_URLCONF="view_tests.default_urls") | ||
| def test_default_urlconf_template_404(self): | ||
| response = self.client.get("/favicon.ico") | ||
| self.assertContains(response, "[namespace='admin']", status_code=404) |
There was a problem hiding this comment.
I tried to find if we could make a test a bit closer to the issue but I wasn't successful
Have tested manually that the issue is fixed and that this covered the code changes 👍
There was a problem hiding this comment.
Can you use assertInHTML and check for the full contents of the <code> tag? I think that would be a little bit clearer for future readers.
tests/view_tests/tests/test_debug.py
Outdated
| @override_settings(ROOT_URLCONF="view_tests.default_urls") | ||
| def test_default_urlconf_template_404(self): | ||
| response = self.client.get("/favicon.ico") | ||
| self.assertContains(response, "[namespace='admin']", status_code=404) |
There was a problem hiding this comment.
Can you use assertInHTML and check for the full contents of the <code> tag? I think that would be a little bit clearer for future readers.
django/views/debug.py
Outdated
| if isinstance(inner_tried, URLResolver): | ||
| inner_tried.debug_key_val = TriedPatternDebugInfo( | ||
| key="namespace", val=inner_tried.namespace | ||
| ) | ||
| else: | ||
| inner_tried.debug_key_val = TriedPatternDebugInfo( | ||
| key="name", val=inner_tried.name | ||
| ) |
There was a problem hiding this comment.
Agree, I think it's pretty idiomatic to pass dicts into templates.
jacobtylerwalls
force-pushed
the
jtw/debug-key-val
branch
from
050e6b1 to
669db1e
Compare
django/views/debug.py
Outdated
| "urlpatterns": tried, | ||
| "urlpatterns": patterns_with_debug_info, |
There was a problem hiding this comment.
I don't know whether we need to be concerned or not but we could add urlpatterns_debug to the context and leave urlpatterns alone or make sure that the structure of urlpatterns is backwards compatible for if someone was overwriting the technical_404.html template
This avoids looking up the nonexistent "name" attribute on URLResolver, which logs verbosely.
jacobtylerwalls
force-pushed
the
jtw/debug-key-val
branch
from
669db1e to
63c077e
Compare
Trac ticket number
ticket-26886 ticket-28526
Branch description
Before, the technical 404 view was written in a way that interrogates a possibly missing
.nameon a url pattern or resolver..nameonly exists onURLPattern.Now, we log something else helpful for
URLResolver, namely.namespace. This takes care of the huge traceback cited in the issue.A custom template tag might have been a nicer implementation than what I ended up with, but the technical 404 template is not inside an app, so we can't register a custom tag AFIACT.
Checklist
mainbranch.