Page MenuHomePhabricator

Odd looking status handling code in GadgetHooks::onEditFilterMergedContent()
Closed, ResolvedPublic

Description

Looks like we have some incorrect variable reuse/clashing

	public static function onEditFilterMergedContent( $context, $content, $status, $summary ) {
		$title = $context->getTitle();

		if ( !$title->inNamespace( NS_GADGET_DEFINITION ) ) {
			return true;
		}

		if ( !$content instanceof GadgetDefinitionContent ) {
			// This should not be possible?
			throw new Exception(
				"Tried to save non-GadgetDefinitionContent to {$title->getPrefixedText()}"
			);
		}

		$status = $content->validate();
		if ( !$status->isGood() ) {
			$status->merge( $status );
			return false;
		}

		return true;
	}

Specifically, $status->merge( $status ); -- How can you merge the object into itself? I'm guessing $status = $content->validate(); should be something like $status2 and then if it's not good, merging one into the other

		$status2 = $content->validate();
		if ( !$status2->isGood() ) {
			$status->merge( $status2 );
			return false;
		}

Introduced by @Legoktm and @kaldari in https://github.com/wikimedia/mediawiki-extensions-Gadgets/commit/519f30355ec2677ce8c2872bd2688bf1159c08ff it seems

Event Timeline

Reedy created this task.May 14 2018, 10:49 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 14 2018, 10:49 PM
Reedy updated the task description. (Show Details)May 14 2018, 10:50 PM
Reedy added subscribers: kaldari, Legoktm.

Change 433093 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[mediawiki/extensions/Gadgets@master] Fix variable name reuse in EditFilterMergedContent hook

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

Change 433093 merged by jenkins-bot:
[mediawiki/extensions/Gadgets@master] Fix variable name reuse in EditFilterMergedContent hook

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

Legoktm closed this task as Resolved.May 15 2018, 1:02 AM
Vvjjkkii renamed this task from Odd looking status handling code in GadgetHooks::onEditFilterMergedContent() to 4ycaaaaaaa.Jul 1 2018, 1:10 AM
Vvjjkkii reopened this task as Open.
Vvjjkkii removed Legoktm as the assignee of this task.
Vvjjkkii triaged this task as High priority.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed subscribers: gerritbot, Aklapper.
CommunityTechBot renamed this task from 4ycaaaaaaa to Odd looking status handling code in GadgetHooks::onEditFilterMergedContent().Jul 2 2018, 4:11 PM
CommunityTechBot closed this task as Resolved.
CommunityTechBot assigned this task to Legoktm.
CommunityTechBot raised the priority of this task from High to Needs Triage.
CommunityTechBot updated the task description. (Show Details)
CommunityTechBot added subscribers: gerritbot, Aklapper.