Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#27238 closed Cleanup/optimization (fixed)

Disable check_pattern_startswith_slash if settings.APPEND_SLASH=False

Reported by: Mathieu Comandon Owned by: Alasdair Nicol
Component: Core (System checks) Version: 1.10
Severity: Normal Keywords:
Cc: Alasdair Nicol Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

I would like to propose the removal of the check_pattern_startswith_slash system check as there are legitimate reasons to have url patterns starting with a slash and this warning can be misleading.

The Django framework has a strong bias in favor of trailing slashes in URLs but not everyone wishes to set up their urls that way. If this warning was to be respected, it's impossible to have urls without a trailing slash in some situations:

# myapp. urls
urlpatterns = [
    url(r'^dashboard', include('dashboard.urls')),
]

# dashboard.urls
urlpatterns = [
    url(r'^$', views.dashboard_home, name='dashboard_home'),
    url(r'^/users$', views.users, name='dashboard_users'),
]

The above URLconfs allow to have the URLs /dashboard and /dashboard/users but it will produce the warning.

When trying to make the slash optional in the root URLconf with r'^dashboard/?', the 2nd url will reverse to /dashboardusers.

I know that some system checks can be silenced but I'd be in favor of the complete removal of the check since it can be misleading for users who do not wish trailing slashes in their URLs.

Change History (6)

comment:1 Changed 2 years ago by Tim Graham

Cc: Alasdair Nicol added

The original ticket is #23813. Maybe it's enough to disable the check if settings.APPEND_SLASH = False?

Last edited 2 years ago by Alasdair Nicol (previous) (diff)

comment:2 Changed 2 years ago by Alasdair Nicol

We could change the check_resolver method so that it passes the regex when called recursively.

  • If we call check_resolver(resolver, r'^dashboard') then we shouldn't run check_pattern_startswith_slash. This would prevent the false positives for strycore's example.
  • If we call check_resolver(resolver, r'^dashboard/') or check_resolver(resolver, None) (initial call) then it is ok to run check_pattern_startswith_slash.

I haven't tried writing a patch for this yet. Modifying the check_resolver method like this might be overly complex. I like the simplicity of checking settings.APPEND_SLASH as Tim suggested.

comment:3 Changed 2 years ago by Tim Graham

Easy pickings: set
Summary: Removal of check_pattern_startswith_slashDisable check_pattern_startswith_slash if settings.APPEND_SLASH=False
Triage Stage: UnreviewedAccepted

comment:4 Changed 2 years ago by Alasdair Nicol

Has patch: set
Owner: changed from nobody to Alasdair Nicol
Status: newassigned

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

Resolution: fixed
Status: assignedclosed

In 911d9f4e:

Fixed #27238 -- Disabled check_pattern_startswith_slash if settings.APPEND_SLASH=False.

Thanks strycore for the report and timgraham for suggesting the
solution.

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

In 190cd0e4:

[1.10.x] Fixed #27238 -- Disabled check_pattern_startswith_slash if settings.APPEND_SLASH=False.

Thanks strycore for the report and timgraham for suggesting the
solution.

Backport of 911d9f4ed1a39f945769b7198a419850378f9824 from master

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