Page MenuHomePhabricator
Create Task
Maniphest T293783

ImageInfo iiprop=comment query returns empty comment as hidden
Closed, ResolvedPublicBUG REPORT
Actions

Description

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

{"batchcomplete":"","query":{"pages":{"190270":{"pageid":190270,"ns":6,"title":"File:Redsq.png","imagerepository":"local","imageinfo":[{"commenthidden":""}]}}}}

What happens?:
The API returns a commenthidden property

What should have happened instead?:
It should not return this property because the comment is not hidden, it is empty.

Software version (if not a Wikimedia wiki), browser information, screenshots, other information, etc:
Noticed this due to it causing a FileImporter bug at T293768.

Details

ProjectBranchLines +/-Subject
mediawiki/corewmf/1.38.0-wmf.5+108 -36ApiQueryImageInfo: don't show empty comments as deleted
mediawiki/corewmf/1.38.0-wmf.6+108 -36ApiQueryImageInfo: don't show empty comments as deleted
mediawiki/coremaster+108 -36ApiQueryImageInfo: don't show empty comments as deleted
mediawiki/coreREL1_37+108 -36ApiQueryImageInfo: don't show empty comments as deleted
Customize query in gerrit

Related Objects
Search...

Event Timeline

Dylsss created this task.Oct 19 2021, 2:55 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 19 2021, 2:55 PM
Dylsss updated the task description. (Show Details)Oct 19 2021, 2:59 PM
Dylsss mentioned this in T293768: FileImporter inserts "Suppressed comment removed by FileImporter." to a file revision comment when the comment is left blank and is not hidden.
Reedy added a subscriber: Reedy.Oct 19 2021, 4:15 PM

It's fairly obvious from the code why it does

		if ( ( $pcomment || $comment ) && $exists ) {
			if ( isset( $opts['revdelUser'] ) && $opts['revdelUser'] ) {
				$description = $file->getDescription( File::FOR_THIS_USER, $opts['revdelUser'] );
			} else {
				$description = $file->getDescription( File::FOR_PUBLIC );
			}
			if ( $description ) {
				if ( $pcomment ) {
					$vals['parsedcomment'] = Linker::formatComment( $description, $file->getTitle() );
				}
				if ( $comment ) {
					$vals['comment'] = $description;
				}
			} else {
				$vals['commenthidden'] = true;
				$anyHidden = true;
			}
		}

Whereas other usages of commenthidden do stuff like

			if ( $row->fa_deleted & File::DELETED_COMMENT ) {
				$file['commenthidden'] = true;
			}

If we look at LocalFile::getDescription, we probably want to make a change like it does for the guarded checks

	/**
	 * @stable to override
	 * @param int $audience
	 * @param Authority|null $performer
	 * @return string
	 */
	public function getDescription( $audience = self::FOR_PUBLIC, Authority $performer = null ) {
		$this->load();
		if ( $audience == self::FOR_PUBLIC && $this->isDeleted( self::DELETED_COMMENT ) ) {
			return '';
		} elseif ( $audience == self::FOR_THIS_USER && !$this->userCan( self::DELETED_COMMENT, $performer ) ) {
			return '';
		} else {
			return $this->description;
		}
	}

It would look like the userhidden property is similarly potentially confusing too

		if ( ( $user || $userid ) && $exists ) {
			if ( isset( $opts['revdelUser'] ) && $opts['revdelUser'] ) {
				$uploader = $file->getUploader( File::FOR_THIS_USER, $opts['revdelUser'] );
			} else {
				$uploader = $file->getUploader( File::FOR_PUBLIC );
			}
			if ( $uploader ) {
				if ( $user ) {
					$vals['user'] = $uploader->getName();
				}
				if ( $userid ) {
					$vals['userid'] = $uploader->getId();
				}
				if ( !$uploader->isRegistered() ) {
					$vals['anon'] = true;
				}
			} else {
				$vals['userhidden'] = true;
				$anyHidden = true;
			}
		}
Reedy added projects: Regression, Platform Engineering, MW-1.37-release.Oct 19 2021, 4:30 PM
Reedy added a subscriber: • Pchelolo.

It looks like this is potentially a regression from rMW5455e5896782: Deprecate File::getUser in favor of File::getUploader. CC @Pchelolo and tagging Platform

• Pchelolo edited projects, added Platform Team Workboards (MW Expedition); removed Platform Engineering.Oct 19 2021, 4:47 PM
• Pchelolo claimed this task.Oct 19 2021, 4:59 PM
Reedy added a parent task: T293768: FileImporter inserts "Suppressed comment removed by FileImporter." to a file revision comment when the comment is left blank and is not hidden.Oct 19 2021, 5:00 PM
gerritbot added a comment.Oct 26 2021, 5:18 PM

Change 734700 had a related patch set uploaded (by Ppchelko; author: Ppchelko):

[mediawiki/core@master] ApiQueryImageInfo: don't show empty comments as deleted

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

gerritbot added a project: Patch-For-Review.Oct 26 2021, 5:18 PM
gerritbot added a comment.Oct 26 2021, 7:38 PM

Change 734680 had a related patch set uploaded (by Reedy; author: Ppchelko):

[mediawiki/core@REL1_37] ApiQueryImageInfo: don't show empty comments as deleted

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

gerritbot added a comment.Oct 26 2021, 7:56 PM

Change 734681 had a related patch set uploaded (by Reedy; author: Ppchelko):

[mediawiki/core@wmf/1.38.0-wmf.6] ApiQueryImageInfo: don't show empty comments as deleted

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

gerritbot added a comment.Oct 26 2021, 7:56 PM

Change 734682 had a related patch set uploaded (by Reedy; author: Ppchelko):

[mediawiki/core@wmf/1.38.0-wmf.5] ApiQueryImageInfo: don't show empty comments as deleted

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

gerritbot added a comment.Oct 26 2021, 7:59 PM

Change 734700 merged by jenkins-bot:

[mediawiki/core@master] ApiQueryImageInfo: don't show empty comments as deleted

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

ReleaseTaggerBot added a project: MW-1.38-notes (1.38.0-wmf.7; 2021-11-02).Oct 26 2021, 8:00 PM
gerritbot added a comment.Oct 26 2021, 8:03 PM

Change 734680 merged by jenkins-bot:

[mediawiki/core@REL1_37] ApiQueryImageInfo: don't show empty comments as deleted

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

Reedy closed this task as Resolved.Oct 26 2021, 8:44 PM
gerritbot added a comment.Oct 26 2021, 8:54 PM

Change 734681 merged by jenkins-bot:

[mediawiki/core@wmf/1.38.0-wmf.6] ApiQueryImageInfo: don't show empty comments as deleted

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

gerritbot added a comment.Oct 26 2021, 8:54 PM

Change 734682 merged by jenkins-bot:

[mediawiki/core@wmf/1.38.0-wmf.5] ApiQueryImageInfo: don't show empty comments as deleted

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

ReleaseTaggerBot edited projects, added MW-1.37-notes, MW-1.38-notes (1.38.0-wmf.5; 2021-10-19); removed MW-1.38-notes (1.38.0-wmf.7; 2021-11-02).Oct 26 2021, 9:00 PM
Stashbot added a comment.Oct 26 2021, 9:00 PM

Mentioned in SAL (#wikimedia-operations) [2021-10-26T21:00:41Z] <reedy@deploy1002> Synchronized php-1.38.0-wmf.5/includes/api/ApiQueryImageInfo.php: T293783 (duration: 01m 03s)

Stashbot added a comment.Oct 26 2021, 9:01 PM

Mentioned in SAL (#wikimedia-operations) [2021-10-26T21:01:45Z] <reedy@deploy1002> Synchronized php-1.38.0-wmf.6/includes/api/ApiQueryImageInfo.php: T293783 (duration: 01m 03s)

Stashbot added a comment.Oct 26 2021, 9:03 PM

Mentioned in SAL (#wikimedia-operations) [2021-10-26T21:03:26Z] <reedy@deploy1002> Synchronized php-1.38.0-wmf.6/tests/phpunit/includes/api/query/ApiQueryImageInfoTest.php: T293783 (duration: 01m 02s)

Stashbot added a comment.Oct 26 2021, 9:04 PM

Mentioned in SAL (#wikimedia-operations) [2021-10-26T21:04:38Z] <reedy@deploy1002> Synchronized php-1.38.0-wmf.5/tests/phpunit/includes/api/query/ApiQueryImageInfoTest.php: T293783 (duration: 01m 02s)

Reedy mentioned this in T300462: Empty file comment shown as deleted on Special:Undelete.Jan 30 2022, 4:52 PM
Content licensed under Creative Commons Attribution-ShareAlike 3.0 (CC-BY-SA) unless otherwise noted; code licensed under GNU General Public License (GPL) or other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL