Loop detection refactoring

[jhedstrom]

Attempt to address #16.

[Berdir]

I don't think this is correct, it is global for a reason. You can have a loop that involves more than one redirect. In fact, those are the common cases. Something like content/title -> content/old-title -> content/title.

It could even involve another module, so for example, you might have a redirect from the alias to node/5, and then node/5 is being redirected away again by globalredirect. Stuff like that.

[jhedstrom]

Hmm, it completely blows up on load testing (of different URLs from same
IP), so at the very least should be configurable.
On Apr 30, 2015 3:17 PM, "Sascha Grossenbacher" [email protected]
wrote:

I don't think this is correct, it is global for a reason. You can have a
loop that involves more than one redirect. In fact, those are the common
cases. Something like content/title -> content/old-title -> content/title.

It could even involve another module, so for example, you might have a
redirect from the alias to node/5, and then node/5 is being redirected away
again by globalredirect. Stuff like that.

—
Reply to this email directly or view it on GitHub
#21 (comment).

[Berdir]

I see, load testing of redirects, I assume?

I think the best option is to go with option b) in #16. That basically means that when we find a matching redirect, we pass the redirect that we create again through findMatchingRedirect, repeatedly until we find one that doesn't find one. That won't find all cases as mentioned above, but it should find some.

Making it configurable should also be fine, but might not be necessary with this approach?

[jhedstrom]

This makes detection configurable. Option b in #16 sounds good, but I don't have the bandwidth to pursue that at the moment.

This will also be an issue with web crawlers (eg, Google), not just load testing.

[jhedstrom]

This latest attempt removes flood-based detection, and pursues option b in #16.

[jhedstrom]

This should fix the failing tests. I added tests for the loop detection as well.