Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #24713 -- Redirect loop detection in test client is incorrect #4591

Conversation

mounirmesselmeni
Copy link
Contributor

Raising RedirectCycleError only when already on the same view more than twice.
If the count should be other than 2 please let me know.

Raising RedirectCycleError only when already on the same view more than twice
@timgraham
Copy link
Member

A test is also needed.

@timgraham
Copy link
Member

Not sure why you closed the pull request -- do you plan to complete it or did you abandon the issue?

@mounirmesselmeni
Copy link
Contributor Author

After what we discussed on Google group I think the issue need to be reviewed may be it does not make sense to fix it because of the usage example shown on the ticket.

@timgraham
Copy link
Member

I didn't think we reached that conclusion. Could you explain why?

@mounirmesselmeni
Copy link
Contributor Author

Yes, first thing making multiple redirect for a client I don't think it's the right way.
Let take an example: A user is trying to access a home page "/home" and the server need to update an information, let's say a last_visit datetime field or something else, do we need to redirect him to "/update" make this update and redirect him again to "/home". I thing it's better to run a signal, an asynchronous task, make the update by calling a model method or something else or even do the logic from the "/home" view.
If we are not sure that this is a common issue for many (now it look like only one person have this problem) I think it's better to keep things like they are.

@timgraham
Copy link
Member

I agree that the use case seems a bit obscure. I asked for further details from the reporter on the ticket. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants