Wikimedia Bugzilla is closed!

Created attachment 344 [details]
The patch against includes/SpecialBlockip.php

Created attachment 345 [details]
The patch against includes/SpecialBlockip.php

Turns out there was indvalid XHTML there *as well* which kept the output from
being valid XHTML.

Created attachment 347 [details]
The patch against includes/SpecialBlockip.php

Fixing a small error that crept in during testing..

My initial rewrite was to replace split() with explode(), split is much slower
and shouldn't be used unless you need it, see the code for split in
ext/standard/reg.c as of static void php_split and the code for explode() in
ext/standard/string.c as of PHP_FUNCTION(explode)

Here are some benchmarks of 1,000,000 runs of each version of the code

$ php -f bench.php
Time: 59.1960840225 seconds # This is snok (e23)'s original code

$ php -f bench.php 
Time: 53.7820188999 seconds # This is the foreach loop I originally submitted.

$ php -f bench.php 
Time: 23.2223100662 seconds # This is my final optimized version

The code for the final one is:
$blockExpiryOptions = '<option>' . implode('</option><option>', explode(',',
$wgBlockExpiryOptions)) . '</option>';

sadly I can't send a patch for this one as I don't have CVS access and
sourceforge has a 24hr timeframe in which the public cvs server gets updated,
and the code has been modified in the meantime.

Created attachment 350 [details]
A benchmark of various versions of the code.

The benchmark of varios versions of the code.

Don't use explode at all -- the values should be set in an array to begin with.

And please don't waste your time benchmarking the generation of the block 
form to try to save a millisecond of CPU time per day! Maintainable code 
saves far more time in this kind of case.

Speaking of which, what should be done to begin with is to arrange this in a
manner which makes the period names localizable, as every other string should
be, but sadly this isn't the case. As I'm sure you're aware I've been sending
some patches recently to fix some of this in HEAD and will probably send in a
rewrite of this block to make it localizable soon.

Furthermore, although it might be fallacious to argue what "maintainable code"
is (it's by definition in the eye of the beholder) I for one do not think this
(my version, variable names changed):
$string = '<option>' . implode('</option><option>', explode(',', $options)) .
'</option>';

Is any less coherant than this version (the original):
$string = join("</option><option>", split(",", $options));
$string = "<option>" . $string . "</option>";

The existing code is unmaintainably illegible, in part because it's using a 
comma-separated string where an array would make sense.

As for localization, keep in mind that the keys are used as input to 
strtotime(); the prior 1.3/1.4 code simply requires the user to know what will 
be valid input and write it in (in English). Whether this arbitrary set of fixed 
values will be appropriate is not yet known.

(In reply to comment #8)
> The existing code is unmaintainably illegible, in part because it's using a 
> comma-separated string where an array would make sense.

I realize the comma separated list isn't strictly the way to produce the most
legible code, but it was a conscious choice that I made for two reasons.

First, if the options are to be localized eventually, or made into a
wiki-editable message variable, the comma separated list is a simple solution.
Or two really, one with the values and one with the corresponding translated
strings. The best alternative, as far as I know, would be to create a number of
separate variables like blockipopt1, blockipopt2, etc. I'm open for suggestions
on this.

Second, as it is, the list of options is placed in DefaultSettings.php, a file
often used by novices. I think a comma separated list is slightly more intuitive
than the array syntax, and this intuitiveness is worth the extra code in
specialblockip. But this is just my opinion.

Now, with this explanation I hope I can somewhat alleviate the tarnish my
reputation undoubtly will suffer from those 5-6 lines of vile, acid and
illegible code. May history forgive me. Peace. ;)

A basic cookup of how an internationalized version might look:

$options = '2 hours,1 day,3 days,1 week,2 weeks,1 month,3 months,6 months,1
year,infinite';
$array = explode(',', $options);

for( $i=0; $i<count( $array ); ++$i ) {
        echo "<option value=\"$array[$i]\"> blockipopt$i </option>\n";
}

This has some faults however, once rolled out it would be hard to change the
values as they would be more or less hard-coded due to internationalization
issues, it's probably best to leave it as it is for some field-testing to see
how it's accepted.

Either that or going back to the old system of manually entering the date, or
some middleground like entering the nr. of hours.