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

Redirect module #1593

Closed
wants to merge 4 commits into from
Closed

Redirect module #1593

wants to merge 4 commits into from

Conversation

Randomno
Copy link
Contributor

@Randomno Randomno commented Jul 2, 2023

#268

Technically it works but it's garbage. I couldn't even figure out how to get the base URL and redirect to the specified path rather than a whole website. That's the only change that would be necessary to be merged (because it'll change how the module is used), but the whole thing should be redone.

Current usage: [module:redirect|page=https://tasvideos.org/SandBox]
Desired usage: [module:redirect|page=SandBox]

@adelikat
Copy link
Collaborator

adelikat commented Jul 2, 2023

To get teh baseurl, add AppSettings to the constructor of your class, then it will be in the BaseUrl property of that class

@Randomno Randomno changed the title Redirect module (#268) Redirect module ( #268 ) Jul 3, 2023
@Randomno Randomno marked this pull request as ready for review July 3, 2023 07:30
@Randomno
Copy link
Contributor Author

Randomno commented Jul 3, 2023

Works as intended now. Still very minimal implementation and could do with the other features mentioned in #268 (comment) in the future. 2 I can do, others I have no idea.

@Masterjun3
Copy link
Collaborator

How are infinite redirects prevented?

@Randomno
Copy link
Contributor Author

Randomno commented Jul 3, 2023

By the browser from what I tested

image

@Masterjun3
Copy link
Collaborator

I know we would avoid putting in loops, so it'd be rare, but I don't know if I'm comfortable trusting all browser behaviors to properly catch stuff like that. The problem is that it'd be a massive resource drain if any one doesn't stop it.

Btw, the Preview button gives an error alert if you redirect to a page that doesn't exist. Which is probably fine, it's just interesting that the 404 that comes from the PageNotFound goes directly into the JS fetch Preview request, and so it thinks something didn't work.

@Masterjun3 Masterjun3 changed the title Redirect module ( #268 ) Redirect module Jul 3, 2023
@Randomno
Copy link
Contributor Author

Randomno commented Jul 3, 2023

Valid concern and I don't know how to address it. Check the contents of the target page and refuse to redirect if it also contains a redirect? Add something to the target URL like ?redirected=yes and ignore the next redirect if the URL contains it?

@Masterjun3
Copy link
Collaborator

I personally don't think a module for redirection is a proper solution. For me the modules are part of the content of wiki pages, and should not control the behavior of the page itself.

Personally my idea for a redirection would be a new database table, containing every redirect. Then we could add something like a /Wiki/Redirects page where people with proper permissions can create, edit, and delete wiki redirects. That's where we can then put in logic to prevent loops, and put in automation to create proper redirects when pages are moved.

@Randomno
Copy link
Contributor Author

Randomno commented Jul 3, 2023

Fixed repeated redirects.

As for the implementation, I'm not a big fan of it for the same reason you gave. I'm basing it on the MediaWiki syntax of

#REDIRECT [[Page]]

This issue has been around for a long time now and it's holding back wiki reorganisation.

@Randomno
Copy link
Contributor Author

Thoughts?

@adelikat
Copy link
Collaborator

Closing because this has not seen any progress in awhile, and may not be the direction we want to go to handle redirects

@adelikat adelikat closed this Jan 14, 2024
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.

None yet

3 participants