PhpRiot
News Archive
PhpRiot Newsletter
Your Email Address:

More information

The PHP project and Code Review

Note: This article was originally published at Planet PHP on 24 December 2010.
Planet PHP
Reading code is not only fun, its also a great way to exercise your brain - not to mention a fantastic way to discover new ways to solve problems. At workA(we are hiring btw!), for example, I read pretty much every single commit (and merge requests, for that matter) - and I'm subscribed to several different OSS commit lists. I can't say I read every commit to PHP, I focus on the areas I care about, but I do skim over the rest - if only just to see when new features are added.

The PHP project has a good chunk of mailinglists, everything from support lists, developer discussions, QA, and so on. Every commit to our SVN isAautomatically posted to the relevant commit mailinglist(s).
The main commit lists are;
A- PHP (php-cvs@lists.php.net,Ahttp://news.php.net/php.cvs)
A- PHP Documentation (doc-cvs@lists.php.net, http://news.php.net/php.doc.cvs)
A- PHP-GTK (phpgtk-cvs@lists.php.net,Ahttp://news.php.net/php.gtk.cvs)
A- PEAR (pear-cvs@lists.php.net, http://news.php.net/php.pear.cvs)
A- PECL (pecl-cvs@lists.php.net, http://news.php.net/php.pecl.cvs)


Then we have specific lists for each translation of the docs, all the websites, PEAR docs and so on.
The great thing about these commit lists is that *anyone* can subscribe to them, and quite a lot of people actually are.
For example, there are over 200 people registered to the PHP commit list, and over 100 people on the documentation commit list alone. That is ca 25% of the people that are subscribed to their discussion counterparts.
Granted that most of the subscribers do not actively review the commits, I guesstimate there is still a sizable percentage of them that do.

Just the simple fact that I know people will be reading through my commits makes me think about what I am doing a bit more; "Is this really needed?", "Is there be a better way solving this?", "Could it potentially break other things?", "Is this actually correct?"..
The people who review the commits often don't seem like the friendliest people in the world.. If there are issues with the commit; You will be told. No doubt about it. And you will feel like an idiot, for few minutes, for not having caught it yourself - and promise yourself to review your commits better in the future. Learning from your mistakes. I love it. Mission accomplished.


Last year I updated the credit list, printed out by phpinfo() & phpcredits(), for the people maintaining our websites, with the commit message "Throw some credit around" (yes, the typo and incorrect colspan in the commit was caught and fixed).

Fast forward 15months, to last Friday. Another commit from me to PHP trunk with the commit message "Throw some credit around", and then toAPHP 5.3 a minute later with the same commit message.
Less then 10minutes after the commit I had received 2 emails asking me "WTF?" - and a poke on IRC.
I had no idea what those guys were talking about. None what soever. I had not committed anything to any PHP project for several days. Nothing. Not even a typo fix.

Turns out that someone in Asia had somehow managed to get his/hers hands on my PHP.net account credentials.
Interestingly enough the commit doesn't introduce any security holes, bugs, or anything at all really. It just adds "Wolegequ Gelivable" to the credit list (whatever that means).

Its not a great feeling to have your account hacked into, but I do wonder what the intentions were.. Maybe just an credentials check, which was supposed to be followed by evil commits if noone had spotted the first one? TheAChineseAgovernmentAtrying to introduce security holes so they can break into PHP websites?

In any case. It took less then 10minutes for 3 people to catch it, that is pretty cool.


-Hannes

p.s. Last year, one of my new year's resolutions was to "blog once a month". My last blogpost was in January.