MediaWiki

User:Mglaser/Code Reviews/Social Profile

< User:Mglaser

Based on r96501

General edit

  • 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 edit

  • Line 7: Die with some message
  • 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 — 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
  • function efSocialProfileDBUpdate does not return a value
  • Line 214, 313: I prefer to have all the hooks to bind at one place in the file

UserWelcome edit

UserWelcome.php edit

  • 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 <welcomeUser /> 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 <welcomeUser > tag — 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)

UserStats edit

EditCount.php edit

  • place all hook registrations at the beginning of file
  • 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 edit

  • Line 82: a comment about what user_count is for would greatly improve readability
  • Line 123-128 is a duplicate of line 147-152. I think this can be unified

SpecialUpdateEditCounts.php edit

TopFansByStat.php edit

  • Line 59: dbr is alreay set in line 24
  • Line 99: Here, an inline style is set. There is also a CSS file. I would recommend using this to set the style

TopUsers.php edit

  • Line 60: If I get this right, the limit of 50 should not be hardcoded but be $realcount.

TopUsersTag.php edit

  • 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 edit

  • 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
  • Line 854: Use DB_SLAVE for read access

UserSystemMessages edit

UserSystemMessagesClass.php edit

  • Line 9: die if not in MEDIAWIKI context
    • the file contains only the UserSystemMessages class, so I don't think that's necessary (as per my comment above). --Jack Phoenix (Contact) 01:10, 6 November 2011 (UTC)
  • Line 28: DocBlock says $type is 0 by default. However, there is no default value in the function
    • r102135; the SQL file had NOT NULL default '0' so I think that's where the doc comment came from. --Jack Phoenix (Contact) 01:10, 6 November 2011 (UTC)
  • 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... ;)

UserStatus edit

SpecialUserStatus.php edit

  • Line 9: die if not in MEDIAWIKI context

UserStatusClass.php edit

  • 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 edit

  • 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

UserRelationship edit

Relationship_AjaxFunctions.php edit

  • 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 edit

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

SpecialRemoveRelationship.php edit

  • Line 92: Use $this->relationship_type instead of the method call
    • Are you sure that it does the same thing? I'm not 100% sure, which is why I want to test it before making the change. --Jack Phoenix (Contact) 15:18, 7 November 2011 (UTC)

SpecialViewRelationshipRequests.php edit

  • 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?
    • I have absolutely no idea...I think the code might've been copypasted from SpecialRemoveRelationship.php, as the SpecialRemoveRelationship class has such member variables. --Jack Phoenix (Contact) 15:18, 7 November 2011 (UTC)
  • 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 edit

  • 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 edit

  • Line 6/7 and 12/13: setting display should be sufficient, visibility not needed.
    • You're probably right; the code was translated pretty much directly from YUI (Yahoo! User Interface Library), which used to set both properties. --Jack Phoenix (Contact) 15:18, 7 November 2011 (UTC)

UserRelationshipClass.php edit

  • 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.
    • it should be secure due to the usage of the Database class; calling addQuotes() while using Database's insert() or other functions actually leads to double-encoding. --Jack Phoenix (Contact) 15:18, 7 November 2011 (UTC)
  • Line 71,115,159: user might have been deleted in the meanwhile. Check if id really exists.
    • huh? In most situations users won't be deleted and MediaWiki doesn't really support that. --Jack Phoenix (Contact) 15:18, 7 November 2011 (UTC)
  • Line 229, 242: you might want to use wfTimestamp instead.
    • this is something I've been wanting to do a long time, but I haven't had the courage to do it because I don't want to mess up a lot of wikis. Would the output of wfTimestamp(Now)() be equal to the current date() calls in various places in the SocialProfile codebase? Because if so, then I could probably change it...but I've had some problems with timestamp stuff in the past on my local computer. --Jack Phoenix (Contact) 15:18, 7 November 2011 (UTC)
  • Line 368, 389, 409, 435, 473: Use DB_SLAVE for read-only operations
  • Line 603: Use constants for relType for better readability

UserProfile edit

Avatar.php edit

  • 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 edit

SpecialPopulateExistingUsersProfiles.php edit

  • Line 32: you can do that in the constructor: parent::__construct( 'PopulateUserProfiles', 'staff' );
    • actually, no; the second parameter to SpecialPage's constructor is a user right and 'staff' is a user group (and yes, it's bad practise to do stuff based on group rights rather than user rights; I'll fix that someday, but it's not terribly important as Special:PopulateExistingUsersProfiles is meant to be accessed only once, after the initial installation of SocialProfile). --Jack Phoenix (Contact) 15:44, 7 November 2011 (UTC)

SpecialToggleUserPage.php edit

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

SpecialUpdateProfile.php edit

  • 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;
    • no idea...maybe the original code was written in 2007. --Jack Phoenix (Contact) 15:44, 7 November 2011 (UTC)
  • 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 "<script>alert( 'test' );</script>" 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 <div class="cleared" /> . 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?
    • no idea really...if removing it doesn't break stuff, I'll do that later on. --Jack Phoenix (Contact) 15:44, 7 November 2011 (UTC)

UpdateProfile.js edit

  • 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 edit

  • 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 edit

  • Line 36ff: AFAIK, it should be sufficient to just set the display style.
    • the code was directly translated from YUI, so that's why that's there. --Jack Phoenix (Contact) 15:44, 7 November 2011 (UTC)
  • Line 61 (in for): Use var for variable declaration to keep it in local scope and have it destroyed after function execution

UserProfilePage.php edit

  • 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

UserGifts edit

GiftsClass.php edit

SpecialGiftManager.php edit

  • 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
    • Database takes care of sanitizing the SQL; I'm not totally sure about the XSS, but I believe that the variable should be appropriately escaped. If this is not the case, please email me ASAP with steps to reproduce. --Jack Phoenix (Contact) 15:14, 6 November 2011 (UTC)

SpecialGiftManagerLogo.php edit

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

UserGiftsClass.php edit

  • 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
    • Database takes care of sanitizing the SQL; I'm not totally sure about the XSS, but I believe that the variable should be appropriately escaped. If this is not the case, please email me ASAP with steps to reproduce. --Jack Phoenix (Contact) 15:14, 6 November 2011 (UTC)

UserBoard edit

SpecialUserBoard.php edit

UserBoardClass.php edit

  • Line 9, 10: var is deprecated. Use public or private instead
    • the class member variables were unused anyway, so I removed them in r102178. --Jack Phoenix (Contact) 15:08, 6 November 2011 (UTC)
  • Line 16: constructor is always public
  • Line 45: $message is not properly escaped in SpecialSendBoardBlast.php line 65. XSS and SQL injections
    • I believe that the Database class takes care of escaping already for us, but if I'm incorrect, please email me with steps to reproduce — I take security very seriously. --Jack Phoenix (Contact) 15:08, 6 November 2011 (UTC)
  • Line 158: Check todo comment
  • Line 208: Use DB_SLAVE for read-only access

UserActivity edit

UserActivityClass.php edit

  • 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)
      • Is there perhaps something else you can check for that would confirm whether the extensions are in use? I thought about class_exists, but Vote and Comment are pretty generic class names and could lead to false positives so I'm not sure about that. Reach Out to the Truth 02:12, 6 November 2011 (UTC)
        • Exactly, and that's why I ended up using Database::tableExists() instead of PHP's class_exists(). If you have better ideas how to detect the presence or lack of thereof of VoteNY and Comments, I'm open to suggestions. --Jack Phoenix (Contact) 15:00, 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 empty IN () clause. I suggest you check that before.
  • Line 530: Use global before starting the for loop

SystemGifts edit

SpecialSystemGiftManager.php edit

SpecialSystemGiftManagerLogo.php edit

  • 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
    • I have no idea either (I didn't write that code originally, the original authors forked it from an old version of Special:Upload and modified it according to their needs) but I didn't change that just yet. --Jack Phoenix (Contact) 15:00, 6 November 2011 (UTC)
  • Line 211: Upload size is hardcoded. You could use $wgMaxUploadSize instead
    • does this really have the same effect? After reading the manual page, I wasn't sure so I didn't change it just yet. If it has the same effect as the current code, I will change it then. --Jack Phoenix (Contact) 15:00, 6 November 2011 (UTC)
  • Line 415: Undefined variable $stash

SpecialViewSystemGift.php edit

SystemGiftsClass.php edit

all done in r102176. --Jack Phoenix (Contact) 15:00, 6 November 2011 (UTC)
  • 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 edit

all done in r102176. --Jack Phoenix (Contact) 15:00, 6 November 2011 (UTC)
  • Line 15, 16: var is deprecated, here, use private instead
  • Line 22: constructor is always public.
Retrieved from "https://www.mediawiki.org/w/index.php?title=User:Mglaser/Code_Reviews/Social_Profile&oldid=4130948"