User:Mglaser/Code Reviews/Social Profile

Based on r96501

General

 * Please put globals at the beginning of a function.
 * Please put hook registrations at the beginning of a file.
 * You should prevent direct access in all files (die if not defined MEDIAWIKI).
 * AFAIK this isn't needed if a file has only classes or a class (as opposed to global variables and/or functions); however, I couldn't find the commit I was looking for, I'm sure that a core developer removed the if not defined MEDIAWIKI checks from a few files due to this very reason. --Jack Phoenix (Contact) 00:22, 6 November 2011 (UTC)
 * A note on the architecture: It would be great to have the fields more dynamic, e.g. make it possible to add new fields via a configuration variable. I realize this is not an easy task given the current architecture :)

SocialProfile.php

 * Line 7: Die with some message
 * added a message in r102129 --Jack Phoenix (Contact) 00:22, 6 November 2011 (UTC)
 * Line 11: What other entry files are there?
 * clarified the comment in r102129. In the past, there was no "SocialProfile", but rather a bunch of social extensions under the extensions directory &mdash; UserProfile, UserGifts, etc. were all separate extensions and one would have had to require_once multiple extensions in their LocalSettings.php instead of requiring the SocialProfile.php file. --Jack Phoenix (Contact) 00:22, 6 November 2011 (UTC)
 * Line 218, 229: I prefer to have all the globals at the beginning of a function
 * fixed in r102129 --Jack Phoenix (Contact) 00:22, 6 November 2011 (UTC)
 * function efSocialProfileDBUpdate does not return a value
 * is there a reason for it to? --Jack Phoenix (Contact) 00:22, 6 November 2011 (UTC)
 * Line 214, 313: I prefer to have all the hooks to bind at one place in the file
 * done in r102129 --Jack Phoenix (Contact) 00:22, 6 November 2011 (UTC)

UserWelcome.php

 * Line 57: Are you sure $wgUser is always an object?
 * in what kind of a situation it wouldn't be an object? The output of the &lt;welcomeUser /&gt; tag isn't mean to be displayed to anonymous users, but the world doesn't explode even if an anon loads a page with the &lt;welcomeUser &gt; tag &mdash; it just looks silly, since anons can't have points or profiles. --Jack Phoenix (Contact) 00:22, 6 November 2011 (UTC)
 * Line 144, 153, 168, 185, 202: Is nofollow always wanted?
 * depends. Wikia used to hard-code it in but it probably should do whatever Linker would do, but that would require converting the code to use Linker (which admittedly now is a lot easier than in the past, given that Linker is finally static, but IIRC link and friends still don't support image links). --Jack Phoenix (Contact) 00:22, 6 November 2011 (UTC)

EditCount.php

 * place all hook registrations at the beginning of file
 * r102132 --Jack Phoenix (Contact) 00:22, 6 November 2011 (UTC)
 * Line 37, 64: use DB_SLAVE as you are only reading
 * that is generally true, but I believe the reason why you'd sometimes use DB_MASTER for reads is to avoid getting outdated data from a slave; for example, you'd want to get the most up-to-date data here so that users can't gain extra points or lose points due to slave lag. Or maybe it's just bad coding by the original authors. :-) --Jack Phoenix (Contact) 00:22, 6 November 2011 (UTC)

GenerateTopUsersReport.php

 * Line 82: a comment about what user_count is for would greatly improve readability
 * r102132 --Jack Phoenix (Contact) 00:22, 6 November 2011 (UTC)
 * Line 123-128 is a duplicate of line 147-152. I think this can be unified
 * r102132 --Jack Phoenix (Contact) 00:22, 6 November 2011 (UTC)

SpecialUpdateEditCounts.php

 * Line 100: title should be internationalized
 * r102132 --Jack Phoenix (Contact) 00:22, 6 November 2011 (UTC)
 * Line 138: Message should be internationalized
 * r102132 --Jack Phoenix (Contact) 00:22, 6 November 2011 (UTC)

TopFansByStat.php

 * Line 59: dbr is alreay set in line 24
 * r102132 --Jack Phoenix (Contact) 00:22, 6 November 2011 (UTC)
 * Line 99: Here, an inline style is set. There is also a CSS file. I would recommend using this to set the style
 * r102132 --Jack Phoenix (Contact) 00:22, 6 November 2011 (UTC)

TopUsers.php

 * Line 60: If I get this right, the limit of 50 should not be hardcoded but be $realcount.
 * r102132 --Jack Phoenix (Contact) 00:22, 6 November 2011 (UTC)

TopUsersTag.php

 * Line 42: if limit is greater than 50, reduce to 50, not 5. I think, 50 should not be hardcoded. Maybe a configuration var?
 * this is something between a site-specific hack and a generally useful feature. It was written so that a site administrator could show "Top X" users on the main page (for example) of a wiki. For that use case, 5 seems more reasonable of a fallback value than 50; I'm not sure how useful it would be to have this limit configurable. This is after all a parser hook version of Special:TopUsers, which shows only 50 users at most (example on Halopedia). --Jack Phoenix (Contact) 00:22, 6 November 2011 (UTC)

UserStatsClass.php

 * Line 409: please document why you use parser->preprocess here.
 * I have no idea why it's being used there, sadly. :-/ --Jack Phoenix (Contact) 00:22, 6 November 2011 (UTC)
 * Line 660: function should return an array, string given
 * r102132 --Jack Phoenix (Contact) 00:22, 6 November 2011 (UTC)
 * Line 854: Use DB_SLAVE for read access
 * r102132 --Jack Phoenix (Contact) 00:22, 6 November 2011 (UTC)

UserSystemMessagesClass.php

 * Line 9: die if not in MEDIAWIKI context
 * Line 28: DocBlock says $type is 0 by default. However, there is no default value in the function
 * Line 59: Use DB_SLAVE and $dbr for read access
 * function getMessageList doesn't seem to be used anywhere. However, it is duplicating code from UserGifts->getUserGiftList. I tend to believe this is some copy&paste artifact.
 * Line 114 to 118: I really like to see that function in core... ;)

SpecialUserStatus.php

 * Line 9: die if not in MEDIAWIKI context

UserStatusClass.php

 * Line 9: AFAIK, constructor is always public
 * Line 48: You should probably check if the query is executed correctly
 * Line 62: You might want to inform the user that message is too long
 * Line 110: $dbr &= The ampersand is no longer neccessary as of PHP 5
 * Line 140: Hardcoded number of status items in history. Better make this configurable
 * You could probably use all the methods in a static way which would safe you all the instantiating in UserStatus_AjaxFunctions

UserStatus_AjaxFunctions.php

 * Line 6: the name "wfSaveStatus" is very general and might cause naming collisions. Maybe better: wfSPSaveStatus
 * Line 33: cf. Line 6
 * Line 44: Use i18n
 * Line 110: Use i18n
 * Line 114,117,123,127: Use i18n

Relationship_AjaxFunctions.php

 * Line 18: $_POST['response'] goes into database unsanitized. Check for MySQL injections.
 * the code is not "demonstratably secure", but it should be secure; I added an extra intval in there for good measure in r102133. --Jack Phoenix (Contact) 00:46, 6 November 2011 (UTC)

SpecialAddRelationship.php

 * Line 99,101: The same function is called twice. You might want to store the result locally

SpecialRemoveRelationship.php

 * Line 92: Use $this->relationship_type instead of the method call

SpecialViewRelationshipRequests.php

 * Line 34: There is also a function $wgUser->isLoggedIn which would make this line more future proof, also in SpecialAdd-, -View- and -RemoveRelationship.php
 * Line 53/54: Where does user_name_to and relationship_type come from?
 * Line 55: $_POST['message'] is not sanitized and not checked in addRelationshipRequest. Use $wgRequest->getVal at least.
 * I think that this is a case of "secure, but not demonstratably so". This special page calls UserRelationship's addRelationshipRequest, which in turn uses MediaWiki's Database abstraction layer, specifically Database(Base)::insert; Database(Base)::insert in turn calls Database(Base)::addQuotes to sanitize the input. --Jack Phoenix (Contact) 00:46, 6 November 2011 (UTC)
 * Line 88: not sure whether parse really eliminates XSS vulnerabilities. You might do another extra check.

SpecialViewRelationships.php

 * Line 72: Check naming: $user is pointing to the title object of the user page, not the user object. Better: $userPage.
 * Line 79: $out is not initialized.

UserRelationship.js

 * Line 6/7 and 12/13: setting display should be sufficient, visibility not needed.

UserRelationshipClass.php

 * Line 16: constructor is always public
 * Line 34: sanitize $message before writing it to database. Check for MySQL injections. Check the other values that are inserted as well.
 * Line 71,115,159: user might have been deleted in the meanwhile. Check if id really exists.
 * Line 229, 242: you might want to use wfTimestamp instead.
 * Line 368, 389, 409, 435, 473: Use DB_SLAVE for read-only operations
 * Line 603: Use constants for relType for better readability

Avatar.php

 * Line 15,16,17: use of var is deprecated since PHP 5. Use public or private instead
 * Line 58: Naming: getAvatarURL actually gives you an image tag
 * this has been annoying me for a while, too, but fixing it will be a tad bit complicated, as many other social tools call that function. --Jack Phoenix (Contact) 00:46, 6 November 2011 (UTC)

SpecialEditProfile.php

 * Line 161, 352, 441: Use DB_SLAVE

SpecialPopulateExistingUsersProfiles.php

 * Line 32: you can do that in the constructor: parent::__construct( 'PopulateUserProfiles', 'staff' );

SpecialToggleUserPage.php

 * Line 41, 49, 62: Fetch db connection only once, as DB_MASTER

SpecialUpdateProfile.php

 * Line 131: Use i18n
 * Line 177: This seems to be really old code for PHP4 support. You might want to consider updating this since it is deprecated as of MW1.17
 * Line 223: This function has no support for international date formats.
 * Line 226: Why is the standard year set to 2007? In line 239, it is set to 0000;
 * Line 265ff: User input is not sanitized. Check for MySQL injections and XSS
 * Line 314ff: User input is not sanitized. Check for MySQL injections and XSS
 * Line 342ff: User input is not sanitized. Check for MySQL injections and XSS
 * Database class should take care of most of that, I think; if I enter " alert( 'test' ); " and save my profile, that is directly shown in the profile page instead of being parsed as JavaScript code. --Jack Phoenix (Contact) 00:46, 6 November 2011 (UTC)
 * Line 373, 573, 716: Use DB_SLAVE for readonly access
 * Line 470: you might as well use . This occurs several times in the following output html
 * Line 735.: in all the other pages, you do not set h1 manually. Is there a special reason you do it here?

UpdateProfile.js

 * Line 24, 25 (in for loop), 31, 35 (in for loop): Use var for variable declaration to keep it in local scope and have it destroyed after function execution

UserProfileClass.php

 * Line 9ff: var is deprecated. Use public or private instead.
 * Line 101: You should set the SQL LIMIT to 1 if you are only reading one row afterwards
 * Line 155: A function "formatBirthday" can also be found in SpecialUpdateProfile, with a slightly different output. You might want to unify this.
 * Line 160: Birthdate format is not internationalized

UserProfilePage.js

 * Line 36ff: AFAIK, it should be sufficient to just set the display style.
 * Line 61 (in for): Use var for variable declaration to keep it in local scope and have it destroyed after function execution

UserProfilePage.php

 * Line 14: var is deprecated. Use public or private instead
 * Line 19: Do not use the ampersand operator, you are not modifying $title within the function
 * Line 418: I am not too happy with the hardcoded namespace number as a fallback. It might already be taken by another extension or local namespace. IMHO it is better tho print an error message when NS_POLL is not set.
 * Line 640f: It may be worth putting this into a seperate function. It's used again e.g. in line 710f
 * Line 750: use $this->isOwner
 * Line 945: initialize $output first, e.g. with $output = '';
 * Line 1238-1291: This code seems to produce no output, since $by_type is always overridden.
 * Line 1320, 1407: I would prefer to have the TTL time in a global settings variable to be more flexible here.
 * Line 1551: Please do not hardcode any formatting. The color should be put in the CSS

GiftsClass.php

 * Line 17, 18: var is deprecated. Use public or private instead
 * Line 24: constructor is always public
 * Line 206, 220: it is more effective to count gift_id instead of *

SpecialGiftManager.php

 * Line 37ff, 50ff: Use $wgRequest->getVal at least.
 * Line 40, 41, 53, 54: This goes directly into the database. Check for XSS and MySQL injections

SpecialGiftManagerLogo.php

 * Line 12-18: var is deprecated. Use public or private instead

UserGiftsClass.php

 * Line 16/17: var is deprecated, use public or private instead
 * Line 23: Constructor is always public
 * Line 51: message is not sanitized. Check for XSS and MySQL injections

SpecialUserBoard.php

 * Line 224: I prefer putting color codes into CSS
 * Line 288: Please put width information into CSS

UserBoardClass.php

 * Line 9, 10: var is deprecated. Use public or private instead
 * Line 16: constructor is always public
 * Line 45: $message is not properly escaped in SpecialSendBoardBlast.php line 65. XSS and SQL injections
 * Line 158: Check todo comment
 * Line 208: Use DB_SLAVE for read-only access

UserActivityClass.php

 * Lines 15-27: var is deprecated. Here, use private instead
 * Line 168, 237: I thought about this a lot and talked to some other people. Finally, I came to the conclusion that in a production environment you should just be able to assume that all neccessary database tables are present. This check is another db call and thus costs performance. Is there any reason why you check table existence just here and not in other SocialProfile modules?
 * The Vote table is related to the VoteNY extension, and the Comments table is related to the Comments extension; these are separate social extensions and they may be present, but it's likelier that they aren't there. --Jack Phoenix (Contact) 00:46, 6 November 2011 (UTC)
 * Line 97, 190, 259, 351, 415, 506, 592, 681, 771: This will lead to a MySQL error if no data is present, since it produces an emtpy IN clause. I suggest you check that before.
 * Line 530: Use global before starting the for loop

SpecialsSystemGiftManager.php

 * Line 53: Use $wgRequest

SpecialSystemGiftManagerLogo.php

 * Lines 13-19: var is deprecated. Use public or private instead
 * Line 55: Not sure why you are passing wgRequest, since it is global anyway
 * Line 211: Upload size is hardcoded. You could use $wgMaxUploadSize instead
 * Line 415: Undefined variable $stash

SpecialViewSystemGift.php

 * Line 50: Use DB_SLAVE for read-only operations.

SystemGiftsClass.php

 * Line 11: var is deprecated, here, use private instead
 * Line 32: constructor is always public. Here, it has no function, you might as well delete this function
 * Line 89: "got" is not internationalized
 * Line 95: missing i18n
 * Line 173: instead of wfSuppressWarnings, you could also check if the index is set

UserSystemGiftsClass.php

 * Line 15, 16: var is deprecated, here, use private instead
 * Line 22: constructor is always public.