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

Already on GitHub? Sign in to your account

Fix/1019 #1053

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
Collaborator

ojii commented Oct 14, 2011

however it's really insanely hard to cover edge-cases (especially for
home-pages) since it's not trivial to check if a page will be the
homepage or not.

Jonas Obrist Started working on #1019, it should prevent the issue in most cases,
however it's really insanely hard to cover edge-cases (especially for
home-pages) since it's not trivial to check if a page will be the
homepage or not.
bf448a1
Collaborator

ojii commented Oct 14, 2011

This partially fixes #1019

Collaborator

ojii commented Oct 17, 2011

given #1019 only arises with bad user input, could we agree on partial fixes for that since it's insanely hard to prevent this in a reasonable fashion for all cases?

Contributor

fivethreeo commented Oct 18, 2011

Do it like this for now and add more as possible edge cases crop up?

Collaborator

ojii commented Oct 18, 2011

I had an alternative idea:

Don't actually do checking (or only VERY basic checking) on creation (eg only existing pages and just check against get_absolute_url) but do the checking in the view: If the page has a redirect and it would redirect to itself (request.path == redirect OR '%s://%s%s' % ('https' if request.is_secure() else 'http', request.get_host(), request.path), don't redirect but show the page.

Thoughts?

Contributor

fivethreeo commented Oct 18, 2011

That would work, and maybe mail a report to the managers?

Collaborator

ojii commented Oct 18, 2011

Not sure if they would want all those mails

Contributor

fivethreeo commented Oct 18, 2011

True. Will it be that many? Django reports 404's the same way.

It is sort of a misconfiguration, but might not be serious enough to warrant a mail.

Contributor

kezabelle commented Oct 18, 2011

Why not raise an InfiniteRedirect error or something, and leave the decision of what to do about notifying the developers to the logging configuration (in 1.3, prior to which it can do the email thing anyway)

addendum:

if it is implemented in the view, maybe do it as a function in utils, so that if it does warrant moving (or calling) elsewhere in the code, it can do so in a clean manner?

Collaborator

ojii commented Oct 18, 2011

If we raise an exception in the view, that's not really nice... warning maybe?

@ojii ojii referenced this pull request Oct 18, 2011

Merged

Fix 1019 2 #1061

Collaborator

ojii commented Oct 18, 2011

Closing this in favor of #1061

@ojii ojii closed this Oct 18, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment