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

Protection template tagging breaks redirects #408

Closed
bardiharborow opened this Issue Feb 15, 2018 · 6 comments

Comments

Projects
None yet
3 participants
@bardiharborow

bardiharborow commented Feb 15, 2018

This has been happening for ages, e.g. https://en.wikipedia.org/wiki/Special:Diff/825759276

@Amorymeltzer

This comment has been minimized.

Show comment
Hide comment
@Amorymeltzer

Amorymeltzer Aug 9, 2018

Contributor

Issue is with twinkleprotect.js#L1384-L1387. Basically, the logic currently can't differentiate between a redirect and something with noinclude. The quick fix would be to rework the logic in that section so that the addition of {{ and }} to tag took place ahead of time, then turn the else if on L1387 to if.

That being said, the fancier way would be to do the above and, once we've determined the page is a redirect, ask if the page has {{Rcat shell on it. If so, do nothing, since that takes care of tags automatically. I suppose text.match(/{{Rcat/i) would do it, but there are a lot of possible redirects for that template.

Contributor

Amorymeltzer commented Aug 9, 2018

Issue is with twinkleprotect.js#L1384-L1387. Basically, the logic currently can't differentiate between a redirect and something with noinclude. The quick fix would be to rework the logic in that section so that the addition of {{ and }} to tag took place ahead of time, then turn the else if on L1387 to if.

That being said, the fancier way would be to do the above and, once we've determined the page is a redirect, ask if the page has {{Rcat shell on it. If so, do nothing, since that takes care of tags automatically. I suppose text.match(/{{Rcat/i) would do it, but there are a lot of possible redirects for that template.

@Amorymeltzer

This comment has been minimized.

Show comment
Hide comment
@Amorymeltzer

Amorymeltzer Aug 9, 2018

Contributor

{{(?:redr|this is a redirect|r(?:edirect)?(?:.?cat.*)?[ _]?sh) should take care of them all?
https://regexr.com/3to54

Contributor

Amorymeltzer commented Aug 9, 2018

{{(?:redr|this is a redirect|r(?:edirect)?(?:.?cat.*)?[ _]?sh) should take care of them all?
https://regexr.com/3to54

@MusikAnimal

This comment has been minimized.

Show comment
Hide comment
@MusikAnimal

MusikAnimal Aug 9, 2018

Collaborator

What if we check for the redirect first, and if present put the <noinclude> after the link? Something like /#REDIRECT \[\[.*?\]\](.*)/i then put the <noinclude> code at the beginning of the match (which might be empty). Or we could just put it at then end of the page entirely, but it'd be nicer to have it towards the top, in the event there is other content.

I am quite fond of checking for {{Rcat shell}}, too. We've received many a compliant about the redundant protection templates.

Collaborator

MusikAnimal commented Aug 9, 2018

What if we check for the redirect first, and if present put the <noinclude> after the link? Something like /#REDIRECT \[\[.*?\]\](.*)/i then put the <noinclude> code at the beginning of the match (which might be empty). Or we could just put it at then end of the page entirely, but it'd be nicer to have it towards the top, in the event there is other content.

I am quite fond of checking for {{Rcat shell}}, too. We've received many a compliant about the redundant protection templates.

@Amorymeltzer

This comment has been minimized.

Show comment
Hide comment
@Amorymeltzer

Amorymeltzer Aug 9, 2018

Contributor

Sorry, I think you missed a word between "put the" and "after the link?" I think you mean the tag, and after the #REDIRECT link itself? I was envisioning something like:

//untested
} else {
	tag = "{{" + tag + "}}"; //Take care of parens here
	if( params.noinclude ) { //Always check so Template-space redirects don't transclude
		text = "<noinclude>" + tag + "</noinclude>" + text;
	}
	if( Morebits.wiki.isPageRedirect() ) {
		//No {{rcat shell}} found
		if (!text.match(/{{(?:redr|this is a redirect|r(?:edirect)?(?:.?cat.*)?[ _]?sh)/i)) {
			text = text.replace(/#REDIRECT ?(\[\[.*?\]\])(.*)/i, "#REDIRECT $1$2\n\n" + tag) //Is this what you meant?
		}
	} else {
		text = tag + "\n" + text;
	}
	summary = "Adding " + params.tag + Twinkle.getPref('summaryAd');
}

FYI I threw a ? after the space before the brackets, sometimes folks don't include that.

Contributor

Amorymeltzer commented Aug 9, 2018

Sorry, I think you missed a word between "put the" and "after the link?" I think you mean the tag, and after the #REDIRECT link itself? I was envisioning something like:

//untested
} else {
	tag = "{{" + tag + "}}"; //Take care of parens here
	if( params.noinclude ) { //Always check so Template-space redirects don't transclude
		text = "<noinclude>" + tag + "</noinclude>" + text;
	}
	if( Morebits.wiki.isPageRedirect() ) {
		//No {{rcat shell}} found
		if (!text.match(/{{(?:redr|this is a redirect|r(?:edirect)?(?:.?cat.*)?[ _]?sh)/i)) {
			text = text.replace(/#REDIRECT ?(\[\[.*?\]\])(.*)/i, "#REDIRECT $1$2\n\n" + tag) //Is this what you meant?
		}
	} else {
		text = tag + "\n" + text;
	}
	summary = "Adding " + params.tag + Twinkle.getPref('summaryAd');
}

FYI I threw a ? after the space before the brackets, sometimes folks don't include that.

@Amorymeltzer

This comment has been minimized.

Show comment
Hide comment
@Amorymeltzer

Amorymeltzer Aug 10, 2018

Contributor

Just getting back to this — babies gonna baby — but nevermind. If we always put the tag below the #REDIRECT I don't think we ever need the noincludes? So instead, this should work:

} else {
	if( Morebits.wiki.isPageRedirect() ) {
		//Only tag if no {{rcat shell}} is found
		if (!text.match(/{{(?:redr|this is a redirect|r(?:edirect)?(?:.?cat.*)?[ _]?sh)/i)) {
			text = text.replace(/#REDIRECT ?(\[\[.*?\]\])(.*)/i, "#REDIRECT $1$2\n\n{{" + tag + "}}");
		} else {
			Morebits.status.info("Redirect category shell present", "nothing to do");
			return;
		}
	} else if( params.noinclude ) {
		text = "<noinclude>{{" + tag + "}}</noinclude>" + text;
	} else {
		text = "{{" + tag + "}}\n" + text;
	}
	summary = "Adding {{" + params.tag + "}}" + Twinkle.getPref('summaryAd');
}

Testing it in userspace seems to work as expected/desired. Assuming I've got the logic right, can open a PR.

Contributor

Amorymeltzer commented Aug 10, 2018

Just getting back to this — babies gonna baby — but nevermind. If we always put the tag below the #REDIRECT I don't think we ever need the noincludes? So instead, this should work:

} else {
	if( Morebits.wiki.isPageRedirect() ) {
		//Only tag if no {{rcat shell}} is found
		if (!text.match(/{{(?:redr|this is a redirect|r(?:edirect)?(?:.?cat.*)?[ _]?sh)/i)) {
			text = text.replace(/#REDIRECT ?(\[\[.*?\]\])(.*)/i, "#REDIRECT $1$2\n\n{{" + tag + "}}");
		} else {
			Morebits.status.info("Redirect category shell present", "nothing to do");
			return;
		}
	} else if( params.noinclude ) {
		text = "<noinclude>{{" + tag + "}}</noinclude>" + text;
	} else {
		text = "{{" + tag + "}}\n" + text;
	}
	summary = "Adding {{" + params.tag + "}}" + Twinkle.getPref('summaryAd');
}

Testing it in userspace seems to work as expected/desired. Assuming I've got the logic right, can open a PR.

@MusikAnimal

This comment has been minimized.

Show comment
Hide comment
@MusikAnimal

MusikAnimal Aug 10, 2018

Collaborator

Ah, thanks. GitHub tried to render <noinclude> which is why it said "put the after the link". I just added the backticks now my message is legible. You are correct that noinclude isn't even needed, so disregard!

What you have at #408 (comment) is basically what I was proposing. Feel free to make a PR and I will help test it! :)

Collaborator

MusikAnimal commented Aug 10, 2018

Ah, thanks. GitHub tried to render <noinclude> which is why it said "put the after the link". I just added the backticks now my message is legible. You are correct that noinclude isn't even needed, so disregard!

What you have at #408 (comment) is basically what I was proposing. Feel free to make a PR and I will help test it! :)

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