Page MenuHomePhabricator

LocalFileMoveBatch's $logger parameter to protected to enable NSFileRepo extension to continue to work
Closed, ResolvedPublicBUG REPORT

Description

List of steps to reproduce (step by step, including full links if applicable):

  • ensure the NSFileRepo extension is installed (I know, but bear with me please)
  • upload a file and try to move it to another namespace (which is the main feature of NSFileRepo)
  • an error will be returned involving $this->logger->debug on line 192 of includes/filerepo/file/LocalFileMoveBatch.php

What happens?:
The error mentioned results from the fact that the NSFileRepo's NSLocalFile->move() function cannot set the $logger parameter of the LocalFileMoveBatch parent class.
This stems from the fact that LocalFileMoveBatch defines the $logger parameter as private rather than protected.

What should have happened instead?:
The NSFileRepo based file move should still be able to work (despite the lack of maintenance to this extension).
To fix it, all I'm asking is for the $logger parameter of the LocalFileMoveBatch class (includes/filerepo/file/LocalFileMoveBatch.php) to be set to protected rather than private.
Once this is done, NSFileRepo can work again (and I can confirm that I have succeeded with that).

Software version (if not a Wikimedia wiki), browser information, screenshots, other information, etc.:
Currently using 1.37.1, but I know this problem also existed in 1.36.

Event Timeline

Ammarpad added a subscriber: Ammarpad.

There's no mention of $logger in the said method, includes/filerepo/file/NSLocalFile.php#L322-L355 (in fact in the whole repo), so changing the property visibility will not fix the actual problem.

$logger is probably null at that stage because a subclass at NSLocalFileMoveBatch#L424 defines its constructor and does not call the parent one — which sets the property. I think this would be fixed if the extension's class call its parent constructor

Good point! I've tested this and can confirm the following change to extensions/NSFileRepo/includes/filerepo/file/NSLocalFile.php starting at line 429 works.
So to be clear, my original proposition of altering mediawiki's LocalFileMoveBatch class is *not* necessary, indeed.
Apologies for my misunderstanding of the correct solution direction.

class NSLocalFileMoveBatch extends LocalFileMoveBatch {
	/**
	 * @param LocalFile $file
	 * @param Title $target
	 */
	public function __construct( LocalFile $file, Title $target ) {
		parent::__construct( $file, $target );
		$this->oldRel = $this->oldHash . NSLocalFile::getFileNameStripped( $this->oldName );
		$this->newRel = $this->newHash . NSLocalFile::getFileNameStripped( $this->newName );
	}

Change 763500 had a related patch set uploaded (by Llanddewi; author: Llanddewi):

[mediawiki/extensions/NSFileRepo@master] component: solves debug error when moving file to a different namespace

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

Change 763983 had a related patch set uploaded (by Robert Vogel; author: Llanddewi):

[mediawiki/extensions/NSFileRepo@REL1_35] component: solves debug error when moving file to a different namespace

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

Change 763500 merged by jenkins-bot:

[mediawiki/extensions/NSFileRepo@master] component: solves debug error when moving file to a different namespace

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

Thanks for the contribution. I have merged it into master. Currently checking LTS branch REL1_35.

Change 763983 merged by jenkins-bot:

[mediawiki/extensions/NSFileRepo@REL1_35] component: solves debug error when moving file to a different namespace

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

Osnard claimed this task.

Change has been merged into master and REL1_35 (LTS). Thanks!