[06:45:36] legoktm: looking at UrlShortener: this extension will actually emit Location headers with spaces in the URL if requested, correct? [06:46:09] I checked, it does at least filter out control characters such as line breaks [06:46:09] Yes [06:47:19] hopefully spaces won't confuse clients in a security-relevant way [06:49:57] Hm, are there any cases where str_replace(' ', '%20') would be problematic? [06:50:06] (applying that to all URLs) [06:51:00] no, that would be fine [06:51:53] maybe trim the input if that's not done already [06:52:12] url.spec.whatwg.org says it's an error for a URL to start or end with a space [07:09:22] TimStarling: https://gerrit.wikimedia.org/r/374702 [07:16:17] * legoktm zzz [15:21:56] anomie: Congratulations on 357892 being merged! A major step. :-) [15:24:30] Now I get to start on updating extensions to use it instead of accessing comment fields directly. [15:24:56] Indeed. [15:29:47] Uh... [15:29:52] update.php is broken on my wiki :( [15:30:40] https://phabricator.wikimedia.org/P5947 [15:31:24] I'll fix it [15:33:44] anomie: https://gerrit.wikimedia.org/r/374833 [16:12:34] Reedy: Care to make the same fix in maintenance/sqlite/archives/patch-comment-table.sql? [16:41:45] anomie: done [16:55:05] that's better [18:33:54] anomie: Postgres still broken, sadly. https://travis-ci.org/wikimedia/mediawiki/builds [18:35:20] At least it's getting farther. [18:38:31] True. [18:38:48] Error: 23502 ERROR: null value in column "ipb_by" violates not-null constraint [18:40:08] It'd be nice if the developers who get upset when postgres gets broken... Actually pitched in [18:41:06] rror: 42703 ERROR: column "imgcomment_description_id" of relation "unittest_image_comment_temp" does not exist [18:41:06] LINE 1: ...TO "unittest_image_comment_temp" (imgcomment_name,imgcomment... [18:43:01] * anomie will look in a few minutes [18:43:40] the travis page keeps becoming unusable [18:44:26] imgcomment_comment_id INTEGER NOT NULL, [18:45:23] imgcomment_comment_id in the schema is another copy-paste error on my part. [18:45:30] I was just about to ask [18:45:41] I'll fix up those in postgres tables.sql if you want? [18:45:46] and the patch file [18:47:45] https://gerrit.wikimedia.org/r/374855 [18:49:11] The question is how many of the errors does that fix? [18:50:34] There's 372 instances of imgcomment_description_id in the travis pg log [18:50:42] Oh, no, 620 [18:50:51] ~2 per test [18:50:53] So 300+ ? [18:51:13] 1450 errors [18:51:18] So over 20% [18:56:00] Should be make the next run log results somewhat more useable [18:58:07] "This log is too long to be displayed. Please reduce the verbosity of your build or download the raw log." [18:58:08] Aha [18:58:12] anomie: Potentially a lot more than that [18:58:39] 4.5MB log file [18:58:39] lol [18:58:48] * anomie attempts to run the tests locally against PG [18:58:52] 2704 matches [18:59:11] The log length has exceeded the limit of 4 MB (this usually means that the test suite is raising the same exception over and over). [18:59:13] o_0 [18:59:26] Merge it and see where we get to next :) [19:42:25] Ugh, why do so many fields not have defaults in PG where they do in MySQL. [19:52:04] because constraints and shit [20:17:38] I wonder what about 99c80a8f made my local unit tests fail in JobQueueTest (P5903). [20:47:00] Reedy: What would you think about killing PG's need for nextSequenceValue() now (and ignoring Oracle), instead of waiting for the rest of T164898? The ordering constraints there are a pain. [20:47:01] T164898: PostgreSQL schema change for consistency with MySQL - https://phabricator.wikimedia.org/T164898 [21:01:56] RFC meeting starting now in #wikimedia-office: Migrate to HTML5 section ids [21:06:50] anomie: How bad are the postgres tests after fixing the schema issue? [21:08:09] Reedy: I found 4 other things needing fixing. And then there are two things failing thanks to having a call to CommentStore::insert() in between a call to nextSequenceValue() and insertId() for the main table, and I suspect there are probably others that aren't being caught by the test. [21:09:23] Hmm [21:09:29] Incremental improvement at least [21:09:37] Not like MW 1.30 is due for release any time soon [21:10:47] anomie: It's also like I said before... It'd nice nice if the people that cared about postgres actually helped [21:10:52] Rather than just bitched when it gets broken [21:14:59] anomie: want to merge my patch to fix the schema copy pasta errors, and then I'll see how it looks on travis [21:19:55] Reedy: https://gerrit.wikimedia.org/r/374893 would probably be better. [21:21:49] looks sane [21:21:59] Can't test easily though [21:24:27] * Reedy waits to see what jerkins has to say [22:06:19] anomie: https://travis-ci.org/wikimedia/mediawiki/jobs/270188653 [22:06:23] 3 errors, 7 failures [22:07:43] Reedy: I'm pretty sure those are due to the nextSequenceValue ordering thing. [22:10:59] anomie: hey! about that 3k line patch I cced you on: most of the code isn't new, it just moved. I have been thinking that I should add comments like // this used to be Revision::newFromId [22:11:02] would that help? [22:11:02] The wikipage delete one doesn't look to be [22:13:20] [ 'revcomment_rev' => $revids ] [22:13:50] Ah [22:13:50] if ( $wgCommentTableSchemaMigrationStage > MIGRATION_OLD ) { [22:13:50] $dbw->delete( 'revision_comment_temp', [ 'revcomment_rev' => $revids ], __METHOD__ ); [22:13:50] } [22:14:05] anomie: I guess just stick a count( $revids ) on that if [22:15:28] as makeList doesn't like empty arrays [22:15:35] Though, shouldn't it have stuck something in there already? [22:17:21] $revids being empty means no revisions selected [22:26:49] no entry in revision_comment_temp after deletion [22:26:50] Failed asserting that '1' matches expected 0. [22:26:59] that doesn't sound like next seq either...