Page MenuHomePhabricator

General CR and cleanup of CollaborationKit
Closed, ResolvedPublic

Description

CollaborationListContent.php

  • Two instances of this, but $context is never used
						$context = wfEscapeWikiText( substr( $part, 30 ) );
						if ( strlen( $context ) === 30 ) {
							$context .= '...';
						}
  • Line 964, $column is undefined
  • sortUsersIntoColumns is $column actually an array? $column->items usage suggests it's not...
  • Line 601 $itemTags; what is this to do?
  • Line 28, typo "og" should be "of"
  • Line 111-114 replace with return ( $value == 'members' || $value == 'normal' || $value == 'error' );
  • Lines 724 and 725 are the same array key
	private function matchesTag( array $tagSpecifier, array $itemTags ) {
		if ( !$tagSpecifier ) {
			return true;
		}
		$matchesAllGroups = true;
		foreach ( $tagSpecifier as $tagGroups ) {
			foreach ( $tagGroups as $tagAlt ) {
				$matchesOneAlternative = false;
				$itemTags;
				if ( in_array( $tagAlt, $itemTags ) ) {
					$matchesOneAlternative = true;
					break;
				}
			}
			if ( !$matchesOneAlternative ) {
				$matchesAllGroups = false;
				break;
			}
		}
		return $matchesAllGroups;
	}
  • matchesOneAlternative is questionably defined

CollaborationHubContent.php

  • Lines 857 and 858 are the same array key

ext.CollaborationKit.list.edit.js

  • cur = getCurrentJson - cur isn't used anywhere else. Is this assignment needed?

CollaborationHubContentEditor.php

		foreach ( CollaborationHubContent::getThemeColours() as $colour ) {
			$colours['collaborationkit-' . $colour] = $colour;
		}
  • Possibly list all messages in full above for grep-ability?

SpecialCreateCollaborationHub.php

  • Does all the commented out code need to stay?

CollaborationHubContent.php

	public function convertToHumanEditable() {
		$this->decode();

		$output = $this->displayName;
		$output .= self::HUMAN_DESC_SPLIT;
		$output .= $this->introduction;
		$output .= self::HUMAN_DESC_SPLIT;
		$output .= $this->footer;
		$output .= self::HUMAN_DESC_SPLIT;
		$output .= $this->image;
		$output .= self::HUMAN_DESC_SPLIT;
		$output .= $this->themeColour;
		$output .= self::HUMAN_DESC_SPLIT;
		$output .= $this->getHumanEditableContent();
		return $output;
	}
  • There seems to be a couple of similar implementations of the above?

CollaborationHubContentHandler.php

	/**
	 * @return CollaborationHubContent
	 */
	public function makeEmptyContent() {
		return new CollaborationHubContent( '{ "display_name": "", "introduction": "", "footer": "", "content": [] }' );
	}
  • Can't decide if this should be a php array, and passed through json_encode or something for readability...

CollaborationListContent/CollaborationHubContent

  • Identical implementations of escapeForHumanEditable and unescapeForHumanEditable

General

  • JS files missing "use strict", I know it's not compulsory, but generally recommended...
  • Various rather loooong lines. I notice Generic.Files.LineLength.TooLong disabled in phpcs.xml too
  • UsageException is deprecated
  • Commented out code blocks. Remove where appropriate. If using it to disable a feature, return a flag that disables it instead?

Related Objects

Event Timeline

Change 339958 had a related patch set uploaded (by Reedy):
Various PHP cleanup of CollaborationKit

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

CollaborationHubContent.php
Lines 857 and 858 are the same array key

CollaborationListContent.php
Lines 724 and 725 are the same array key

Subtle difference: double-quotes vs. single quotes. "\n" thus refers to a newline character while '\n' refers to, literally, backslash and n.

CollaborationHubContent.php
Lines 857 and 858 are the same array key

CollaborationListContent.php
Lines 724 and 725 are the same array key

Subtle difference: double-quotes vs. single quotes. "\n" thus refers to a newline character while '\n' refers to, literally, backslash and n.

Indeed. I might file a bug with PhpStorm about that :)

Change 339958 merged by jenkins-bot:
Various PHP cleanup of CollaborationKit

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

Change 339961 had a related patch set uploaded (by Reedy):
More PHP cleanup of CollaborationKit

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

Change 339961 merged by jenkins-bot:
More PHP cleanup of CollaborationKit

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

Change 339962 had a related patch set uploaded (by Reedy):
Define extension.json requirements for MediaWiki and EventLogging extension

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

Change 339962 merged by jenkins-bot:
Define extension.json requirements for MediaWiki and EventLogging extension

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

Change 339964 had a related patch set uploaded (by Reedy):
CSS improvements according to PhpStorm

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

This is what remains for CSS/LESS etc after the commit above (as far as PhpStorm is concerned)

Screen Shot 2017-02-26 at 20.12.51.png (838×1 px, 178 KB)

Change 339966 had a related patch set uploaded (by Reedy):
Remove unused variables. var-ify various declarations

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

Change 339964 merged by jenkins-bot:
CSS improvements according to PhpStorm

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

Change 339966 merged by jenkins-bot:
Remove unused variables. var-ify various declarations

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

sortUsersIntoColumns is $column actually an array? $column->items usage suggests it's not...

Yes, it's an array containing stdObjects.

sortUsersIntoColumns is $column actually an array? $column->items usage suggests it's not...

Yes, it's an array containing stdObjects.

		foreach ( $column->items as $item ) {

That $column->items may be an array.. But $column seems to be an object itself

Harej updated the task description. (Show Details)

CollaborationHubContentEditor.php

		foreach ( CollaborationHubContent::getThemeColours() as $colour ) {
			$colours['collaborationkit-' . $colour] = $colour;
		}

Possibly list all messages in full above for grep-ability?

There are too many for this to be feasible. Something like 23.

All of the comments have now been addressed through merged or pending patches (aside from the grep-ability thing which we have decided not to do). Please let us know if you identify any further issues.

CollaborationHubContentEditor.php

		foreach ( CollaborationHubContent::getThemeColours() as $colour ) {
			$colours['collaborationkit-' . $colour] = $colour;
		}

Possibly list all messages in full above for grep-ability?

There are too many for this to be feasible. Something like 23.

Other code manages it fine...

https://github.com/wikimedia/mediawiki/blob/master/includes/MWGrants.php#L59-L65

Hmm, that doesn't look as awful as I expected. I suppose we can give it a shot.

Yeah. Ours will probably be a bit messier, but it's not horrible.