[https://github.com/wrwrwr/django/compare/dropped/redirect-with-view-name-
loops A few tests] that are not part of the proposed changes, but
demonstrate how one could try to test for loops (currently 3 of these just
past, while a browser would run into a loop, and 2 actually loop
indefinitely).
--
Ticket URL: <https://code.djangoproject.com/ticket/23682>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
* needs_better_patch: => 0
* has_patch: 0 => 1
* needs_tests: => 0
* needs_docs: => 0
Comment:
[https://github.com/wrwrwr/django/compare/ticket_23682 Proposed changes]:
* defined a new `CircularRedirectError`;
* changed the loop detection to throw the exception on repeated URLs and
chains longer than 20;
* added a "redirect to self" test with a changing query argument.
Could all the loop prevention tests maybe live in `test_client` rather
than `test_client_regress`?
--
Ticket URL: <https://code.djangoproject.com/ticket/23682#comment:1>
* type: Uncategorized => New feature
* stage: Unreviewed => Accepted
Comment:
It seems reasonable to me as infinite redirects could indeed be something
a developer would want to be notified about. Skimming the original ticket
where this was added (#4476), I couldn't find anything about why the
decision was made.
Combining the `test_client` and `test_client_regress` tests would probably
be welcome, but making it a separate commit/ticket is best.
--
Ticket URL: <https://code.djangoproject.com/ticket/23682#comment:2>
Comment (by prestontimmons):
I reviewed and tested this patch. It looks good to me, although the import
statements should reflect Django's convention of being alphabetized.
Do you want to create a pull request from your branch? This has the nice
side-effect of triggering an automated test run and verifying the merge
status.
--
Ticket URL: <https://code.djangoproject.com/ticket/23682#comment:3>
Comment (by wrwrwr):
Thanks. Did some imports reordering (see also #23860) and created
[https://github.com/django/django/pull/3537 a pull request].
--
Ticket URL: <https://code.djangoproject.com/ticket/23682#comment:4>
* needs_better_patch: 0 => 1
* needs_docs: 0 => 1
Comment:
Thanks for adding the pull request. I left a few comments on it.
--
Ticket URL: <https://code.djangoproject.com/ticket/23682#comment:5>
Comment (by wrwrwr):
Thanks again; rebased, undid the unnecessary reordering. added docs on the
new exception and a point to the 1.8 release notes.
--
Ticket URL: <https://code.djangoproject.com/ticket/23682#comment:6>
* needs_better_patch: 1 => 0
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/23682#comment:7>
Comment (by prestontimmons):
This patch looks good to me.
--
Ticket URL: <https://code.djangoproject.com/ticket/23682#comment:8>
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/23682#comment:9>
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"056a3c6c374f15e23746ea8568cd5b11bfe7d442"]:
{{{
#!CommitTicketReference repository=""
revision="056a3c6c374f15e23746ea8568cd5b11bfe7d442"
Fixed #23682 -- Enhanced circular redirects detection in tests.
When the test client detects a redirect to a URL seen in the
currently followed chain it will now raise a RedirectCycleError
instead of just returning the first repeated response.
It will also complain when a single chain of redirects is longer
than 20, as this often means a redirect loop with varying URLs,
and even if it's not actually one, such long chains are likely
to be treated as loops by browsers.
Thanks Preston Timmons, Berker Peksag, and Tim Graham for reviews.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23682#comment:10>