r103691 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r103690‎ | r103691 | r103692 >
Date:17:18, 19 November 2011
Author:reedy
Status:reverted (Comments)
Tags:
Comment:
* (bug 32020) Improve messages output by the database updaters

Patch by David Baumgarten
Modified paths:
  • /trunk/phase3/CREDITS (modified) (history)
  • /trunk/phase3/includes/installer/DatabaseUpdater.php (modified) (history)

Diff [purge]

Index: trunk/phase3/CREDITS
@@ -96,6 +96,7 @@
9797 * Dan Collins
9898 * Dan Nessett
9999 * Daniel Arnold
 100+* David Baumgarten
100101 * Denny Vrandecic
101102 * Edward Z. Yang
102103 * Erwin Dokter
Index: trunk/phase3/includes/installer/DatabaseUpdater.php
@@ -412,10 +412,10 @@
413413 * @param $fullpath Boolean Whether to treat $patch path as a relative or not
414414 */
415415 protected function addTable( $name, $patch, $fullpath = false ) {
 416+ $this->output( "Creating $name table... " );
416417 if ( $this->db->tableExists( $name, __METHOD__ ) ) {
417 - $this->output( "...$name table already exists.\n" );
 418+ $this->output( "...$name table already exists. Skipping create table $name\n" );
418419 } else {
419 - $this->output( "Creating $name table..." );
420420 $this->applyPatch( $patch, $fullpath );
421421 $this->output( "ok\n" );
422422 }
@@ -429,12 +429,12 @@
430430 * @param $fullpath Boolean Whether to treat $patch path as a relative or not
431431 */
432432 protected function addField( $table, $field, $patch, $fullpath = false ) {
 433+ $this->output( "Adding $field field to table $table..." );
433434 if ( !$this->db->tableExists( $table, __METHOD__ ) ) {
434435 $this->output( "...$table table does not exist, skipping new field patch\n" );
435436 } elseif ( $this->db->fieldExists( $table, $field, __METHOD__ ) ) {
436 - $this->output( "...have $field field in $table table.\n" );
 437+ $this->output( "...already have $field field in $table table, skipping new field patch\n" );
437438 } else {
438 - $this->output( "Adding $field field to table $table..." );
439439 $this->applyPatch( $patch, $fullpath );
440440 $this->output( "ok\n" );
441441 }
@@ -448,10 +448,10 @@
449449 * @param $fullpath Boolean Whether to treat $patch path as a relative or not
450450 */
451451 protected function addIndex( $table, $index, $patch, $fullpath = false ) {
 452+ $this->output( "Adding $index key to table $table... " );
452453 if ( $this->db->indexExists( $table, $index, __METHOD__ ) ) {
453 - $this->output( "...$index key already set on $table table.\n" );
 454+ $this->output( "...$index key already set on $table table, skipping new index patch\n" );
454455 } else {
455 - $this->output( "Adding $index key to table $table... " );
456456 $this->applyPatch( $patch, $fullpath );
457457 $this->output( "ok\n" );
458458 }
@@ -466,12 +466,12 @@
467467 * @param $fullpath Boolean Whether to treat $patch path as a relative or not
468468 */
469469 protected function dropField( $table, $field, $patch, $fullpath = false ) {
 470+ $this->output( "Dropping field $field from table $table... " );
470471 if ( $this->db->fieldExists( $table, $field, __METHOD__ ) ) {
471 - $this->output( "Table $table contains $field field. Dropping... " );
472472 $this->applyPatch( $patch, $fullpath );
473473 $this->output( "ok\n" );
474474 } else {
475 - $this->output( "...$table table does not contain $field field.\n" );
 475+ $this->output( "...$table table does not contain $field field, skipping drop field\n" );
476476 }
477477 }
478478
@@ -484,12 +484,12 @@
485485 * @param $fullpath Boolean: Whether to treat $patch path as a relative or not
486486 */
487487 protected function dropIndex( $table, $index, $patch, $fullpath = false ) {
 488+ $this->output( "Dropping $index key from table $table... " );
488489 if ( $this->db->indexExists( $table, $index, __METHOD__ ) ) {
489 - $this->output( "Dropping $index key from table $table... " );
490490 $this->applyPatch( $patch, $fullpath );
491491 $this->output( "ok\n" );
492492 } else {
493 - $this->output( "...$index key doesn't exist.\n" );
 493+ $this->output( "...$index key doesn't exist in table $table, skipping drop key\n" );
494494 }
495495 }
496496
@@ -499,12 +499,12 @@
500500 * @param $fullpath bool
501501 */
502502 protected function dropTable( $table, $patch, $fullpath = false ) {
 503+ $this->output( "Dropping table $table... " );
503504 if ( $this->db->tableExists( $table, __METHOD__ ) ) {
504 - $this->output( "Dropping table $table... " );
505505 $this->applyPatch( $patch, $fullpath );
506506 $this->output( "ok\n" );
507507 } else {
508 - $this->output( "...$table doesn't exist.\n" );
 508+ $this->output( "...$table doesn't exist, skipping drop table.\n" );
509509 }
510510 }
511511
@@ -518,6 +518,7 @@
519519 */
520520 public function modifyField( $table, $field, $patch, $fullpath = false ) {
521521 $updateKey = "$table-$field-$patch";
 522+ $this->output( "Modifying $field field of table $table... " );
522523 if ( !$this->db->tableExists( $table, __METHOD__ ) ) {
523524 $this->output( "...$table table does not exist, skipping modify field patch\n" );
524525 } elseif ( !$this->db->fieldExists( $table, $field, __METHOD__ ) ) {
@@ -525,7 +526,6 @@
526527 } elseif( $this->updateRowExists( $updateKey ) ) {
527528 $this->output( "...$field in table $table already modified by patch $patch\n" );
528529 } else {
529 - $this->output( "Modifying $field field of table $table..." );
530530 $this->applyPatch( $patch, $fullpath );
531531 $this->insertUpdateRow( $updateKey );
532532 $this->output( "ok\n" );

Follow-up revisions

RevisionCommit summaryAuthorDate
r103892revert r103691 that makes update.php output garbage...hashar13:28, 22 November 2011

Comments

#Comment by 😂 (talk | contribs)   23:12, 19 November 2011

This is why I said not to apply this patch as-is, you're undoing the work I did for bug 22753. All lines referring to non-actions should begin with "...". We really should abstract this.

#Comment by Reedy (talk | contribs)   00:14, 20 November 2011

Wouldn't it just make more sense to not output anything when there is no change? Though, extra info is somewhat better than not enough

Unfortunately, when people comment on bugs, leaving vague comments hinting at something that could be anything, it doesn't give any useful guidance.

I know the console isn't a particularily flexible format, but I don't think displaying whether something changed or not is dependent on an ellipse prefix is good usability at all...

#Comment by 😂 (talk | contribs)   00:15, 20 November 2011

I'd be fine with making the extra output optional (--verbose?). Abstracting it first through a single function would make that easier.

#Comment by Reedy (talk | contribs)   00:24, 20 November 2011

That sounds like a good plan. Fixing both the point behind bug 22753 and this bug 32020 properly

I'll part revert this and a couple more of its sorta followups tomorrow

#Comment by Reedy (talk | contribs)   10:35, 20 November 2011

If we're going to go down that road, is there much point reverting these 3 commits?

#Comment by OverlordQ (talk | contribs)   19:36, 21 November 2011

Please do, I dont know who thought it would be a good idea to spew all this extra cruft.

Status & tagging log