Page MenuHomePhabricator

Create redirect when moving modules
Closed, ResolvedPublic

Description

When moving a module called "A" to a module called "B", Module:A should say "return require('Module:B')". Also, https://gerrit.wikimedia.org/r/#/c/146608/ should be implemented. Redirects for js and css pages have already been enabled per T73200: Support redirects in JavaScriptContent and T73201: Support redirects in CssContent.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
GTrang raised the priority of this task from to Needs Triage.Dec 8 2015, 2:13 AM
GTrang updated the task description. (Show Details)
GTrang added a project: Scribunto.
GTrang subscribed.

Change 146608 had a related patch set uploaded (by Jackmcbarn):
Add redirect support to modules

https://gerrit.wikimedia.org/r/146608

Is there any update on the status of this task? This would enable renaming a deployed module seamlessly, without having to resort to a copy-and-paste move to the new module name, followed by redirecting the old module to the new one.

See https://en.wikipedia.org/wiki/Wikipedia_talk:Lua/Archive_3#Proposal_to_restrict_page_moves_in_the_%22Module%3A%22_namespace_to_administrators for background on the request.

Change 574086 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/Scribunto@master] Add redirects for modules

https://gerrit.wikimedia.org/r/574086

As the task hasn't been updated, no.

Winston_Sung changed the task status from Open to In Progress.Apr 7 2022, 5:57 AM
WDoranWMF changed the task status from In Progress to Stalled.EditedApr 7 2022, 6:18 PM
WDoranWMF subscribed.

Just to be clear, on why I added this task to the Platform Engineering Icebox. We acknowledge the existence of this task but progress on it is unlikely. If anyone can walk through the urgency/priority that would be helpful.

Others are welcome to work on it or help to get it merged.

DannyS712 changed the task status from Stalled to In Progress.Apr 7 2022, 7:19 PM

Just to be clear, on why I added this task to the Platform Engineering Icebox. We acknowledge the existence of this task but progress on it is unlikely. If anyone can walk through the urgency/priority that would be helpful.

Others are welcome to work on it or help to get it merged.

I've rebased the patch and it is ready for review - stalled is not the correct state, since it can still be worked on even if it won't be by platform engineering. Why do you say that progress is unlikely? My patch at https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Scribunto/+/574086 implements this as requested and has tests to confirm that it works, it would be really helpful for WMF communities where module page moves now require manually creating a redirect afterwards

Change 146608 abandoned by Legoktm:

[mediawiki/extensions/Scribunto@master] Add redirect support to modules

Reason:

In favor of I405e7953d00af8a34d5e8addc61a245732c71e8e.

https://gerrit.wikimedia.org/r/146608

I left feedback on the patch, the main question I have is whether we want a preceding --[[ #REDIRECT ]] comment like we do for JS/CSS redirects.

It is completely fine to add (after empty line) such comment after statement as an explanation for such stupid one line modules.

Might be followed by timestamp, if possible, to make automatic generation more obvious and to find the event faster in version history of target page.

When all the usages and links to the old page have been updated, the redirect could be deleted, and migration process has worked without any interrupt.

It is completely fine to add (after empty line) such comment after statement as an explanation for such stupid one line modules.

Ack.

Might be followed by timestamp, if possible, to make automatic generation more obvious and to find the event faster in version history of target page.

Interesting idea, I would like to hear others' opinions on this idea and would prefer to leave it out of the initial patch.


My current thinking is that we want it to look like:

-- #REDIRECT
return require( "Module:Target" )

Yes/no?

(If we don't hear from Danny in the next few days I'll update the patch)

My current thinking is that we want it to look like:

-- #REDIRECT
return require( "Module:Target" )

I would prefer this format.

Also, would we like to support localized #REDIRECT or keep it in English?

Also, would we like to support localized #REDIRECT or keep it in English?

English is sufficient, since the audience are Lua programmers rather than encyclopedic authors. Lua is English.

My major concern is that someone encounters the module link and asks which bloody fool wrote a module of one silly line.

-- #REDIRECT (page move)

The idea is to detect all occurrencies even in currently inactive paths, then deleting that redirect.

Also, would we like to support localized #REDIRECT or keep it in English?

Currently module pages are forced to English because as PerfektesChaos notes, Lua is English. I don't think this is the ideal behavior (see e.g. T126308, T299279 as problems caused by this), but it is probably too much to take on here. If we do fix that, we could add localization for #REDIRECT afterwards, as I do disagree that "English is sufficient".

My major concern is that someone encounters the module link and asks which bloody fool wrote a module of one silly line.

People can use the page history and figure it out pretty easily.

Also, would we like to support localized #REDIRECT or keep it in English?

English is sufficient, since the audience are Lua programmers rather than encyclopedic authors. Lua is English.

My major concern is that someone encounters the module link and asks which bloody fool wrote a module of one silly line.

Given that the audience is programmers, I feel they will recognize that the module is delegating to another for its implementation.

I ended up removing -- #REDIRECT after reading Anomie's earlier code review on the patch which pointed out that it wasn't necessary (like in JS/CSS) because there's no confusion with a legitimate module.

Using something simplistic like the proposed return require( "Module:Target" ) can easily be detected by the target module, perhaps even affecting its functionality. Specifically, ... will have a value during the module initialization because the module is required and not directly #invoked and mw.getCurrentFrame():getTitle() will refer to the #invoked title and not the redirection target. Also reported script errors will list the #invoked module in the call stack because the target module is not truly treated as the Lua "main" module.

A more complete, albeit more complex, solution would be to catch the #invoke function name by using an __index metamethod and then calling mw.getCurrentFrame():getParent():callParserFunction('#invoke', {[[TargetModuleName]], funcname, ...}), providing the function name and the frame args. The tricky part would be massaging the args to include the TargetModuleName and the function name caught by the __index metamethod as well as the frame args from mw.getCurrentFrame().args.

Something along these lines:

return setmetatable({}, {
	__index = function(pkg, funcname)
		return function(frame)
			local pfargs = {[[TargetModuleName]], funcname}
			-- repack sequential args; implication: numeric parameter keys will be altered
			for i, v in ipairs(frame.args) do
				pfargs[1 + #pfargs] = v
			end
			-- copy other args; implication: accessing all the args means all their values will be expanded
			for k, v in pairs(frame.args) do
				if 'string' == type(k) then
					pfargs[k] = v
				end
			end
			return frame:getParent():callParserFunction('#invoke', pfargs)
		end
	end
})

Perhaps the best solution would be to add something to the Scribunto Lua API and just call that for the redirection then it could have PHP counter parts like many other parts of the Scribunto Lua API and could easily implement a #invoke redirection functionality which would always work and could not really be detected by any Scribunto Lua module (and thus cannot affect their functionality).

Maybe something similar to frame:extensionTag, which is essentially a specialized wrapper for callParserFunction with #tag, could be developed for #invoke allowing for the function name to be passed through in fashion like content is for frame:extensionTag. The up side of such a thing would be not needing to play with the args (which cause them to be expanded).

Another issue to consider is parsing to determine what constitutes a redirect module. There are many different Lua syntaxes that can accomplish the same thing and I imagine they would not all be detected as redirects even thought they accomplished effectively the same thing. For example even if we stick with the flawed return require( "Module:Target" ) , I could add variable to the mix to hold the interim require return value. I could change the literal string format to single quotes or one of the many possible long string literal formats. I could leave out the parentheses in the require functional call using the single literal string functional call syntax. There are many ways such a module redirect could be modified and we need to ensure that they are still detected as a valid redirect module (in much the same way as there are many ways to modify #REDIRECT wikitext redirects while they are still considered redirects).

For these reasons, I believe a dedicated Scribunto API change is much preferred for introducing module redirects (e.g., providing the funcname in the frame would go a long ways but it should be possible to cause Scribunto to just punt to another module with same funcname and frame args, possibly by just having Lua run another "main" module).

I'm sorry that I haven't handled this task. I recently returned from a long bout of unexpected inactivity, and while I plan to resume my contributions here on Phabricator its unfair to claim tasks that I might not work on when others may be interested in handling them. I'm removing myself as the assignee in a batch-action, but if someone feels that I really should be the one to handle this task feel free to re-assign me and I'll take a look.

@DannyS712 You have an open Gerrit change on this, it’d be easier if you could finish it. If you can’t or don’t want to, maybe you should also abandon the change (it can always be restored), not only unassign the task.

Change 574086 merged by jenkins-bot:

[mediawiki/extensions/Scribunto@master] Add redirects for modules

https://gerrit.wikimedia.org/r/574086

Re: Tech News - it looks like this is ready to be announced in next week's edition.
What wording and links (documentation?) would you suggest as the content? My best-guess is something like:

Changes later this week

  • It will be possible to redirect pages in the Module namespace, and moving a Module will leave a redirect behind.

But I'm uncertain about the wording, and can't see any existing documentation about it on mediawiki-wiki. Thanks!

The first part of the sentence is not entirely correct: the construct used for redirecting modules is standard Lua code, which could always be used to “redirect” modules, but they haven’t previously been recognized as such (weren’t italic on special pages, weren’t hidden from Special:UnconnectedPages etc.), so the new thing is that one particular syntax to “redirect” modules is now recognized as a redirect (plus that moving modules will leave redirects behind).

Specific suggestions for how to word it, and what to link to, would be greatly appreciated!

cscott claimed this task.

Re: Tech News - it looks like this is ready to be announced in next week's edition.
But I'm uncertain about the wording, and can't see any existing documentation about it on mediawiki-wiki. Thanks!

Added documentation at https://www.mediawiki.org/w/index.php?title=Extension%3AScribunto%2FLua_reference_manual&diff=6267770&oldid=6228438

Suggested wording:

"Full support for redirects in the Module namespace was added: the "Move Page" functionality in the sidebar will now leave an appropriate redirect behind, and such redirects are appropriately recognized (italic on special pages, hidden from Special:UnconnectedPages etc.)."

Two questions:

  1. Should the redirect page use design similar to a general redirect indicating that this is a redirect instead of continuing to use scribunto-doc-page-does-not-exist/scribunto-doc-page-show?
  2. Does not support any syntax that is different from return require [[Module:Target]] (even if the number of spaces is different return require[[Module:Target]], or quote return require "Module:Target", etc.) is a "feature" of this new functionality?
  1. Does not support any syntax that is different from return require [[Module:Target]] (even if the number of spaces is different return require[[Module:Target]], or quote return require "Module:Target", etc.) is a "feature" of this new functionality?
  • we might as well keep it a strict check, we can loosen it later if we feel it's best.
  • making it lenient in a future patch is okay

Only local namespace name is supported like "Modul" on huwiki. Please allow canonical namespace name too, like "Module".

Only local namespace name is supported like "Modul" on huwiki. Please allow canonical namespace name too, like "Module".

Could you provide an example URL?

Caused by

includes/Engines/LuaCommon/LuaEngine.php#1006
if ( $content->equals( $this->makeRedirectContent( $title ) ) ) {

which require an exact match.

It seems to me a superior solution would just be to use the existing wikitext redirects (necessitating a change in the content model upon rename/moves) and have Scribunto fetch the targets of such things before it #invokes, requires, etc.

Of course there is still the issue to potentially moving across namespaces but I am sure that could be restricted somehow.