Last modified: 2010-05-15 15:37:34 UTC

Wikimedia Bugzilla is closed!

Wikimedia has migrated from Bugzilla to Phabricator. Bug reports should be created and updated in Wikimedia Phabricator instead. Please create an account in Phabricator and add your Bugzilla email address to it.
Wikimedia Bugzilla is read-only. If you try to edit or create any bug report in Bugzilla you will be shown an intentional error message.
In order to access the Phabricator task corresponding to a Bugzilla report, just remove "static-" from its URL.
You could still run searches in Bugzilla or access your list of votes but bug reports will obviously not be up-to-date in Bugzilla.
Bug 1650 - A patch to fix a bug and incoherant code in Special:Blockip
A patch to fix a bug and incoherant code in Special:Blockip
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
Special pages (Other open bugs)
1.5.x
Other All
: Normal normal (vote)
: ---
Assigned To: Nobody - You can work on this!
: patch
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-03-07 12:35 UTC by Ævar Arnfjörð Bjarmason
Modified: 2010-05-15 15:37 UTC (History)
0 users

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
The patch against includes/SpecialBlockip.php (1.04 KB, patch)
2005-03-07 12:36 UTC, Ævar Arnfjörð Bjarmason
Details
The patch against includes/SpecialBlockip.php (1.12 KB, patch)
2005-03-07 15:16 UTC, Ævar Arnfjörð Bjarmason
Details
The patch against includes/SpecialBlockip.php (1.11 KB, patch)
2005-03-07 16:42 UTC, Ævar Arnfjörð Bjarmason
Details
A benchmark of various versions of the code. (1.19 KB, text/plain)
2005-03-08 12:13 UTC, Ævar Arnfjörð Bjarmason
Details

Description Ævar Arnfjörð Bjarmason 2005-03-07 12:35:51 UTC
Currently Special:Blockip outputs the following code:

<?= <option>2 hours</option><option>1 day</option>[...]<option>infinite</option> ?>

This causes (in Gecko) the first element to not be rendered at all, as well as
of course breaking XHTML compilance.

Furthermore I felt the code used to generate the list was incoherant and used an
inappropriate function (split where explode should be used) so I rewrote it.

Marked this as a blocker because it blocks the user from using the "2 hours" option.
Comment 1 Ævar Arnfjörð Bjarmason 2005-03-07 12:36:36 UTC
Created attachment 344 [details]
The patch against includes/SpecialBlockip.php
Comment 2 Ævar Arnfjörð Bjarmason 2005-03-07 15:16:27 UTC
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.
Comment 3 Ævar Arnfjörð Bjarmason 2005-03-07 16:42:01 UTC
Created attachment 347 [details]
The patch against includes/SpecialBlockip.php

Fixing a small error that crept in during testing..
Comment 4 Ævar Arnfjörð Bjarmason 2005-03-08 12:12:09 UTC
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.
Comment 5 Ævar Arnfjörð Bjarmason 2005-03-08 12:13:48 UTC
Created attachment 350 [details]
A benchmark of various versions of the code.

The benchmark of varios versions of the code.
Comment 6 Brion Vibber 2005-03-08 12:53:30 UTC
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.
Comment 7 Ævar Arnfjörð Bjarmason 2005-03-08 13:52:12 UTC
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>";
Comment 8 Brion Vibber 2005-03-08 22:01:54 UTC
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.
Comment 9 Snok 2005-03-09 16:15:28 UTC
(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. ;)
Comment 10 Ævar Arnfjörð Bjarmason 2005-03-09 16:37:41 UTC
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.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links