Opened 2 years ago

Closed 2 years ago

#26440 closed Cleanup/optimization (fixed)

Add check that all items in url patterns are url instances

Reported by: Alasdair Nicol Owned by: Burhan Khalid
Component: Core (System checks) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

If you include a tuple instead of a url() instance in your urlpatterns, the checks framework throws an error which isn't very helpful.

urlpatterns = ['', # leading string might be left by users converting from patterns() in earlier Django versions
    (r'^$', my_view),
    url(r'^admin/', admin.site.urls),
]
  File "site-packages/django/core/checks/urls.py", line 67, in check_pattern_startswith_slash
    regex_pattern = pattern.regex.pattern
AttributeError: 'tuple' object has no attribute 'regex'

I suggest that we should create a new warning if the pattern is not a url() instance.

Change History (10)

comment:1 Changed 2 years ago by Alasdair Nicol

I thought this might be a good ticket if somebody's looking for a ticket during the Djangocon EU sprints. If it's not picked up, then I'll take a look at it next week.

comment:2 Changed 2 years ago by Tim Graham

Triage Stage: UnreviewedAccepted

comment:3 Changed 2 years ago by Burhan Khalid

Owner: changed from nobody to Burhan Khalid
Status: newassigned

comment:4 Changed 2 years ago by Tim Graham

Has patch: set
Patch needs improvement: set

Left comments for improvement on the PR.

comment:5 Changed 2 years ago by Alasdair Nicol

I've created a new pr, which builds on burhan's work.

https://github.com/django/django/pull/6514

comment:6 Changed 2 years ago by Tim Graham

Patch needs improvement: unset

comment:7 Changed 2 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:8 Changed 2 years ago by Alasdair Nicol

Patch needs improvement: set

Sorry for changing the patch after it had been reviewed. I've added a couple more patches to the pull request

  • Added explicit test that URL checks are recursive, and simplified other url configs
  • Added hints when users have a string or tuple instead of a url() instance

I think the text of the hints could be improved. I'm not 100% sure that the tuple hint is necessary - maybe saying the main warning 'Ensure that urlpatterns is a list' is enough.

comment:9 Changed 2 years ago by Tim Graham

Patch needs improvement: unset

comment:10 Changed 2 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In eb5d7bc2:

Fixed #26440 -- Added a warning for non-url()s in urlpatterns.

Thanks Burhan Khalid for the initial patch and knbk/timgraham
for review.

Note: See TracTickets for help on using tickets.
Back to Top