Code

Opened 11 months ago

Closed 10 months ago

Last modified 10 months ago

#20500 closed Cleanup/optimization (fixed)

Flatpages catchall URLconf example that works with APPEND_SLASH

Reported by: josh.23.french@… Owned by: EvilDMP
Component: Documentation Version: master
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'),
)

Attachments (0)

Change History (12)

comment:1 Changed 11 months ago by ByteMonk

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to ByteMonk
  • Patch needs improvement unset
  • Status changed from new to assigned

comment:2 Changed 11 months ago by claudep

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

comment:3 Changed 11 months ago by josh.23.french@…

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 follow-up: Changed 11 months ago by claudep

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?

comment:5 in reply to: ↑ 4 ; follow-up: Changed 11 months ago by 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.)


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 Changed 11 months ago by josh.23.french

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.)

comment:7 in reply to: ↑ 5 Changed 11 months ago by claudep

  • Triage Stage changed from Unreviewed to Accepted
  • Version changed from 1.4 to master

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 Changed 11 months ago by EvilDMP

  • Owner changed from ByteMonk to EvilDMP

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 Changed 11 months ago by timo

  • 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 Changed 11 months ago by josh.23.french

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 Changed 10 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 536703abf029ac1b9c2ebcf6f82fb16da524bebe:

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

Thanks josh.23.french@.

comment:12 Changed 10 months ago by Tim Graham <timograham@…>

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.