r62744 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r62743‎ | r62744 | r62745 >
Date:18:27, 20 February 2010
Author:freakolowsky
Status:resolved (Comments)
Tags:
Comment:
tableName has to be encoded before field type checking
Modified paths:
  • /trunk/phase3/includes/db/DatabaseOracle.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/db/DatabaseOracle.php
——@@ -283,9 +283,9 @@
284284
285285 function doQuery( $sql ) {
286286 wfDebug( "SQL: [$sql]\n" );
287 - if ( !mb_check_encoding( $sql ) ) {
288 - throw new MWException( "SQL encoding is invalid\n$sql" );
289 - }
 287+ if ( !mb_check_encoding( $sql ) ) {
 288+ throw new MWException( "SQL encoding is invalid\n$sql" );
 289+ }
290290
291291 // handle some oracle specifics
292292 // remove AS column/table/subquery namings
——@@ -1024,6 +1024,7 @@
10251025 if (is_array($table)) {
10261026 $table = array_map( array( &$this, 'tableName' ), $table );
10271027 }
 1028+ $table = $this->tableName($table);
10281029
10291030 $conds2 = array();
10301031 $conds = ($conds != null && !is_array($conds)) ? array($conds) : $conds;

Follow-up revisions

RevisionCommit summaryAuthorDate
r62748Fix for r62744: use tabs for indentation, not spacesialex19:22, 20 February 2010

Comments

#Comment by Jack Phoenix (talk | contribs)   19:12, 20 February 2010

Please use tabs for indenting code, not spaces. You accidentally changed tabs to spaces here. Please also remember that our coding style is very spacey. So $table = $this->tableName($table); should be $table = $this->tableName( $table );

#Comment by Simetrical (talk | contribs)   20:38, 21 February 2010

What happens here if a query updates a binary blob? Surely that will result in an invalid encoding, but might nevertheless be correct?

#Comment by Freakolowsky (talk | contribs)   23:01, 21 February 2010

As long as blob or clob value can be casted to a varchar or raw value there should be no problem ... if it can't be cast there is no other option as to use an insert or replace wrapper or something. But as long as people stick to abstraction wrappers and don't use doQuery for everything this should play.

#Comment by Tim Starling (talk | contribs)   10:33, 22 February 2010

You pass the encoded table name through to parent::selectRow(), where it will be passed through tableName() a second time. Presumably this is not intended.

DatabaseOracle::fieldInfo() requires table names to be converted before calling, but this is not the case with the other implementations, such as DatabaseMysql::fieldInfo(). I think you should have added the tableName() call to fieldInfoMulti(), instead of selectRow().

#Comment by Freakolowsky (talk | contribs)   14:43, 22 February 2010

So what you're saying is that calling tableName column-count-times is better than calling it one time, just because mysql does it that way. I have no problem with changing it, just don't see the point in changing it from one to x calls "just because mysql does it that way".

But then again ... that may be just me.

#Comment by Simetrical (talk | contribs)   15:13, 22 February 2010

If the point of DBMS abstraction layers is to present a consistent interface regardless of the underlying DBMS, then yes, you should be presenting the same interfaces as the other classes, just because they do it that way. fieldInfo() is a public function and cannot have different meanings in different classes (e.g., one requiring you convert the table name first and another not).

#Comment by Freakolowsky (talk | contribs)   15:40, 22 February 2010

point taken ... will change

#Comment by Freakolowsky (talk | contribs)   10:39, 23 February 2010

fixed in r62869

Status & tagging log