From 3e2259be9302e5a7f0a2a12d8a98319127ec8940 Mon Sep 17 00:00:00 2001 From: Petr Pchelko Date: Wed, 2 Jun 2021 09:48:33 -0700 Subject: [PATCH] ArchivedFile: replace ::getUser methods with ::getUploader Change-Id: I43ea7be80e8546e217d6aa371bb48f0699129cc4 --- RELEASE-NOTES-1.37 | 3 + includes/filerepo/file/ArchivedFile.php | 59 +++++++++++++-- .../revisiondelete/RevDelArchivedFileItem.php | 12 ++-- includes/specials/SpecialUndelete.php | 12 ++-- .../includes/filerepo/file/LocalFileTest.php | 71 ++++++++++++++++++- 5 files changed, 139 insertions(+), 18 deletions(-) diff --git a/RELEASE-NOTES-1.37 b/RELEASE-NOTES-1.37 index 6aa1a673e34..ca4cb552a5a 100644 --- a/RELEASE-NOTES-1.37 +++ b/RELEASE-NOTES-1.37 @@ -375,6 +375,9 @@ because of Phabricator reports. use 'by' property with UserIdentity value instead. - 'by' property with blocker's ID, use 'by' property with UserIdentity value instead. +* ArchivedFile::getUser, ::getRawUser, ::getRawUserText were deprecated in + favor of ::getUploader. ::getRawDescription was deprecated in favor of + ::getDescription with RAW audience parameter. * … === Other changes in 1.37 === diff --git a/includes/filerepo/file/ArchivedFile.php b/includes/filerepo/file/ArchivedFile.php index b29621a13d0..25fc5c3832e 100644 --- a/includes/filerepo/file/ArchivedFile.php +++ b/includes/filerepo/file/ArchivedFile.php @@ -24,6 +24,7 @@ use MediaWiki\MediaWikiServices; use MediaWiki\Permissions\Authority; use MediaWiki\Revision\RevisionRecord; +use MediaWiki\User\UserIdentity; /** * Class representing a row of the 'filearchive' table @@ -32,6 +33,12 @@ use MediaWiki\Revision\RevisionRecord; * @ingroup FileAbstraction */ class ArchivedFile { + + // Audience options for ::getDescription() and ::getUploader() + public const FOR_PUBLIC = 1; + public const FOR_THIS_USER = 2; + public const RAW = 3; + /** @var int Filearchive row ID */ private $id; @@ -68,7 +75,7 @@ class ArchivedFile { /** @var string Upload description */ private $description; - /** @var User|null Uploader */ + /** @var UserIdentity|null Uploader */ private $user; /** @var string Time of upload */ @@ -511,16 +518,18 @@ class ArchivedFile { * @note Prior to MediaWiki 1.23, this method always * returned the user id, and was inconsistent with * the rest of the file classes. + * @deprecated since 1.37. Use ::getUploader instead. * @param string $type 'text', 'id', or 'object' * @return int|string|User|null * @throws MWException * @since 1.31 added 'object' */ public function getUser( $type = 'text' ) { + wfDeprecated( __METHOD__, '1.37' ); $this->load(); if ( $type === 'object' ) { - return $this->user; + return $this->user ? User::newFromIdentity( $this->user ) : null; } elseif ( $type === 'text' ) { return $this->user ? $this->user->getName() : ''; } elseif ( $type === 'id' ) { @@ -530,15 +539,46 @@ class ArchivedFile { throw new MWException( "Unknown type '$type'." ); } + /** + * @since 1.37 + * @stable to override + * @param int $audience One of: + * File::FOR_PUBLIC to be displayed to all users + * File::FOR_THIS_USER to be displayed to the given user + * File::RAW get the description regardless of permissions + * @param Authority|null $performer to check for, only if FOR_THIS_USER is + * passed to the $audience parameter + * @return UserIdentity|null + */ + public function getUploader( int $audience = self::FOR_PUBLIC, Authority $performer = null ): ?UserIdentity { + $this->load(); + if ( $audience === self::FOR_PUBLIC && $this->isDeleted( File::DELETED_USER ) ) { + return null; + } elseif ( $audience === self::FOR_THIS_USER && !$this->userCan( File::DELETED_USER, $performer ) ) { + return null; + } else { + return $this->user; + } + } + /** * Return upload description. * - * @return string|int + * @since 1.37 the method takes $audience and $performer parameters. + * @param int $audience One of: + * File::FOR_PUBLIC to be displayed to all users + * File::FOR_THIS_USER to be displayed to the given user + * File::RAW get the description regardless of permissions + * @param Authority|null $performer to check for, only if FOR_THIS_USER is + * passed to the $audience parameter + * @return string */ - public function getDescription() { + public function getDescription( int $audience = self::FOR_PUBLIC, Authority $performer = null ): string { $this->load(); - if ( $this->isDeleted( File::DELETED_COMMENT ) ) { - return 0; + if ( $audience === self::FOR_PUBLIC && $this->isDeleted( File::DELETED_COMMENT ) ) { + return ''; + } elseif ( $audience === self::FOR_THIS_USER && !$this->userCan( File::DELETED_COMMENT, $performer ) ) { + return ''; } else { return $this->description; } @@ -547,29 +587,34 @@ class ArchivedFile { /** * Return the user ID of the uploader. * + * @deprecated since 1.37. Use ::getUploader with self::RAW audience. * @return int */ public function getRawUser() { + wfDeprecated( __METHOD__, '1.37' ); return $this->getUser( 'id' ); } /** * Return the user name of the uploader. * + * @deprecated since 1.37. Use ::getUploader with self::RAW audience. * @return string */ public function getRawUserText() { + wfDeprecated( __METHOD__, '1.37' ); return $this->getUser( 'text' ); } /** * Return upload description. * + * @deprecated since 1.37. Use ::getDescription with self::RAW audience. * @return string */ public function getRawDescription() { + wfDeprecated( __METHOD__, '1.37' ); $this->load(); - return $this->description; } diff --git a/includes/revisiondelete/RevDelArchivedFileItem.php b/includes/revisiondelete/RevDelArchivedFileItem.php index 3eda27e356d..3a710e839ff 100644 --- a/includes/revisiondelete/RevDelArchivedFileItem.php +++ b/includes/revisiondelete/RevDelArchivedFileItem.php @@ -129,15 +129,17 @@ class RevDelArchivedFileItem extends RevDelFileItem { ), ]; } - if ( $file->userCan( RevisionRecord::DELETED_USER, $user ) ) { + $uploader = $file->getUploader( ArchivedFile::FOR_THIS_USER, $user ); + if ( $uploader ) { $ret += [ - 'userid' => $file->getUser( 'id' ), - 'user' => $file->getUser( 'text' ), + 'userid' => $uploader->getId(), + 'user' => $uploader->getName(), ]; } - if ( $file->userCan( RevisionRecord::DELETED_COMMENT, $user ) ) { + $comment = $file->getDescription( ArchivedFile::FOR_THIS_USER, $user ); + if ( $comment ) { $ret += [ - 'comment' => $file->getRawDescription(), + 'comment' => $comment, ]; } diff --git a/includes/specials/SpecialUndelete.php b/includes/specials/SpecialUndelete.php index c64e6afdb80..7f38bb8d6e9 100644 --- a/includes/specials/SpecialUndelete.php +++ b/includes/specials/SpecialUndelete.php @@ -1269,14 +1269,15 @@ class SpecialUndelete extends SpecialPage { * @return string HTML fragment */ private function getFileUser( $file ) { - if ( !$file->userCan( File::DELETED_USER, $this->getUser() ) ) { + $uploader = $file->getUploader( File::FOR_THIS_USER, $this->getAuthority() ); + if ( !$uploader ) { return '' . $this->msg( 'rev-deleted-user' )->escaped() . ''; } - $link = Linker::userLink( $file->getRawUser(), $file->getRawUserText() ) . - Linker::userToolLinks( $file->getRawUser(), $file->getRawUserText() ); + $link = Linker::userLink( $uploader->getId(), $uploader->getName() ) . + Linker::userToolLinks( $uploader->getId(), $uploader->getName() ); if ( $file->isDeleted( File::DELETED_USER ) ) { $link = '' . $link . ''; @@ -1292,12 +1293,13 @@ class SpecialUndelete extends SpecialPage { * @return string HTML fragment */ private function getFileComment( $file ) { - if ( !$file->userCan( File::DELETED_COMMENT, $this->getUser() ) ) { + $comment = $file->getDescription( File::FOR_THIS_USER, $this->getAuthority() ); + if ( !$comment ) { return '' . $this->msg( 'rev-deleted-comment' )->escaped() . ''; } - $link = Linker::commentBlock( $file->getRawDescription() ); + $link = Linker::commentBlock( $comment ); if ( $file->isDeleted( File::DELETED_COMMENT ) ) { $link = '' . $link . ''; diff --git a/tests/phpunit/includes/filerepo/file/LocalFileTest.php b/tests/phpunit/includes/filerepo/file/LocalFileTest.php index c645af4879f..e05f26c3c86 100644 --- a/tests/phpunit/includes/filerepo/file/LocalFileTest.php +++ b/tests/phpunit/includes/filerepo/file/LocalFileTest.php @@ -390,6 +390,39 @@ class LocalFileTest extends MediaWikiIntegrationTestCase { return $file; } + private function getArchivedFileWithDeletion( + UserIdentity $uploader, + int $deletedFlags + ): ArchivedFile { + return ArchivedFile::newFromRow( (object)[ + 'fa_id' => 1, + 'fa_storage_group' => 'test', + 'fa_storage_key' => 'bla', + 'fa_name' => 'Random-11m.png', + 'fa_archive_name' => 'Random-11m.png', + 'fa_size' => 10816824, + 'fa_width' => 1000, + 'fa_height' => 1800, + 'fa_metadata' => '', + 'fa_bits' => 16, + 'fa_media_type' => 'BITMAP', + 'fa_major_mime' => 'image', + 'fa_minor_mime' => 'png', + 'fa_description_id' => $this->getServiceContainer() + ->getCommentStore() + ->createComment( $this->db, 'comment' )->id, + 'fa_actor' => $this->getServiceContainer() + ->getActorStore() + ->acquireActorId( $uploader, $this->db ), + 'fa_user' => $uploader->getId(), + 'fa_user_text' => $uploader->getName(), + 'fa_timestamp' => $this->db->timestamp( '20201105235242' ), + 'fa_sha1' => 'sy02psim0bgdh0jt4vdltuzoh7j80ru', + 'fa_deleted' => $deletedFlags, + ] + ); + } + /** * @dataProvider providePermissionChecks * @covers LocalFile::getUploader @@ -410,13 +443,49 @@ class LocalFileTest extends MediaWikiIntegrationTestCase { /** * @dataProvider providePermissionChecks - * @covers LocalFile::getDescription + * @covers ArchivedFile::getDescription */ public function testGetDescription( Authority $performer, int $audience, int $deleted, bool $expected + ) { + $file = $this->getArchivedFileWithDeletion( $performer->getUser(), $deleted ); + if ( $expected ) { + $this->assertSame( 'comment', $file->getDescription( $audience, $performer ) ); + } else { + $this->assertSame( '', $file->getDescription( $audience, $performer ) ); + } + } + + /** + * @dataProvider providePermissionChecks + * @covers ArchivedFile::getUploader + */ + public function testArchivedGetUploader( + Authority $performer, + int $audience, + int $deleted, + bool $expected + ) { + $file = $this->getArchivedFileWithDeletion( $performer->getUser(), $deleted ); + if ( $expected ) { + $this->assertTrue( $performer->getUser()->equals( $file->getUploader( $audience, $performer ) ) ); + } else { + $this->assertNull( $file->getUploader( $audience, $performer ) ); + } + } + + /** + * @dataProvider providePermissionChecks + * @covers LocalFile::getDescription + */ + public function testArchivedGetDescription( + Authority $performer, + int $audience, + int $deleted, + bool $expected ) { $file = $this->getOldLocalFileWithDeletion( $performer->getUser(), $deleted ); if ( $expected ) {