[Django] #31330: Flatpage catchall is killing APPEND_SLASH functionality on normal pages
Django
-------------------------------------+-------------------------------------
Reporter: | Owner: nobody
meesterguyman |
Type: | Status: new
Uncategorized |
Component: | Version: 3.0
Uncategorized | Keywords: append_slash,
Severity: Normal | flatpage, catchall
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
We would like to append slashes to all URLs that do not have them, and we
would also like to have a catchall flatpage expression, so our editors can
enter whatever they like in that area of the CMS. The problem is that the
flatpage validator adds the slash, but then only checks to see if a
Flatpage exists for the resultant path
(https://github.com/django/django/blob/master/django/contrib/flatpages/views.py#L41),
whereas the is_valid_path method
(https://github.com/django/django/blob/b9cf764be62e77b4777b3a75ec256f6209a57671/django/urls/base.py#L150)
checks the urlconf, of which the catchall is a part, returns True, because
the non-slash url fits the catchall. Hence, it does not redirect to the
slashed version, making it impossible to reach and resulting in a 404.
Again, here is the flow:
1. Attempt to reach "/my-valid-page-without-a-slash"
2. is_valid_path returns True because "/my-valid-page-without-a-slash"
fits the flatpage catchall and hence does not append a slash
3. The page is not found in Flatpage entries, resulting in 404
It seems to me that if APPEND_SLASH is set to True, then we should not be
looking in the urlconf for the page without the slash, but should instead
immediately append the slash, then check the urlconf, in which case the
correct pattern would be found and flatpages would never be called.
--
Ticket URL: <https://code.djangoproject.com/ticket/31330>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: contrib.flatpages | Version: 3.0
Severity: Normal | Resolution:
Keywords: append_slash, | Triage Stage:
flatpage, catchall | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by meesterguyman):
* type: Uncategorized => Bug
* component: Uncategorized => contrib.flatpages
* easy: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/31330#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: meesterguyman | Owner: nobody
Type: Bug | Status: new
Component: contrib.flatpages | Version: 3.0
Severity: Normal | Resolution:
Keywords: append_slash, | Triage Stage:
flatpage, catchall | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by felixxm):
Thanks for this ticket, I think there is a regression introduced in
df41b5a05d4e00e80e73afe629072e37873e767a in `flatpages`
[https://docs.djangoproject.com/en/3.0/ref/contrib/flatpages/#using-the-
urlconf docs], i.e. a "catchall" pattern should contain a trailing slash
when you use `APPEND_SLASH`:
{{{
urlpatterns += [
path('<path:url>/', views.flatpage),
]
}}}
Can you check your app with this change?
--
Ticket URL: <https://code.djangoproject.com/ticket/31330#comment:2>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: assigned
Component: contrib.flatpages | Version: 3.0
Severity: Normal | Resolution:
Keywords: append_slash, | Triage Stage:
flatpage, catchall | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* owner: nobody => Rohit Jha
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/31330#comment:3>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: contrib.flatpages | Version: 3.0
Severity: Normal | Resolution:
Keywords: append_slash, | Triage Stage:
flatpage, catchall | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Rohit Jha):
* owner: Rohit Jha => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/31330#comment:4>
Django
-------------------------------------+-------------------------------------
| Kulkarni
Type: Bug | Status: assigned
Component: contrib.flatpages | Version: 3.0
Severity: Normal | Resolution:
Keywords: append_slash, | Triage Stage:
flatpage, catchall | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* owner: nobody => Ninad Kulkarni
* status: new => assigned
Comment:
I would like to work on this ticket but could someone please guide me
about how to solve this issue?
--
Ticket URL: <https://code.djangoproject.com/ticket/31330#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: meesterguyman | Owner: Ninad
| Kulkarni
Type: Bug | Status: assigned
Component: contrib.flatpages | Version: 3.0
Severity: Normal | Resolution:
Keywords: append_slash, | Triage Stage:
flatpage, catchall | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by felixxm):
Ninad, there is nothing to work on for a moment. Ticket is still not
accepted, we're waiting for confirmation from the reporter that it's a
documentation regression.
--
Ticket URL: <https://code.djangoproject.com/ticket/31330#comment:4>
Django
example.
-------------------------------------+-------------------------------------
Reporter: meesterguyman | Owner: Ninad
| Kulkarni
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: append_slash, | Triage Stage: Accepted
flatpage, catchall |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* component: contrib.flatpages => Documentation
* stage: Unreviewed => Accepted
Comment:
meesterguyman, I understand that you're not using
`contrib.flatpages.forms.FlatpageForm` because in such case trailing slash
is required when `APPEND_SLASH` is True. I confirmed that when you allow
for URLs without a trailing slash and want to use a “catchall” pattern
then a trailing slash is required in:
{{{
path('<path:url>/', views.flatpage)
}}}
It's a regression in df41b5a05d4e00e80e73afe629072e37873e767a.
--
Ticket URL: <https://code.djangoproject.com/ticket/31330#comment:5>
Django
example.
-------------------------------------+-------------------------------------
Reporter: meesterguyman | Owner: Ninad
| Kulkarni
Type: Bug | Status: assigned
Component: Documentation | Version: 3.0
Severity: Normal | Resolution:
Keywords: append_slash, | Triage Stage: Accepted
flatpage, catchall |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by felixxm):
Ninad, we need to add a trailing slash to
[https://github.com/django/django/blob/7e15795bf06d362f20257d2e9db378ba8940dc39/docs/ref/contrib/flatpages.txt#L82
flatpages URLconf example].
--
Ticket URL: <https://code.djangoproject.com/ticket/31330#comment:6>
Django
example.
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: append_slash, | Triage Stage: Accepted
flatpage, catchall |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by felixxm):
* owner: Ninad Kulkarni => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/31330#comment:7>
Django
example.
-------------------------------------+-------------------------------------
| Ramezani
Type: Bug | Status: assigned
Component: Documentation | Version: 3.0
Severity: Normal | Resolution:
Keywords: append_slash, | Triage Stage: Accepted
flatpage, catchall |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* owner: (none) => Hasan Ramezani
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/31330#comment:8>
Django
example.
-------------------------------------+-------------------------------------
Reporter: meesterguyman | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Component: Documentation | Version: 3.0
Severity: Normal | Resolution:
Keywords: append_slash, | Triage Stage: Accepted
flatpage, catchall |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Hasan Ramezani):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/31330#comment:9>
Django
example.
-------------------------------------+-------------------------------------
Reporter: meesterguyman | Owner: Hasan
| Ramezani
Component: Documentation | Version: 3.0
Severity: Normal | Resolution: fixed
Keywords: append_slash, | Triage Stage: Accepted
flatpage, catchall |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"a0916d7212aaae634f4388d47d8717abc2cd9036" a0916d7]:
{{{
#!CommitTicketReference repository=""
revision="a0916d7212aaae634f4388d47d8717abc2cd9036"
Fixed #31330 -- Updated flatpages URLconf example to work with
APPEND_SLASH.
Regression in df41b5a05d4e00e80e73afe629072e37873e767a.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31330#comment:10>
Django
example.
-------------------------------------+-------------------------------------
Reporter: meesterguyman | Owner: Hasan
| Ramezani
Type: Bug | Status: closed
Component: Documentation | Version: 3.0
Severity: Normal | Resolution: fixed
Keywords: append_slash, | Triage Stage: Accepted
flatpage, catchall |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"6831049fc20689010deb445d402a376322fb471c" 6831049]:
{{{
#!CommitTicketReference repository=""
revision="6831049fc20689010deb445d402a376322fb471c"
[3.0.x] Fixed #31330 -- Updated flatpages URLconf example to work with
APPEND_SLASH.
Regression in df41b5a05d4e00e80e73afe629072e37873e767a.
Backport of a0916d7212aaae634f4388d47d8717abc2cd9036 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31330#comment:11>
Django
example.
-------------------------------------+-------------------------------------
Reporter: meesterguyman | Owner: Hasan
| Ramezani
Component: Documentation | Version: 3.0
Severity: Normal | Resolution:
flatpage, catchall |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: fixed =>
Comment:
I think the addition of the slash causes a different problem.
Any valid page URL will match the catch all pattern, but the actual URL
that is captured will not have the last slash in it. This will cause the
flatpages view code to fail to find the page, add a slash to the URL and
then return a permanent redirect to that URL with an extra slash on the
end of it. So if the URL is '/someurl/', the user will be redirected to
'/someurl//'.
I think the last slash needs to be inside the captured part of the URL,
which can be done with re_path, like this:-
{{{
re_path('^(?P<url>.+/)$', views.flatpage)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31330#comment:12>
Django
example.
-------------------------------------+-------------------------------------
Reporter: meesterguyman | Owner: Hasan
| Ramezani
Type: Bug | Status: new
Component: Documentation | Version: 3.0
Severity: Normal | Resolution:
flatpage, catchall | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
Comment:
Simon, Thanks.
[https://github.com/django/django/pull/12629 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/31330#comment:13>
Django
example.
-------------------------------------+-------------------------------------
Reporter: meesterguyman | Owner: Hasan
| Ramezani
Component: Documentation | Version: 3.0
Severity: Normal | Resolution: fixed
Keywords: append_slash, | Triage Stage: Ready for
flatpage, catchall | checkin
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"8f2a6c76d19e4010c4683b20ed7f1eb4b07c17eb" 8f2a6c76]:
{{{
#!CommitTicketReference repository=""
revision="8f2a6c76d19e4010c4683b20ed7f1eb4b07c17eb"
Fixed #31330 -- Corrected catchall URL pattern in flatpages docs.
Use re_path() pattern with the regex used before original regression in
df41b5a05d4e00e80e73afe629072e37873e767a.
Regression in a0916d7212aaae634f4388d47d8717abc2cd9036.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31330#comment:14>
Django
example.
-------------------------------------+-------------------------------------
Reporter: meesterguyman | Owner: Hasan
| Ramezani
Type: Bug | Status: closed
Component: Documentation | Version: 3.0
Severity: Normal | Resolution: fixed
Keywords: append_slash, | Triage Stage: Ready for
flatpage, catchall | checkin
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Carlton Gibson <carlton.gibson@…>):
In [changeset:"3bb1e6c5048f6d8cc6c9bd373799e08955c8aac0" 3bb1e6c]:
{{{
#!CommitTicketReference repository=""
revision="3bb1e6c5048f6d8cc6c9bd373799e08955c8aac0"
[3.0.x] Fixed #31330 -- Corrected catchall URL pattern in flatpages docs.
Use re_path() pattern with the regex used before original regression in
df41b5a05d4e00e80e73afe629072e37873e767a.
Regression in a0916d7212aaae634f4388d47d8717abc2cd9036.
Backport of 8f2a6c76d19e4010c4683b20ed7f1eb4b07c17eb from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31330#comment:15>