(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
  }
}

Steps to reproduce

• log in as sysop
• create page Test1 with wikitext Test1
• create page Talk:Test1 with wikitext Talk:Test1
• create page Test2 with witkitext Test2
• create page Talk:Test2 with wikitext Talk:Test2
• Special:Move with the following settings
  • move from Test2 to Test1
  • tick "Move associated talk page"

• you will receive a warning that "The destination page "Test1" already exists. Do you want to delete it to make way for the move?"
• tick "Yes, delete the page "Test1". Move

What happens?

• Test2 moves to Test1, deleting the old Test1 (good)
• Talk:Test2 silently fails to move to Talk:Test1, deleting the old Talk:Test1 (bad, bug)

What should happen instead?

• "Yes, delete the page Test1" should probably say "Yes, delete the pages Test1 and Test2"
• Test2 moves to Test1, deleting the old Test1
• Talk:Test2 moves to Talk:Test1, and doesn't delete the old Talk:Test1

Notes

• This is probably for sysops only. Page movers have different behavior (they can move over redirects only)
• Should this ticket also update the API?
• There could be times when Test1 doesn't exist but Talk:Test1 exists. Would we need to handle that situation?

This should not happen, instead the delete and move warning should be displayed another time (first time for the page itself, second time for the talk page) for the talk page making it possible to move the page and its talk page together.

I think a better fix for this is to ask once instead of twice. See suggested new wording above of "Yes, delete the pages Test1 and Talk:Test1". Asking twice would be pretty complex.

Thanks for writing up the reproduction steps @Novem_Linguae - it made it very clear to me what this bug is. One question, in your "What should happen instead?" bullet points you wrote

Talk:Test2 moves to Talk:Test1, and doesn't delete the old Talk:Test1

I'm not sure I understand this point - wouldn't we want Talk:Test1 to be deleted so that Talk:Test2 can be moved over it, in the same way as just happened for the article?

It's been awhile and this is pretty complicated. But yeah. Maybe I got confused and the following would make more sense:

• Yes, delete the page Test1" should probably say "Yes, delete the pages Test1 and Talk:Test1"
• Test2 moves to Test1, deleting the old Test1
• Talk:Test2 moves to Talk:Test1, deleting the old Talk:Test1

What should happen if an administrator attempts to move Page B and Talk:Page B when Page A doesn't exist, but Talk:Page A does?

Building on @Novem_Linguae's open question above - what if there were three variants of the error message: Page exists, Talk page exists, and both exist?

• The destination page "PAGENAME" already exists. Do you want to delete it to make way for the move?
• The destination talk page "Talk:PAGENAME" already exists. Do you want to delete it to make way for the move?
• The destination page "PAGENAME" already exists, along with a talk page "Talk:PAGENAME". Do you want to delete them to make way for the move?

The destination page "PAGENAME" already exists. Do you want to delete it to make way for the move?

This is currently working as such regardless the presence of an accompanying talk page.

The destination talk page "Talk:PAGENAME" already exists. Do you want to delete it to make way for the move?

Breaking this down. Are you referring to the presence of Talk:PAGENAME without the presence of (Article):PAGENAME? If so, I would prefer that the system to prompt the admin to check the edit history of the talk page first. English Wikipedia don't allow unaccompanied Talk page to exist. If they do, it warrants a check before deletion.

The destination page "PAGENAME" already exists, along with a talk page "Talk:PAGENAME". Do you want to delete them to make way for the move?

Yes. To delete both destination page and accompanying talk page.

Change #1207414 had a related patch set uploaded (by Pppery; author: Pppery):

[mediawiki/core@master] Adjust handling of talk page and protections when moving pages

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

Change #1207414 had a related patch set uploaded (by Pppery; author: Pppery):

[mediawiki/core@master] Adjust handling of talk page and protections when moving pages

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

I'd like to test Pppery's new version on my Ubuntu Linux development machine which I bought and set up this past January. I should be able to do that, right? But I keep running into errors. Every time I come back to this after months off, it feels like I have to learn "hello, world" basics all over again, like it's Groundhog Day in November...

$ git pull
warning: unable to unlink 'includes/Notification/Handlers/RecentChangeNotificationHandler.php': Permission denied
warning: unable to unlink 'includes/page/Event/PageCreatedEvent.php': Permission denied
warning: unable to unlink 'includes/page/Event/PageCreatedListener.php': Permission denied
warning: unable to unlink 'includes/page/Event/PageDeletedEvent.php': Permission denied
warning: unable to unlink 'includes/page/Event/PageDeletedListener.php': Permission denied
warning: unable to unlink 'includes/page/Event/PageEvent.php': Permission denied

etc. etc.

$ composer update --no-dev
Uninstall of wikimedia/langconv failed
Uninstall of symfony/var-dumper failed
Uninstall of symfony/string failed
Uninstall of symfony/service-contracts failed

etc. etc.

$ git clone https://gerrit.wikimedia.org/r/mediawiki/core.git mediawiki
error: Your local changes to the following files would be overwritten by merge:
.eslintignore
.git-blame-ignore-revs
.gitignore
.mailmap

etc. etc.

$ git reset --hard
Aborting

$ git clean -fd

$ git pull
warning: unable to unlink 'includes/Notification/Handlers/RecentChangeNotificationHandler.php': Permission denied
warning: unable to unlink 'includes/page/Event/PageCreatedEvent.php': Permission denied
warning: unable to unlink 'includes/page/Event/PageCreatedListener.php': Permission denied
warning: unable to unlink 'includes/page/Event/PageDeletedEvent.php': Permission denied

etc. etc.

$ git pull origin master
error: Your local changes to the following files would be overwritten by merge:
.eslintignore
.git-blame-ignore-revs
.gitignore
.mailmap

etc. etc.

I have no idea why your MediaWiki clone is in a hosed state - I would suggest doing a rm -rf mediawiki (or deleting the whole directory in some other way) and recloning from scratch.

Maybe it's because I tried setting up Docker on the theory that might make things easier for me, as that seems to be the "preferred" way to do things... I'm not sure, I barely remember trying that before I got stuck again.

The first two look like they might be permissions errors, which could possibly be solved with sudo chown of the mediawiki directory.

"Your local changes to the following files would be overwritten by merge" means you changed some files to be different than what's in the git repo, and git wants you to do something with those changes first before it does other things. For example, stashing them, or git fetch origin; git reset --hard origin/master to discard all the local changes.

I think Pppery's idea of deleting everything, then re-cloning from scratch, is a good one and that might be easier than chasing down multiple errors.

Feel free to move this onwiki and ping me if you're still having trouble. I don't want to get too off topic in this ticket.

Change #1207414 merged by jenkins-bot:

[mediawiki/core@master] Adjust handling of talk page and protections when moving pages

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

I added the following to the release notes:

• (T12814) Special:MovePage can now move over existing pages when moving the talk page (following the same logic as for moving the subject page).
• (T12814) Special:MovePage will now fail loudly if you ask to move the the talk page but it couldn't be moved (rather than not moving the talk page and displaying an easy-to-miss error message)
• (T85393) Special:MovePage will now require explicit confirmation if the page you are trying to move to is protected against creation.

Similar wording would probably also make sense in Tech News.

This will be deployed in early January.

Change #1221055 had a related patch set uploaded (by Dragoniez; author: Dragoniez):

[mediawiki/core@master] Fix typo and improve consistency in move-related messages

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

Change #1221055 merged by jenkins-bot:

[mediawiki/core@master] Fix typo and improve consistency in move-related messages

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

This did not get announced for some reason, no point doing so a month later.