r102625 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r102624‎ | r102625 | r102626 >
Date:07:41, 10 November 2011
Author:freakolowsky
Status:resolved (Comments)
Tags:
Comment:
* Added getInfinity to DatabaseOracle
* Block - replaced 'infinity' strings with DB->getInfinity calls
* UploadStash - added sequence value generation for ID
Modified paths:
  • /trunk/phase3/includes/Block.php (modified) (history)
  • /trunk/phase3/includes/api/ApiQueryBlocks.php (modified) (history)
  • /trunk/phase3/includes/db/DatabaseOracle.php (modified) (history)
  • /trunk/phase3/includes/upload/UploadStash.php (modified) (history)
  • /trunk/phase3/tests/phpunit/includes/api/ApiBlockTest.php (modified) (history)

Diff [purge]

Index: trunk/phase3/tests/phpunit/includes/api/ApiBlockTest.php
——@@ -55,7 +55,7 @@
5656
5757 $this->assertEquals( 'UTApiBlockee', (string)$block->getTarget() );
5858 $this->assertEquals( 'Some reason', $block->mReason );
59 - $this->assertEquals( 'infinity', $block->mExpiry );
 59+ $this->assertEquals( $this->db->getInfinity(), $block->mExpiry );
6060
6161 }
6262
Index: trunk/phase3/includes/upload/UploadStash.php
——@@ -229,6 +229,7 @@
230230 $dbw = $this->repo->getMasterDb();
231231
232232 $this->fileMetadata[$key] = array(
 233+ 'us_id' => $dbw->nextSequenceValue( 'uploadstash_us_id_seq' ),
233234 'us_user' => $this->userId,
234235 'us_key' => $key,
235236 'us_orig_path' => $path,
Index: trunk/phase3/includes/db/DatabaseOracle.php
——@@ -548,8 +548,9 @@
549549 $val = $val->fetch();
550550 }
551551
 552+ // backward compatibility
552553 if ( preg_match( '/^timestamp.*/i', $col_type ) == 1 && strtolower( $val ) == 'infinity' ) {
553 - $val = '31-12-2030 12:00:00.000000';
 554+ $val = $this->getInfinity();
554555 }
555556
556557 $val = ( $wgContLang != null ) ? $wgContLang->checkTitleEncoding( $val ) : $val;
——@@ -1315,4 +1316,9 @@
13161317 public function getSearchEngine() {
13171318 return 'SearchOracle';
13181319 }
 1320+
 1321+ public function getInfinity() {
 1322+ return '31-12-2030 12:00:00.000000';
 1323+ }
 1324+
13191325 } // end DatabaseOracle class
Index: trunk/phase3/includes/api/ApiQueryBlocks.php
——@@ -130,8 +130,8 @@
131131 $this->addWhereIf( 'ipb_user != 0', isset( $show['account'] ) );
132132 $this->addWhereIf( 'ipb_user != 0 OR ipb_range_end > ipb_range_start', isset( $show['!ip'] ) );
133133 $this->addWhereIf( 'ipb_user = 0 AND ipb_range_end = ipb_range_start', isset( $show['ip'] ) );
134 - $this->addWhereIf( "ipb_expiry = 'infinity'", isset( $show['!temp'] ) );
135 - $this->addWhereIf( "ipb_expiry != 'infinity'", isset( $show['temp'] ) );
 134+ $this->addWhereIf( "ipb_expiry = '".$db->getInfinity()."'", isset( $show['!temp'] ) );
 135+ $this->addWhereIf( "ipb_expiry != '".$db->getInfinity()."'", isset( $show['temp'] ) );
136136 $this->addWhereIf( "ipb_range_end = ipb_range_start", isset( $show['!range'] ) );
137137 $this->addWhereIf( "ipb_range_end > ipb_range_start", isset( $show['range'] ) );
138138 }
Index: trunk/phase3/includes/Block.php
——@@ -81,8 +81,8 @@
8282 $this->mAuto = $auto;
8383 $this->isHardblock( !$anonOnly );
8484 $this->prevents( 'createaccount', $createAccount );
85 - if ( $expiry == 'infinity' || $expiry == wfGetDB( DB_SLAVE )->getInfinity() ) {
86 - $this->mExpiry = 'infinity';
 85+ if ( $expiry == wfGetDB( DB_SLAVE )->getInfinity() ) {
 86+ $this->mExpiry = $expiry;
8787 } else {
8888 $this->mExpiry = wfTimestamp( TS_MW, $expiry );
8989 }
——@@ -362,9 +362,8 @@
363363 $this->mId = $row->ipb_id;
364364
365365 // I wish I didn't have to do this
366 - $db = wfGetDB( DB_SLAVE );
367 - if ( $row->ipb_expiry == $db->getInfinity() ) {
368 - $this->mExpiry = 'infinity';
 366+ if ( $row->ipb_expiry == wfGetDB( DB_SLAVE )->getInfinity() ) {
 367+ $this->mExpiry = $row->ipb_expiry;
369368 } else {
370369 $this->mExpiry = wfTimestamp( TS_MW, $row->ipb_expiry );
371370 }
——@@ -653,7 +652,7 @@
654653 $autoblock->mHideName = $this->mHideName;
655654 $autoblock->prevents( 'editownusertalk', $this->prevents( 'editownusertalk' ) );
656655
657 - if ( $this->mExpiry == 'infinity' ) {
 656+ if ( $this->mExpiry == wfGetDB( DB_SLAVE )->getInfinity() ) {
658657 # Original block was indefinite, start an autoblock now
659658 $autoblock->mExpiry = Block::getAutoblockExpiry( $timestamp );
660659 } else {

Follow-up revisions

RevisionCommit summaryAuthorDate
r102629* reverted changes to Block and ApiBlockTest made in r102625 ... PEBKACfreakolowsky09:36, 10 November 2011
r102677* fix as per comment on CR r102625#c25856freakolowsky19:49, 10 November 2011

Comments

#Comment by Aaron Schulz (talk | contribs)   08:49, 10 November 2011

We should only use DB-dependent infinity strings when we *have* too (e.g. saving from the DB). Object fields should use 'infinity' and MW_TS format.

#Comment by Freakolowsky (talk | contribs)   09:10, 10 November 2011

That would mean the old version was wrong i.e.: doAutoblock expects mExpiry to contain db specific infinity. If you have performance considerations because to get infinity you have to go trough a database object, that function could just be statically overloaded.

#Comment by Freakolowsky (talk | contribs)   09:12, 10 November 2011

never mind that overload idea ... fail

#Comment by Freakolowsky (talk | contribs)   09:21, 10 November 2011

actually never mind everything :D PEBKAC

#Comment by Aaron Schulz (talk | contribs)   19:09, 10 November 2011 
+			$this->addWhereIf( "ipb_expiry = '".$db->getInfinity()."'", isset( $show['!temp'] ) );
+			$this->addWhereIf( "ipb_expiry != '".$db->getInfinity()."'", isset( $show['temp'] ) );

This should use $db->addQuotes() instead of manual double-quotes.

#Comment by OverlordQ (talk | contribs)   19:37, 16 November 2011

This breaks postgres.

Postgres already had a sequence for this field called us_id_seq see r100640. You also shouldn't need to explicitly define us_id with sensible table schema specifying the default.

#Comment by OverlordQ (talk | contribs)   19:40, 16 November 2011

meh, ignore me, I realize I named the sequence wrong, digging through old commits we decided to use <tablename>_<tableabbrev_<field>_seq for sequence names, forgot to do that when I added the table.

Status & tagging log