Code

#19099 closed Bug (fixed)

Broken link email sent on redirect by i18n_patterns

Reported by: aaugustin Owned by: aaugustin
Component: Core (Other) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

With the following settings:

MIDDLEWARE_CLASSES = {
        # ...,
        'django.middleware.locale.LocaleMiddleware',
        'django.middleware.common.CommonMiddleware',
        # ...,
}
SEND_BROKEN_LINKS_EMAIL = True

when a redirect to an i18n'd URL occurs, a broken link email is sent.

I'm attaching a failing test case.

This may be the expected behavior, but it's hard to diagnose. I started receiving broken link emails for '/', with various referrers and UAs. But going to that URL worked and didn't send an email, because there's no referrer when you go straight to the URL.

The solution might be documenting that CommonMiddleware must come before LocaleMiddleware. The commit logs say that they used to be in that order, but that we swapped them because APPEND_SLASH didn't work (I haven't re-verified this yet). And then I started receiving the broken link emails.

Attachments (1)

19099-failing-test-case.diff (1.1 KB) - added by aaugustin 21 months ago.

Download all attachments as: .zip

Change History (7)

Changed 21 months ago by aaugustin

comment:1 Changed 21 months ago by aaugustin

  • Owner changed from nobody to aaugustin

comment:2 Changed 20 months ago by claudep

  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 19 months ago by aaugustin

After some more tests, here's what I've found.

Currently, the documentation says that LocaleMiddleware should come before CommonMiddleware. It means that during response processing, CommonMiddleware.process_response runs first, and it sends the broken link email before LocaleMiddleware.process_response gets a chance to redirect to the expected URL. (That's very similar to #1859.)

Putting CommonMiddleware before LocaleMiddleware solves the problem with broken link emails, but creates other bugs; notably, missing trailing slashes aren't appended to URLs when the prefix is a non-default language.

These results can be reproduced easily with:

  • a URLconf with i18n_patterns, such as the following:
    urlpatterns = i18n_patterns('views',
        url(r'^contact/$', 'contact'),
    )
    
  • APPEND_SLASH = True
  • LANGUAGE_CODE = 'en'
  • LANGUAGES = (('en', _('English')), ('fr', _('French')))

So, in the current state of things, i18n_patterns and SEND_BROKEN_LINK_EMAILS are incompatible.


The best solution I can think of is to move the broken link emails feature to its own middleware, say, BrokenLinksMiddleware, that should come before LocaleMiddleware.

Besides, 404 errors reporting by email sounds a bit archaic these days; does it really belong in CommonMiddleware?

It's a bit difficult to create a clean deprecation path for this change. I think CommonMiddleware.process_response could raise warnings if SEND_BROKEN_LINK_EMAILS is True but BrokenLinksMiddleware isn't in MIDDLEWARE_CLASSES. In this case, during the deprecation period, it would call BrokenLinksMiddleware.process_response.

comment:4 Changed 18 months ago by aaugustin

  • Component changed from Internationalization to Core (Other)
  • Has patch set
  • Status changed from new to assigned

comment:5 Changed 18 months ago by claudep

  • Triage Stage changed from Accepted to Ready for checkin

Looks good, thanks.

comment:6 Changed 18 months ago by Aymeric Augustin <aymeric.augustin@…>

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

In 50a985b09b439a0d52aad8694d377a3483cb02e1:

Fixed #19099 -- Split broken link emails out of common middleware.

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.