Opened 9 years ago

Closed 8 years ago

#3228 closed enhancement (fixed)

Added SmartSlashMiddleware

Reported by: mihai_preda@… Owned by: mtredinnick
Component: Core (Other) Version: master
Severity: normal Keywords: APPEND_SLASH middleware
Cc: jesse.lovelace@…, silassewell@…, andy-django@…, real.human@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Implements a new way of 'append slash redirect', as described here:
http://groups.google.com/group/django-developers/browse_thread/thread/d6ca6e1442658598

This patch requires some of the small refactorings from ticket #3224.

Attachments (5)

smartslash.diff (1.0 KB) - added by mihai_preda@… 9 years ago.
SmartSlashMiddleware
smartslash-2.diff (7.6 KB) - added by andy-django@… 8 years ago.
changes.diff (1.9 KB) - added by Andy Gayton <andy-django@…> 8 years ago.
changes since smartslash-2.diff was created
smartslash-3.diff (10.2 KB) - added by Andy Gayton <andy-django@…> 8 years ago.
smartslash-3.1.diff (10.2 KB) - added by Andy Gayton <andy-django@…> 8 years ago.
fixed typo in docs/middleware.txt

Download all attachments as: .zip

Change History (28)

Changed 9 years ago by mihai_preda@…

SmartSlashMiddleware

comment:1 Changed 9 years ago by SmileyChris

  • Triage Stage changed from Unreviewed to Design decision needed

comment:2 Changed 8 years ago by mtredinnick

  • Needs documentation set

I'm +0 on this, I guess. If we do use it, I think it should replace the existing APPEND_SLASH behaviour in CommonMiddleware, rather than becoming yet another option. It looks like it will work fairly smoothly in cases where APPEND_SLASH is currently being used. However, we will need the facility to disable it entirely, just as with APPEND_SLASH.

comment:3 Changed 8 years ago by gsf@…

+1 on this patch replacing the existing APPEND_SLASH behavior. When I want to create a URL without an end slash, I shouldn't have to set a flag that breaks the admin interface. This patch puts the control of URLs where it should be -- in urls.py.

comment:4 Changed 8 years ago by anonymous

  • Cc jesse.lovelace@… added

comment:5 Changed 8 years ago by anonymous

  • Cc silassewell@… added

comment:6 Changed 8 years ago by anonymous

  • Cc andy-django@… added

comment:7 Changed 8 years ago by mrmachine

  • Cc real.human@… added

+1 on this from me too. We should be able to define URLS that specifically do or do not end in a slash in urls.py, instead of having to choose one or the other and depending on files containing a period.

comment:8 Changed 8 years ago by andy-django@…

+1 something like this would help me out a lot as well

comment:9 Changed 8 years ago by Simon G. <dev@…>

  • Patch needs improvement set
  • Summary changed from [patch] Added SmartSlashMiddleware to Added SmartSlashMiddleware

Can one of the +1 commenters here want to fix up the current patch to trunk and implement Malcolm's suggestions above?

comment:10 Changed 8 years ago by andy-django@…

Just checking - Malcolm's suggestions are to:

  • port smartslash.diff to replace the existing APPEND_SLASH behaviour in CommonMiddleware?
  • and to provide a patch for the docs?

Are there any backwards compatibility concerns about changing the current behaviour?

Changed 8 years ago by andy-django@…

comment:11 Changed 8 years ago by andy-django@…

Attached is a new patch to replace the existing APPEND_SLASH behaviour in CommonMiddleware, with unit tests.

If this looks ok, I'll do up a patch as well for the docs.

cheers,

comment:12 Changed 8 years ago by andy-django@…

Just a quick note, smartslash-2.diff doesn't require the refactorings from ticket #3224.

comment:13 Changed 8 years ago by cablehead

Gentle bump for review - we're using this patch over at http://rst2a.com - it'd be great to get something which has this effect back into trunk.

Cheers!

comment:14 Changed 8 years ago by jacob

  • Triage Stage changed from Design decision needed to Ready for checkin

comment:15 Changed 8 years ago by mtredinnick

Two small problems before this can be checked in:

  1. lines 59-69 (the exception message) won't work as expected. It's not a multi-line string. Wrap things in parentheses or something.
  2. There should be a small documentation change to middleware.txt included to describe the new behaviour.

comment:16 Changed 8 years ago by mtredinnick

  • Triage Stage changed from Ready for checkin to Accepted

comment:17 Changed 8 years ago by Andy Gayton <andy-django@…>

Hey Malcolm, I can get those things fixed up in the next couple of hours if you like.

Not sure if you have started on these fixes yet?

I did an svn up the other day, and got some conflicts on this patch. I think they need to be resolved as well.

comment:18 Changed 8 years ago by mtredinnick

Andy: I'm not fixing it, so go ahead.

comment:19 Changed 8 years ago by Andy Gayton <andy-django@…>

  • Owner changed from nobody to cablehead

Changed 8 years ago by Andy Gayton <andy-django@…>

changes since smartslash-2.diff was created

comment:20 Changed 8 years ago by Andy Gayton <andy-django@…>

  • damn sorry, overlooked the lines 59-69 the, i've added a test for it and fixed
  • docs updated
  • resolved conflicts
    • http.get_host(request) updated to request.get_host()
    • added test for #5762 and reapplied the urlquote change
  • now making use of tests/urls.py which gets rid of the URLCONF hackery in the middleware test setUp

Changed 8 years ago by Andy Gayton <andy-django@…>

Changed 8 years ago by Andy Gayton <andy-django@…>

fixed typo in docs/middleware.txt

comment:21 Changed 8 years ago by Andy Gayton <andy-django@…>

  • Needs documentation unset
  • Owner changed from cablehead to mtredinnick
  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready for checkin

comment:22 Changed 8 years ago by Andy Gayton <andy-django@…>

Hope this is good to go, any issues, just assign back to me.

comment:23 Changed 8 years ago by mtredinnick

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

(In [6852]) Fixed #3228 -- Added new APPEND_SLASH handling behaviour in the common middleware. Makes customisation a bit easier. Thanks, Mihai Preda and Andy Gayton.

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