User:MarkAHershberger/Weekly reports/2010-W16

Title.php refactoring

I spent the entire refactoring Title::getUserPermissionsErrorsInternal() and writing tests for complete coverage for almost every line of code that I touched. The one part that is not yet covered are the two functions that cover hooks and the return values from hooks.

Last week I reported that the two functions — Title::getUserPermissionsErrors() and Title::getUserPermissionsErrorsInternal() — covered 335 lines of code. I ended up refactoring this into 11 functions covering 411 lines of code and new doc comments. With the refactoring, it should be easy to add new permission checks or move existing permission checks to hooks.

Going through the code so carefully helped me find a few interesting problems in the code. The Perl geek in me was amused to find

   if( $cascadingSources > 0 && isset($restrictions[$action]) )

where $cascadingSources is an array. While this code would be meaningful in in Perl (in a comparison, an array is automatically cast to a scalar value containing the number of elements in the array), in PHP the comparison is always true. I tried to make sure that code cleanup like this was in separate commits so it wouldn't get lost in the big refactoring commit.

Still, there were a number of white-space changes in the refactoring commit and MaxSem complained that these made the code harder to review. I think most of this problem could be alleviated by having a “show white-space diffs” toggle in the CodeReview extension.

Finally, I've uploaded a coverage report that PHPUnit produces optionally. The report makes it easy to see the code coverage for Title.php’s permission checking and it was a great tool to help me make sure that I actually read the code instead of just skimming it. I've applied for a toolserver account so that I can begin running these sorts of reports regularly and have the results available to everyone.

Planned work

Additionally, Mdale has produced a JS2Support extension that should be reviewed. Since integrating this work is a high priority, I'll try to make sure it gets done.

Because I spent the week working on Title.php, I didn't have a chance to work on the feedback for uploading by URL or respecting the watchlist and ignore warnings checkboxes. I'm going to postpone that some more to work on the JS2Support work.