Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#20500 closed Cleanup/optimization (fixed)

Flatpages catchall URLconf example that works with APPEND_SLASH

Reported by: josh.23.french@… Owned by: Daniele Procida
Component: Documentation Version: dev
Severity: Normal Keywords: flatpages
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

In https://docs.djangoproject.com/en/dev/ref/contrib/flatpages/#using-the-urlconf the catchall example doesn't include a slash anywhere, so it catches all pages without a slash.

So we just need to add a slash. (inside the match parenthesis)

# Your other patterns here
urlpatterns += patterns('django.contrib.flatpages.views',
    (r'^(?P<url>.*/)$', 'flatpage'),
)

Change History (12)

comment:1 by ByteMonk, 11 years ago

Owner: changed from nobody to ByteMonk
Status: newassigned

comment:2 by Claude Paroz, 11 years ago

I'm not sure I follow your argumentation. Could you please add some concrete example about the problem here?

comment:3 by josh.23.french@…, 11 years ago

Here's an example urlconf without the slash:

urlpatterns = patterns('',
    url(r'^articles/$', 'news.views.archive'),
    url(r'^articles/2003/$', 'news.views.special_case_2003'),
    url(r'^articles/(\d{4})/$', 'news.views.year_archive'),
    url(r'^articles/(\d{4})/(\d{2})/$', 'news.views.month_archive'),
    url(r'^articles/(\d{4})/(\d{2})/(\d+)/$', 'news.views.article_detail'),
)
# Directly from the docs
urlpatterns += patterns('django.contrib.flatpages.views',
    (r'^(?P<url>.*)$', 'flatpage'),
)

When /articles is asked for, the flatpages pattern matches it, and if there is no /articles/ flatpage, you get a 404.

What should happen is that /articles gets redirected to /articles/ and the correct view gets processed.

The APPEND_SLASH logic only works if the sans-slash url isn't valid and a slashed one is valid. (django.middleware.common, line 71)

/articles       <-- valid, no redirect

The current pattern will match either, so no redirect to a slashed url is returned when flatpages sends a 404. (flatpages.views line 40)

By adding the slash in the urlpattern, we require that all flatpages have slashes... those that don't will be redirected by the APPEND_SLASHES logic. So /articles will redirect to /articles/, and /flatpage to /flatpage/. (All flatpages are required by admin to have slashes, so there is no problem of flatpages existing without slashes, unless somebody's doing something weird.)

Does that make more sense? Or am I missing something here?

comment:4 by Claude Paroz, 11 years ago

Thanks for the useful explanation. I would consider this as a bug in the flatpage view. In my opinion, the view should simply redirect after testing if not url.endswith('/') and settings.APPEND_SLASH:. Thoughts?

in reply to:  4 ; comment:5 by josh.23.french@…, 11 years ago

If we skip right to the redirect, we'd end up redirecting pages to 404s, which seems like something we want to keep. (196ac8f8b3)
(If it's not, ignore everything that follows.)


Keep in mind I'm completely new to Django's codebase(4 months), so the following code might be inappropriate.

This logic might be too heavy, but if we want to keep the no-redirect in cases where it'll end up a 404, it's something to think about:

from django.core import urlresolvers

if not url.endswith('/') and settings.APPEND_SLASH:
    url += '/'
    
    # Check if new url will resolve, and get the view info if it does.
    try:
        found_url = urlresolvers.resolve(url)
    except urlresolvers.Resolver404:
        raise
    
    # Check if the new url is a flatpage. If it is, skip the second half of the OR and redirect.
    if (FlatPage.objects.filter(url__exact=url, sites__id__exact=site_id).exists()
    
       # Check if the new url_name would bring us back here. (If it won't, redirect.
       # If it will, both sides of the OR are False and we raise the 404.)
       or (found_url.url_name != "django.contrib.flatpages.views.flatpage")):
       
        # Stole this from CommonMiddleware. We want the same warning... and CommonMiddleware's warning will never get raise'd.
        if settings.DEBUG and request.method == 'POST':
            raise RuntimeError((""
            "You called this URL via POST, but the URL doesn't end "
            "in a slash and you have APPEND_SLASH set. Django can't "
            "redirect to the slash URL while maintaining POST data. "
            "Change your form to point to %s (note the trailing "
            "slash), or set APPEND_SLASH=False in your Django "
            "settings.") % url)
        return HttpResponsePermanentRedirect(url)
    else:
        # No flatpage exists
        raise
else:
    raise

This is basically the APPEND_SLASH logic with a check for FlatPages specifically.

comment:6 by Joshua French, 11 years ago

The whole try/except around found_url = urlresolvers.resolve(url) is excessive. Apparently, Resolver404 is a subclass of Http404. (I couldn't edit it-I assume because I didn't have an account, so I got one.)

in reply to:  5 comment:7 by Claude Paroz, 11 years ago

Triage Stage: UnreviewedAccepted
Version: 1.4master

Replying to josh.23.french@…:

If we skip right to the redirect, we'd end up redirecting pages to 404s, which seems like something we want to keep. (196ac8f8b3)
(If it's not, ignore everything that follows.)

Good point, you are right.

Maybe you were also right from the start, telling people to add the final slash in the catch-all pattern when APPEND_SLASH is True might be the better solution.

comment:8 by Daniele Procida, 11 years ago

Owner: changed from ByteMonk to Daniele Procida

I have marked this ticket as suitable for a first-time committer attending a Don't be afraid to commit workshop.

The next planned session will be hosted by Cardiff Dev Workshop on Saturday 8th June.

If you want to tackle this ticket before then, or at any time in fact, please don't let the fact that it's assigned to me stop you. Feel free to re-assign it to yourself and do whatever you like to it.

comment:9 by Tim Graham, 11 years ago

Has patch: set

Came across a pull request. Haven't followed the discussion to determine if it's how we want to solve this.
https://github.com/django/django/pull/1221

comment:10 by Joshua French, 11 years ago

If we do want to fix it this way, that patch also needs a warning (or note) that the slash must be removed when APPEND_SLASH = False.

Something like:

You can also set it up as a "catchall" pattern. In this case, it is important
to place the pattern at the end of the other urlpatterns::

    # Your other patterns here
    urlpatterns += patterns('django.contrib.flatpages.views',
        (r'^(?P<url>.*/)$', 'flatpage'),
    )

.. warning::

    If you set APPEND_SLASH to False, you must remove the slash in the catchall
    pattern, or flatpages without a trailing slash will not be matched.

comment:11 by Tim Graham <timograham@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In 536703abf029ac1b9c2ebcf6f82fb16da524bebe:

Fixed #20500 - Updated flatpages URLconf example to work with APPEND_SLASH.

Thanks josh.23.french@.

comment:12 by Tim Graham <timograham@…>, 11 years ago

In b75f1f3d27d06e5fe4fdcc8e74ec3e7caeff7704:

[1.5.x] Fixed #20500 - Updated flatpages URLconf example to work with APPEND_SLASH.

Thanks josh.23.french@.

Backport of 536703abf0 from master

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