r103706 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r103705‎ | r103706 | r103707 >
Date:20:17, 19 November 2011
Author:reedy
Status:reverted (Comments)
Tags:
Comment:
* (bug 8859) Database::update should take array of tables too

Original patch by Andrew Dunbar

Also apply in DatabaseOracle and DB2 code as they override update
Modified paths:
  • /trunk/phase3/includes/db/Database.php (modified) (history)
  • /trunk/phase3/includes/db/DatabaseIbm_db2.php (modified) (history)
  • /trunk/phase3/includes/db/DatabaseOracle.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/db/DatabaseOracle.php
——@@ -1197,7 +1197,11 @@
11981198 function update( $table, $values, $conds, $fname = 'DatabaseOracle::update', $options = array() ) {
11991199 global $wgContLang;
12001200
1201 - $table = $this->tableName( $table );
 1201+ if ( is_array( $table ) ) {
 1202+ $table = implode( ',', array_map( array( $this, 'tableName' ), $table ) );
 1203+ } else {
 1204+ $table = $this->tableName( $table );
 1205+ }
12021206 $opts = $this->makeUpdateOptions( $options );
12031207 $sql = "UPDATE $opts $table SET ";
12041208
Index: trunk/phase3/includes/db/DatabaseIbm_db2.php
——@@ -984,7 +984,11 @@
985985 public function update( $table, $values, $conds, $fname = 'DatabaseIbm_db2::update',
986986 $options = array() )
987987 {
988 - $table = $this->tableName( $table );
 988+ if ( is_array( $table ) ) {
 989+ $table = implode( ',', array_map( array( $this, 'tableName' ), $table ) );
 990+ } else {
 991+ $table = $this->tableName( $table );
 992+ }
989993 $opts = $this->makeUpdateOptions( $options );
990994 $sql = "UPDATE $opts $table SET "
991995 . $this->makeList( $values, LIST_SET_PREPARED );
Index: trunk/phase3/includes/db/Database.php
——@@ -1683,28 +1683,32 @@
16841684 /**
16851685 * UPDATE wrapper. Takes a condition array and a SET array.
16861686 *
1687 - * @param $table String name of the table to UPDATE. This will be passed through
 1687+ * @param $table String|array name of the table to UPDATE. This will be passed through
16881688 * DatabaseBase::tableName().
16891689 *
1690 - * @param $values Array: An array of values to SET. For each array element,
 1690+ * @param $values Array An array of values to SET. For each array element,
16911691 * the key gives the field name, and the value gives the data
16921692 * to set that field to. The data will be quoted by
16931693 * DatabaseBase::addQuotes().
16941694 *
1695 - * @param $conds Array: An array of conditions (WHERE). See
 1695+ * @param $conds Array An array of conditions (WHERE). See
16961696 * DatabaseBase::select() for the details of the format of
16971697 * condition arrays. Use '*' to update all rows.
16981698 *
1699 - * @param $fname String: The function name of the caller (from __METHOD__),
 1699+ * @param $fname String The function name of the caller (from __METHOD__),
17001700 * for logging and profiling.
17011701 *
1702 - * @param $options Array: An array of UPDATE options, can be:
 1702+ * @param $options Array An array of UPDATE options, can be:
17031703 * - IGNORE: Ignore unique key conflicts
17041704 * - LOW_PRIORITY: MySQL-specific, see MySQL manual.
17051705 * @return Boolean
17061706 */
17071707 function update( $table, $values, $conds, $fname = 'DatabaseBase::update', $options = array() ) {
1708 - $table = $this->tableName( $table );
 1708+ if ( is_array( $table ) ) {
 1709+ $table = implode( ',', array_map( array( $this, 'tableName' ), $table ) );
 1710+ } else {
 1711+ $table = $this->tableName( $table );
 1712+ }
17091713 $opts = $this->makeUpdateOptions( $options );
17101714 $sql = "UPDATE $opts $table SET " . $this->makeList( $values, LIST_SET );
17111715

Follow-up revisions

RevisionCommit summaryAuthorDate
r103710RELEASE-NOTES-1.19 for r103706, r103708...reedy21:17, 19 November 2011
r104926Reverting r103706...reedy00:29, 2 December 2011

Comments

#Comment by Brion VIBBER (talk | contribs)   02:09, 23 November 2011

To be useful, wouldn't this need to take join conditions and such?

#Comment by Reedy (talk | contribs)   22:31, 28 November 2011

Probably.

And nicely, it'll keep the same parameter order as select 

	function select( $table, $vars, $conds = '', $fname = 'DatabaseBase::select', $options = array(), $join_conds = array() ) {

That makes a change! :D

#Comment by Reedy (talk | contribs)   23:57, 1 December 2011

http://dev.mysql.com/doc/refman/5.0/en/update.html

Multiple-table syntax: 

UPDATE [LOW_PRIORITY] [IGNORE] table_references
    SET col_name1={expr1|DEFAULT} [, col_name2={expr2|DEFAULT}] ...
    [WHERE where_condition]

For MySQL at least, there is no join conditions on an update...

You'd have to do it $cods...

Having said that...

http://www.sqlite.org/lang_update.html

SQLite doesn't do multiple table updates

http://www.postgresql.org/docs/8.2/static/sql-update.html

Same for Postgres

http://psoug.org/reference/update.html

And seemingly for Oracle


Seems to make more sense to just revert this commit, and close the bug as essentially invalid?

#Comment by Reedy (talk | contribs)   00:02, 2 December 2011

Bah, not made obvious 

The preceding example shows an inner join that uses the comma operator, but multiple-table UPDATE statements can use any type of join permitted in SELECT statements, such as LEFT JOIN.


But still, seemingly only mysql

#Comment by Brion VIBBER (talk | contribs)   00:22, 2 December 2011

Yeah, if nothing else supports this and we have no intent to use it anyway.... no reason to keep it. :)

Status & tagging log