Opened 2 years ago

Closed 2 years ago

#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 2 years ago.

Download all attachments as: .zip

Change History (7)

Changed 2 years ago by aaugustin

comment:1 Changed 2 years ago by aaugustin

  • Owner changed from nobody to aaugustin

comment:2 Changed 2 years ago by claudep

  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 2 years 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 2 years ago by aaugustin

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

comment:5 Changed 2 years ago by claudep

  • Triage Stage changed from Accepted to Ready for checkin

Looks good, thanks.

comment:6 Changed 2 years 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.

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