• Bug 18260 has been marked as a duplicate of this bug. ***

Nowadays, when you move a normal page, you have the setting "Move associated talk page" autoticked (also see bug 17775).

I wonder if that influences this bug and if the overwriting of the Talk page is still silently dropped in the described case. Can anyone reproduce?

Err, 'bump'? Not really sure how Phabricator works, but this is still a bug and the problem is still the same as was described in 07 (if I'm reading things correctly).

As someone who moves a lot of pages in my administrative capacity at en.wiki this would make my life a lot easier. Is a fix feasible?

See also my post at VPT on en.wiki (https://en.wikipedia.org/w/index.php?title=Wikipedia:Village_pump_%28technical%29&oldid=673634409#Moving_over_talk_page)

@Jenks24: Do you have a specific question? :)
Currently nobody is working on this task (no assignee is set) and I'm not aware of anybody planning to work on this either.
Contributed patches are very welcome and the best way to get this task closer to resolution.

@Aklapper Thanks for the response. You've basically answered my first question, which was going to be "is anything going to happen with this bug, because I'd absolutely love it if someone could fix it?" Judging by your response I guess the answer is "not in the near future, at least". My next question would be "is there some way I can convince a developer to work on this bug?" I am happy to offer a bribe :)

Unfortunately I have next to nil technical know how and have never coded anything in my life, so fixing it myself is out.

Apologies if this isn't really what Phabricator is meant to be used for, it's just that I wasn't really sure how or where else to contact dev(s) to ask theses questions.

(I did run into catches, snags and hassles, but it can be done.)

Change 256014 had a related patch set uploaded (by Bartosz Dziewoński):
SpecialMovepage: Display warnings when talk or subpage of move target exists

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

@matmarex Sorry to be a pain, but any chance of a status/progress update for this? I tried reading the gerrit but was a bit confused to say the least. Really appreciate the work you've done on this so far.

Sorry, I haven't been working on this recently :(

Change 256014 abandoned by Bartosz Dziewoński:

[mediawiki/core@master] SpecialMovepage: Display warnings when talk or subpage of move target exists

Reason:

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

I'm tackling this as my first MediaWiki core coding project, though I haven't formally checked out the code yet. Working on understanding the (somewhat complicated) control flow.

@Catrope : back on Feb 09 2009 you added a comment (per r47042 MediaWiki - Code Review archive) // FIXME: Use Title::moveSubpages() here on line 382.
This comment is still in the current version of the code (line 847), albeit modified for the new "factory", the rationale for which I don't understand.
I'm confused about what is meant by this. As on line 849 I see if ( $this->moveSubpages && ( isn't moveSubpages() already used there? Can you clarify what the TODO wants to be done? Or remove it if it is no longer applicable. Thanks

Change #1034588 had a related patch set uploaded (by Wbm1058; author: Wbm1058):

[mediawiki/core@master] Show the page name on the MovePage checkbox for "Yes, delete the page"

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

@Wbm1058 Sorry for the late response, I was traveling and missed your message at first.

I think the confusion comes from the fact that there are two things with very similar names:

1. The SpecialMovePage class has a property called $moveSubpages, defined here. It's a boolean (true or false) and indicates whether we should move the subpages of the page we're moving. (It's set to whether the user has asked us to move subpages here, and if they've asked for it but they're not allowed to it's set back to false here.)
2. At the time I wrote that comment in 2009 (15 years ago!), the Title class has a method called moveSubpages. This method still exists, but it was moved to the MovePage class. This method contains a bunch of code that handles moving the subpages of page.

The code near my FIXME comment checks the property (#1, referred to as $this->moveSubpages) to see if it should move the subpages, and if so, it then reimplements all the logic for moving the subpages. My comment says that it shouldn't do that, it should use the method that we have for this purpose (#2, referred to as Title::movePages(), though it doesn't live in Title anymore). The method is already used in a bunch of other places that need to move subpages (though most of them use moveSubpagesIfAllowed, but that does the same thing just with extra permissions checks).

In other words, the code currently looks like this:

if ( $this->moveSubpages ) {
    // ... 100+ lines of code to do the hard work of moving the subpages
}

And my comment argues it should instead look like this:

if ( $this->moveSubpages ) {
    // Not sure if these parameters are exactly right, but something like this:
    $mp->moveSubpagesIfAllowed( $this->getAuthority(), $this->reason, $createRedirect );
}

I haven't checked in detail whether the code in SpecialMovePage is the same as the code that I propose reusing from MovePage, or whether SpecialMovePage does something special that MovePage doesn't do (I didn't check that in 2009, and it might have changed in the past 15 years). Whoever considers resolving my FIXME comment should look at that carefully.

The factory stuff is more complicated than it looks, it's just something that gives you a MovePage object. So previously, this code did something like:

// To move the page itself, you would do something like:
$oldTitle->moveTo( $newTitle, ... other params ... );
// To move subpages, you would do something like:
$oldTitle->moveSubpages( $newTitle, ... other params ... );

And now it looks like this instead:

// First create a MovePage object
$mp = $this->movePageFactory->newMovePage( $oldTitle, $newTitle );
// To move the page itself, you would do:
$mp->moveIfAllowed( ... params ... );
// To move the subpages, you would do:
$mp->moveSubpagesIfAllowed( ... params ... );

I hope that clears things up a little, feel free to ask more questions if anything is still unclear.

Thanks @Catrope. Indeed I missed the distinction between the boolean and the function. Got it.

I found API: (bug 17357) Add subpage moving to the API which is relevant to this. I don't think there should be any significant difference between moving a page using the API versus using Special:MovePage.

I'll try to be that braver soul who finally addresses this :|

Change #1034588 merged by jenkins-bot:

[mediawiki/core@master] Show the page name on the MovePage checkbox for "Yes, delete the page"

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

In T12814#9824207, @Catrope wrote:

I hope that clears things up a little, feel free to ask more questions if anything is still unclear.

Thanks. I know that 15 years is like a lifetime in IT. A quarter-century has passed already since I was last paid $ to write code (with one small exception at Wikipedia's Reward Board, ha!). I'm very much comfortable writing procedural code, not so much with object oriented.

Looking at the code to # Do the actual move. I understand that the first line
$mp = $this->movePageFactory->newMovePage( $ot, $nt );
just creates a MovePage object; nothing happens procedurally. The next line, which I'm assuming does execute some procedure, is where I'm stuck.

# check whether the requested actions are permitted / possible
$userPermitted = $mp->authorizeMove( $this->getAuthority(), $this->reason )->isOK();

This executes the authorizeMove function (err, "method"), where the first argument passed to the method $performer has a data type "Authority". I'm familiar with data types string, integer, boolean, etc. but assume this "Authority" is something defined elsewhere in the code. Searching for function getAuthority I find it defined all over the place, and it's not immediately clear which getAuthority $this->getAuthority() is.

In T12814#9840347, @Wbm1058 wrote:

In T12814#9824207, @Catrope wrote:

Looking at the code to # Do the actual move. I understand that the first line
$mp = $this->movePageFactory->newMovePage( $ot, $nt );
just creates a MovePage object; nothing happens procedurally.

Yes, that's right.

The next line, which I'm assuming does execute some procedure, is where I'm stuck.

# check whether the requested actions are permitted / possible
$userPermitted = $mp->authorizeMove( $this->getAuthority(), $this->reason )->isOK();

This executes the authorizeMove function (err, "method"), where the first argument passed to the method $performer has a data type "Authority". I'm familiar with data types string, integer, boolean, etc. but assume this "Authority" is something defined elsewhere in the code. Searching for function getAuthority I find it defined all over the place, and it's not immediately clear which getAuthority $this->getAuthority() is.

Very briefly: how exactly this is obtained is a bit convoluted, in part due to a half-finished refactor, but in practice $this->getAuthority() almost certainly returns a User object representing the currently logged-in user (or if the user is not logged in, a User object representing a logged-out IP address "user"). authorizeMove uses this to check whether the user is allowed to perform the move, by calling $performer->authorizeWrite(), which ends up invoking the authorizeWrite method in the UserAuthority class.

Longer explanation: $this refers to the current object, in this case an instance of the SpecialMovePage class, so it's SpecialMovePage's getAuthority method that gets invoked here. SpecialMovePage doesn't explicitly define a getAuthority method, but the very first line of the class says class SpecialMovePage extends UnlistedSpecialPage, which means it inherits all methods that exist in the UnlistedSpecialPage class. UnlistedSpecialPage in turn extends SpecialPage, and that does define a getAuthority method here, so that's the one that is called. That method ends up being a wrapper around the getAuthority method in IContextSource. There are different classes that implement the IContextSource interface, but the one that is almost always used is ContextSource, so the code that most probably gets called is this.

At a higher level, it mostly doesn't matter exactly how getAuthority works, what matters most is that it's going to return some sort of object that implements the Authority interface (defined here), meaning that it exposes a set of methods that let you ask whether an action is allowed.

This authority stuff is a bit more complex than it really needs to be, and it seems that Authority is synonymous with User in most situations in practice. According to this comment it's structured this way because there are future ambitions to create more separation between User (someone's identity) and Authority (what someone is allowed to do). For now, the only types of a non-User-based authority are SimpleAuthority (which has very limited, specified permissions, used in unit tests to check that having certain permissions is sufficient to take an action; for example, this lets a unit test create a scenario where someone has the move permission but nothing else) and UltimateAuthority (which is allowed to do everything, used in both real code and some unit tests to bypass permission checks). User implements all the methods that the Authority interface requires, but they're all wrappers for the UserAuthority equivalents of those methods. And in some places MovePage converts an Authority back to a User because it needs to call a hook that expects a User and hasn't been updated to expect an Authority. This is all because User and Authority have been separated but not disentangled, so there are a lot of awkward connections and entanglements between the two right now, and that makes the code more confusing and adds more layers of indirection.

In summary: the details are very confusing, but they mostly don't matter, and it's a helpful simplification (even for people who are experienced with object oriented code) to think of the return value of getAuthority as "the current user, but you can only ask it questions related to permissions". In some edge-case-y situations (mostly when the code is called by unit tests) that description might not be exactly right and you might be dealing with some other object that's pretending to be that, but we're promising you that those pretenders will be good enough at lying that you won't notice and your code will still work.

(This kind of abstraction is kind of analogous to how, back in the day before operating systems built this in, you could "print to PDF" by installing a program that lied to the OS and pretended to be a printer, but "printed" your document to a PDF file rather than to a physical device. If the program is good at pretending to be a printer, and the OS enables this by clearly defining what a printer needs to be able to do and keeping that set of requirements small, then both sides are happy, and the system as a whole is more flexible by letting you swap out pieces of it.)

Thanks @Catrope. You have a talent for explaining things at just the right level I need. Now I understand that this authority stuff isn't really where the code changes need to be made for this Phabricator. Admins/page movers have the authority; the issue is what happens after it's been determined that the move is permitted / possible.

getErrorsArray() is passed to showForm HERE -- this is getting closer to where code changes should be made.

The method Status::getErrorsArray() has been deprecated in 1.43 in favor of new method StatusValue::getMessages().

I found 1017924 Use StatusValue::getMessages() instead of deprecated methods, followup to 1007700 StatusValue: Add a getter for MessageSpecifier list.

An array of MessageSpecifier objects is easier to deal with than the "legacy error array" format, which is an array of whatever wfMessage() accepts, which can be a bunch of different things.

I changed $this->showForm( $status->getErrorsArray(), !$userPermitted ); to $this->showForm( $status->getMessages( 'error' ), !$userPermitted ); and got an internal error

[6d8c8ce84698c817c6cadb0c] /index.php?title=Special:MovePage&action=submit Error: Cannot use object of type MediaWiki\Message\Message as array

So protected function showForm still needs to be updated to replace @param (string|array)[] $err Error messages with this new object of type MediaWiki\Message\Message. And every place where function showForm is called needs to be simultaneously updated to pass the object instead of the array. I haven't found a Phabricator for finishing the cleanup of this deprecated code.

I put a couple var_dump statements in the code, with the objective of understanding the change from the "legacy error array" format → an array of MessageSpecifier objects

$status = $mp->moveIfAllowed( $this->getAuthority(), $this->reason, $createRedirect );
		if ( !$status->isOK() ) {
			var_dump($status->getErrorsArray());
			var_dump($status->getMessages( 'error' ));
			$this->showForm( $status->getMessages( 'error' ), !$userPermitted );
			return;
		}

Looking at the source output -- "Page zwei" is the name of the page I'm attempting to move to, on my test wiki (a page which exists, of course):

var_dump($status->getErrorsArray()); (the "legacy error array" format)

array(1) {
  [0]=>
  array(2) {
    [0]=>
    string(13) "articleexists"
    [1]=>
    string(9) "Page zwei"
  }
}

var_dump($status->getMessages( 'error' )); (the array of MessageSpecifier objects)

array(1) {
  [0]=>
  object(MediaWiki\Message\Message)#892 (11) {
    ["interface":protected]=>
    bool(true)
    ["language":protected]=>
    NULL
    ["userLangCallback":protected]=>
    NULL
    ["key":protected]=>
    string(13) "articleexists"
    ["keysToTry":protected]=>
    array(1) {
      [0]=>
      string(13) "articleexists"
    }
    ["overriddenKey":protected]=>
    NULL
    ["parameters":protected]=>
    array(1) {
      [0]=>
      string(9) "Page zwei"
    }
    ["useDatabase":protected]=>
    bool(true)
    ["contextPage":protected]=>
    NULL
    ["content":protected]=>
    NULL
    ["message":protected]=>
    NULL
  }
}