Opened 4 years ago

Closed 4 years ago

#19099 closed Bug (fixed)

Broken link email sent on redirect by i18n_patterns

Reported by: Aymeric Augustin Owned by: Aymeric Augustin
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 Aymeric Augustin 4 years ago.

Download all attachments as: .zip

Change History (7)

Changed 4 years ago by Aymeric Augustin

comment:1 Changed 4 years ago by Aymeric Augustin

Owner: changed from nobody to Aymeric Augustin

comment:2 Changed 4 years ago by Claude Paroz

Triage Stage: UnreviewedAccepted

comment:3 Changed 4 years ago by Aymeric Augustin

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 4 years ago by Aymeric Augustin

Component: InternationalizationCore (Other)
Has patch: set
Status: newassigned

comment:5 Changed 4 years ago by Claude Paroz

Triage Stage: AcceptedReady for checkin

Looks good, thanks.

comment:6 Changed 4 years ago by Aymeric Augustin <aymeric.augustin@…>

Resolution: fixed
Status: assignedclosed

In 50a985b09b439a0d52aad8694d377a3483cb02e1:

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

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