Page MenuHomePhabricator
Create Task
Maniphest T263866

1.31.10 patching issues
Closed, ResolvedPublic
Actions

Description

Reported to me via email...

https://releases.wikimedia.org/mediawiki/1.31/mediawiki-1.31.10.patch.gz applies fine ontop of the 1.31.9 git tag...

But if you try and apply https://releases.wikimedia.org/mediawiki/1.31/mediawiki-1.31.10.patch.gz ontop of https://releases.wikimedia.org/mediawiki/1.31/mediawiki-1.31.9.patch.gz it gets rejected (ontop of 1.31.8 git tag)

It looks, offhand something weird to do with whitespace of $this->invalidateCache();

reedy@ubuntu64-web-esxi:/var/www/wiki/mediawiki/tools/release/make-release$ diff mediawiki-1.31.9/includes/user/User.php mediawiki-1.31.10/includes/user/User.php 
2531c2531
< 		if ( !$this->mActorId && $dbw ) {
---
> 		if ( $this->mActorId === null || !$this->mActorId && $dbw ) {
2532a2533,2536
> 			if ( !$dbw ) {
> 				// Read from a database, flags are used for wfGetDB()
> 				$dbw = $this->queryFlagsUsed;
> 			}
2535c2539,2541
< 			$this->invalidateCache();
---
> 			if ( $dbw instanceof IDatabase ) {
> 				$this->invalidateCache();
> 			}
reedy@ubuntu64-web-esxi:/var/www/wiki/mediawiki/tools/release/make-release$ grep User\.php ~/mediawiki-1.31.10.patch -A 20
diff -Nruw mediawiki-1.31.9/includes/user/User.php mediawiki-1.31.10/includes/user/User.php
--- mediawiki-1.31.9/includes/user/User.php	2020-09-24 15:09:03.345337400 +0100
+++ mediawiki-1.31.10/includes/user/User.php	2020-09-25 17:46:24.920534800 +0100
@@ -2528,11 +2528,17 @@
 			$this->load();
 		}
 
-		if ( !$this->mActorId && $dbw ) {
+		if ( $this->mActorId === null || !$this->mActorId && $dbw ) {
 			$migration = MediaWikiServices::getInstance()->getActorMigration();
+			if ( !$dbw ) {
+				// Read from a database, flags are used for wfGetDB()
+				$dbw = $this->queryFlagsUsed;
+			}
 			$this->mActorId = $migration->getNewActorId( $dbw, $this );
 
+			if ( $dbw instanceof IDatabase ) {
 			$this->invalidateCache();
+			}
 			$this->setItemLoaded( 'actor' );
 		}

Applying the patch...

reedy@ubuntu64-web-esxi:~/git/mediawiki/core$ patch -p1 -i mediawiki-1.31.10.patch 
patching file includes/ActorMigration.php
patching file includes/Defines.php
patching file includes/user/User.php
Hunk #1 FAILED at 2528.
1 out of 1 hunk FAILED -- saving rejects to file includes/user/User.php.rej
patching file RELEASE-NOTES-1.31
patching file tests/phpunit/includes/ActorMigrationTest.php

And such

reedy@ubuntu64-web-esxi:~/git/mediawiki/core$ cat includes/user/User.php.rej 
--- includes/user/User.php	2020-09-24 15:09:03.345337400 +0100
+++ includes/user/User.php	2020-09-25 17:46:24.920534800 +0100
@@ -2528,11 +2528,17 @@
 			$this->load();
 		}
 
-		if ( !$this->mActorId && $dbw ) {
+		if ( $this->mActorId === null || !$this->mActorId && $dbw ) {
 			$migration = MediaWikiServices::getInstance()->getActorMigration();
+			if ( !$dbw ) {
+				// Read from a database, flags are used for wfGetDB()
+				$dbw = $this->queryFlagsUsed;
+			}
 			$this->mActorId = $migration->getNewActorId( $dbw, $this );
 
+			if ( $dbw instanceof IDatabase ) {
 			$this->invalidateCache();
+			}
 			$this->setItemLoaded( 'actor' );
 		}

Details

SubjectRepoBranchLines +/-
make-release: Don't generate patch files with -wmediawiki/tools/releasemaster+1 -1
Customize query in gerrit

Related Objects

Event Timeline

Reedy created this task.Sep 25 2020, 5:05 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 25 2020, 5:05 PM
Reedy added a project: MediaWiki-Releasing.Sep 25 2020, 5:10 PM
DannyS712 subscribed.Sep 25 2020, 5:11 PM
Aklapper added a comment.Sep 25 2020, 5:35 PM

Also mentioned in https://www.mediawiki.org/wiki/Topic:Vume16fs3nfisr6w and https://www.mediawiki.org/wiki/Topic:Vulv10re5ha7jv1s

Reedy added a comment.Sep 25 2020, 5:43 PM
In T263866#6494741, @Aklapper wrote:

Also mentioned in https://www.mediawiki.org/wiki/Topic:Vume16fs3nfisr6w and https://www.mediawiki.org/wiki/Topic:Vulv10re5ha7jv1s

Not saying they're not related at all... But the filename truncation sounds like T258716: Filenames truncated in 1.34.2 tarball release (The Unarchiver bug!)... So maybe a mix of different issues

Legoktm added a comment.Sep 25 2020, 6:10 PM

The problem seems to be in the 1.31.9 patch file, which has indentation in it that doesn't match the 1.31.9 tag nor tarball. Here's a diff of applying the patch file (from 1.31.8) versus the git tag:

P12799 1.31.9 patch weirdness
1diff --git a/includes/MediaWiki.php b/includes/MediaWiki.php
2index 9fdd48a777..e6709c718b 100644
3--- a/includes/MediaWiki.php
4+++ b/includes/MediaWiki.php
5@@ -792,8 +792,8 @@ class MediaWiki {
6 }
7
8 if ( $this->maybeDoHttpsRedirect() ) {
9- return;
10- }
11+ return;
12+ }
13
14 if ( $title->canExist() && HTMLFileCache::useFileCache( $this->context ) ) {
15 // Try low-level file cache hit
16diff --git a/includes/WebResponse.php b/includes/WebResponse.php
17index 46cb844fcb..445822cab1 100644
18--- a/includes/WebResponse.php
19+++ b/includes/WebResponse.php
20@@ -177,12 +177,12 @@ class WebResponse {
21 'cookie' => $prefixedName,
22 'data' => [
23 'name' => $prefixedName,
24- 'value' => (string)$value,
25- 'expire' => (int)$expire,
26- 'path' => (string)$options['path'],
27- 'domain' => (string)$options['domain'],
28- 'secure' => (bool)$options['secure'],
29- 'httpOnly' => (bool)$options['httpOnly'],
30+ 'value' => (string)$value,
31+ 'expire' => (int)$expire,
32+ 'path' => (string)$options['path'],
33+ 'domain' => (string)$options['domain'],
34+ 'secure' => (bool)$options['secure'],
35+ 'httpOnly' => (bool)$options['httpOnly'],
36 'sameSite' => (string)$options['sameSite']
37 ],
38 'exception' => new RuntimeException( 'Ignored post-send cookie' ),
39@@ -206,33 +206,33 @@ class WebResponse {
40 'secure' => (bool)$options['secure'],
41 'httponly' => (bool)$options['httpOnly'],
42 'samesite' => (string)$options['sameSite'],
43- ];
44+ ];
45
46- // Per RFC 6265, key is name + domain + path
47+ // Per RFC 6265, key is name + domain + path
48 $key = "{$prefixedName}\n{$setOptions['domain']}\n{$setOptions['path']}";
49
50- // If this cookie name was in the request, fake an entry in
51- // self::$setCookies for it so the deleting check works right.
52+ // If this cookie name was in the request, fake an entry in
53+ // self::$setCookies for it so the deleting check works right.
54 if ( isset( $_COOKIE[$prefixedName] ) && !array_key_exists( $key, self::$setCookies ) ) {
55- self::$setCookies[$key] = [];
56- }
57+ self::$setCookies[$key] = [];
58+ }
59
60- // PHP deletes if value is the empty string; also, a past expiry is deleting
61+ // PHP deletes if value is the empty string; also, a past expiry is deleting
62 $deleting = ( $value === '' || $setOptions['expires'] > 0 && $setOptions['expires'] <= time() );
63
64 $logDesc = "$func: \"$prefixedName\", \"$value\", \"" .
65 implode( '", "', $setOptions ) . '"';
66 $optionsForDeduplication = [ $func, $prefixedName, $value, $setOptions ];
67
68- if ( $deleting && !isset( self::$setCookies[$key] ) ) { // isset( null ) is false
69+ if ( $deleting && !isset( self::$setCookies[$key] ) ) { // isset( null ) is false
70 wfDebugLog( 'cookie', "already deleted $logDesc" );
71 return;
72- } elseif ( !$deleting && isset( self::$setCookies[$key] ) &&
73+ } elseif ( !$deleting && isset( self::$setCookies[$key] ) &&
74 self::$setCookies[$key] === $optionsForDeduplication
75- ) {
76+ ) {
77 wfDebugLog( 'cookie', "already set $logDesc" );
78 return;
79- }
80+ }
81
82 wfDebugLog( 'cookie', $logDesc );
83 if ( $func === 'setrawcookie' ) {
84diff --git a/includes/api/ApiMain.php b/includes/api/ApiMain.php
85index 98b541e522..02ef2b8b81 100644
86--- a/includes/api/ApiMain.php
87+++ b/includes/api/ApiMain.php
88@@ -1541,9 +1541,9 @@ class ApiMain extends ApiBase {
89 if ( $request->getProtocol() === 'http' &&
90 (
91 $this->getConfig()->get( 'ForceHTTPS' ) ||
92- $request->getSession()->shouldForceHTTPS() ||
93- ( $this->getUser()->isLoggedIn() &&
94- $this->getUser()->requiresHTTPS() )
95+ $request->getSession()->shouldForceHTTPS() ||
96+ ( $this->getUser()->isLoggedIn() &&
97+ $this->getUser()->requiresHTTPS() )
98 )
99 ) {
100 $this->addDeprecation( 'apiwarn-deprecation-httpsexpected', 'https-expected' );
101diff --git a/includes/session/CookieSessionProvider.php b/includes/session/CookieSessionProvider.php
102index 69bd9372e7..4ea7b8581a 100644
103--- a/includes/session/CookieSessionProvider.php
104+++ b/includes/session/CookieSessionProvider.php
105@@ -356,7 +356,7 @@ class CookieSessionProvider extends SessionProvider {
106 if ( $this->useCrossSiteCookies ) {
107 $value = $request->getCrossSiteCookie( $key, $prefix, $default );
108 } else {
109- $value = $request->getCookie( $key, $prefix, $default );
110+ $value = $request->getCookie( $key, $prefix, $default );
111 }
112 if ( $value === 'deleted' ) {
113 // PHP uses this value when deleting cookies. A legitimate cookie will never have
114diff --git a/includes/session/ImmutableSessionProviderWithCookie.php b/includes/session/ImmutableSessionProviderWithCookie.php
115index 1e1752511b..72c0559d0a 100644
116--- a/includes/session/ImmutableSessionProviderWithCookie.php
117+++ b/includes/session/ImmutableSessionProviderWithCookie.php
118@@ -115,8 +115,8 @@ abstract class ImmutableSessionProviderWithCookie extends SessionProvider {
119 if ( $session->shouldForceHTTPS() || $session->getUser()->requiresHTTPS() ) {
120 // Send a cookie unless $wgForceHTTPS is set (T256095)
121 if ( !$this->config->get( 'ForceHTTPS' ) ) {
122- $response->setCookie( 'forceHTTPS', 'true', null,
123- [ 'prefix' => '', 'secure' => false ] + $options );
124+ $response->setCookie( 'forceHTTPS', 'true', null,
125+ [ 'prefix' => '', 'secure' => false ] + $options );
126 }
127 $options['secure'] = true;
128 }
129diff --git a/includes/shell/Command.php b/includes/shell/Command.php
130index 4ddb7df38d..61f9b9c068 100644
131--- a/includes/shell/Command.php
132+++ b/includes/shell/Command.php
133@@ -400,7 +400,7 @@ class Command {
134 // This solves some shell parsing issues, see T207248
135 $proc = proc_open( $cmd, $desc, $pipes, null, null, [ 'bypass_shell' => true ] );
136 } else {
137- $proc = proc_open( $cmd, $desc, $pipes );
138+ $proc = proc_open( $cmd, $desc, $pipes );
139 }
140
141 if ( !$proc ) {
142diff --git a/includes/user/User.php b/includes/user/User.php
143index 01fc6e910e..0bd7c1536b 100644
144--- a/includes/user/User.php
145+++ b/includes/user/User.php
146@@ -2160,16 +2160,16 @@ class User implements IDBAccessObject, UserIdentity {
147 } else {
148 // Check for group-specific limits
149 // If more than one group applies, use the highest allowance (if higher than the default)
150- foreach ( $this->getGroups() as $group ) {
151- if ( isset( $limits[$group] ) ) {
152- if ( $userLimit === false
153- || $limits[$group][0] / $limits[$group][1] > $userLimit[0] / $userLimit[1]
154- ) {
155- $userLimit = $limits[$group];
156+ foreach ( $this->getGroups() as $group ) {
157+ if ( isset( $limits[$group] ) ) {
158+ if ( $userLimit === false
159+ || $limits[$group][0] / $limits[$group][1] > $userLimit[0] / $userLimit[1]
160+ ) {
161+ $userLimit = $limits[$group];
162+ }
163 }
164 }
165 }
166- }
167
168 // Set the user limit key
169 if ( $userLimit !== false ) {
170@@ -2224,12 +2224,12 @@ class User implements IDBAccessObject, UserIdentity {
171 // phan is confused because &can-bypass's value is a bool, so it assumes
172 // that $userLimit is also a bool here.
173 // @phan-suppress-next-line PhanTypeInvalidExpressionArrayDestructuring
174- list( $max, $period ) = $limit;
175+ list( $max, $period ) = $limit;
176
177 $expiry = $now + (int)$period;
178 $count = 0;
179
180- // Already pinged?
181+ // Already pinged?
182 if ( $data ) {
183 // NOTE: in order to investigate T246991, we write the expiry time
184 // into the payload, along with the count.
185@@ -2252,13 +2252,13 @@ class User implements IDBAccessObject, UserIdentity {
186 'expiry' => MWTimestamp::convert( TS_DB, $storedExpiry ),
187 ]
188 );
189- } else {
190+ } else {
191 // NOTE: We'll keep the original expiry when bumping counters,
192 // resulting in a kind of fixed-window throttle.
193 $expiry = min( $storedExpiry, $now + (int)$period );
194 $count = $storedCount;
195- }
196- }
197+ }
198+ }
199
200 // Limit exceeded!
201 if ( $count >= $max ) {
202@@ -2278,12 +2278,12 @@ class User implements IDBAccessObject, UserIdentity {
203 }
204
205 $triggered = true;
206- }
207+ }
208
209 $count += $incrBy;
210 $data = "$count|$expiry";
211 return $data;
212- }
213+ }
214 );
215 }
216
217@@ -2532,7 +2532,7 @@ class User implements IDBAccessObject, UserIdentity {
218 $migration = MediaWikiServices::getInstance()->getActorMigration();
219 $this->mActorId = $migration->getNewActorId( $dbw, $this );
220
221- $this->invalidateCache();
222+ $this->invalidateCache();
223 $this->setItemLoaded( 'actor' );
224 }
225
226diff --git a/maintenance/dumpCategoriesAsRdf.php b/maintenance/dumpCategoriesAsRdf.php
227index 454ac6dd26..7b7c9b5def 100644
228--- a/maintenance/dumpCategoriesAsRdf.php
229+++ b/maintenance/dumpCategoriesAsRdf.php
230@@ -157,8 +157,8 @@ class DumpCategoriesAsRdf extends Maintenance {
231 (int)$row->cat_subcats
232 );
233 if ( $row->page_id ) {
234- $pages[$row->page_id] = $row->page_title;
235- }
236+ $pages[$row->page_id] = $row->page_title;
237+ }
238 }
239
240 foreach ( $this->getCategoryLinksIterator( $dbr, array_keys( $pages ) ) as $row ) {
241diff --git a/tests/phpunit/includes/session/CookieSessionProviderTest.php b/tests/phpunit/includes/session/CookieSessionProviderTest.php
242index 1a32845fdc..87be58ef85 100644
243--- a/tests/phpunit/includes/session/CookieSessionProviderTest.php
244+++ b/tests/phpunit/includes/session/CookieSessionProviderTest.php
245@@ -456,7 +456,7 @@ class CookieSessionProviderTest extends MediaWikiTestCase {
246 if ( $forceHTTPS ) {
247 $this->assertSame( null, $request->response()->getCookie( 'forceHTTPS' ) );
248 } else {
249- $this->assertSame( '', $request->response()->getCookie( 'forceHTTPS' ) );
250+ $this->assertSame( '', $request->response()->getCookie( 'forceHTTPS' ) );
251 }
252 $this->assertSame( [], $backend->getData() );
253
254@@ -473,7 +473,7 @@ class CookieSessionProviderTest extends MediaWikiTestCase {
255 if ( $forceHTTPS ) {
256 $this->assertSame( null, $request->response()->getCookie( 'forceHTTPS' ) );
257 } else {
258- $this->assertSame( '', $request->response()->getCookie( 'forceHTTPS' ) );
259+ $this->assertSame( '', $request->response()->getCookie( 'forceHTTPS' ) );
260 }
261 $this->assertSame( [], $backend->getData() );
262
263@@ -491,7 +491,7 @@ class CookieSessionProviderTest extends MediaWikiTestCase {
264 if ( $forceHTTPS ) {
265 $this->assertSame( null, $request->response()->getCookie( 'forceHTTPS' ) );
266 } else {
267- $this->assertSame( 'true', $request->response()->getCookie( 'forceHTTPS' ) );
268+ $this->assertSame( 'true', $request->response()->getCookie( 'forceHTTPS' ) );
269 }
270 $this->assertSame( [], $backend->getData() );
271 }
272diff --git a/tests/phpunit/includes/shell/CommandTest.php b/tests/phpunit/includes/shell/CommandTest.php
273index 8f1ba7a939..08a583cdac 100644
274--- a/tests/phpunit/includes/shell/CommandTest.php
275+++ b/tests/phpunit/includes/shell/CommandTest.php
276@@ -115,8 +115,8 @@ class CommandTest extends PHPUnit\Framework\TestCase {
277 if ( wfIsWindows() ) {
278 $this->assertEquals( '"echo" "a" "b" c d', $command->command );
279 } else {
280- $this->assertEquals( "'echo' 'a' 'b' c d", $command->command );
281- }
282+ $this->assertEquals( "'echo' 'a' 'b' c d", $command->command );
283+ }
284 }
285
286 public function testT69870() {
287diff --git a/tests/phpunit/includes/shell/ShellTest.php b/tests/phpunit/includes/shell/ShellTest.php
288index 934f32ec6a..6c1c14e8d7 100644
289--- a/tests/phpunit/includes/shell/ShellTest.php
290+++ b/tests/phpunit/includes/shell/ShellTest.php
291@@ -49,10 +49,10 @@ class ShellTest extends MediaWikiTestCase {
292 public function testMakeScriptCommand(
293 $expected,
294 $expectedWin,
295- $script,
296- $parameters,
297- $options = [],
298- $hook = null
299+ $script,
300+ $parameters,
301+ $options = [],
302+ $hook = null
303 ) {
304 // Running tests under Vagrant involves MWMultiVersion that uses the below hook
305 $this->setMwGlobals( 'wgHooks', [] );
306@@ -72,7 +72,7 @@ class ShellTest extends MediaWikiTestCase {
307 if ( wfIsWindows() ) {
308 $this->assertEquals( $expectedWin, $wrapper->command );
309 } else {
310- $this->assertEquals( $expected, $wrapper->command );
311+ $this->assertEquals( $expected, $wrapper->command );
312 }
313 $this->assertSame( 0, $wrapper->restrictions & Shell::NO_LOCALSETTINGS );
314 }

Legoktm added a comment.Sep 25 2020, 6:19 PM

The culprit is that the patch file is generated with diff -Nruw, in which -w problematically ignores any whitespace changes, and by pure bad luck we have patches that touch those same lines.

gerritbot added a comment.Sep 25 2020, 6:21 PM

Change 630222 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[mediawiki/tools/release@master] make-release: Don't generate patch files with -w

https://gerrit.wikimedia.org/r/630222

gerritbot added a project: Patch-For-Review.Sep 25 2020, 6:21 PM
gerritbot added a comment.Sep 25 2020, 6:25 PM

Change 630222 merged by jenkins-bot:
[mediawiki/tools/release@master] make-release: Don't generate patch files with -w

https://gerrit.wikimedia.org/r/630222

Reedy triaged this task as High priority.Sep 25 2020, 6:30 PM
Maintenance_bot removed a project: Patch-For-Review.Sep 25 2020, 7:10 PM
Legoktm mentioned this in T263883: Verify MediaWiki patch files apply in successive upgrades.Sep 25 2020, 7:37 PM
Reedy closed this task as Resolved.Sep 25 2020, 7:50 PM
Reedy assigned this task to Legoktm.

Thanks to Lego for the help!

I've re-released the .9 and .10 patches with whitespace diff appropriate versions.

https://releases.wikimedia.org/mediawiki/1.31/mediawiki-1.31.10-fixup.patch.gz has been created for those running the old .9 version to get to the correct .10 version.

Will announce it properly too

Legoktm mentioned this in T293323: Transitioning Responsibility for MediaWiki Releases.Nov 23 2021, 5:50 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL