Skip to content

Commit 29d86e8

Browse files
Krinklereedy
Krinkle
authored and
reedy
committed
build: Add phan and address outstanding warnings
* Reader: Move $handle to subclasses as its an implementation that the abstract class has no use for (also, Reader\PHP doesn't need it). Also stop unset'ing it and assign null instead, to address Phan warnings. * Reader: Remove constructor from the shared interface. After construction, the reader can be passed around without typing to a specific implementation, but the constructor itself cannot be substituted since the caller can't decide which class is right, plus readers like Reader\PHP take an array as parameter instead of a filename. This solves a Phan warning about incompatible constructors in the implementation of `abstract public __construct`. * Cli: Rename internal $argv parameter to $args because it seems our Phan SecurityCheckPlugin is treating this special and is refusing to untaint it, thus yielding security warnings in a way we can't correct. * Writer: Similarly move $handle to subclasses. Unlike Reader, the abstract Writer did actually reference $handle, namely in the destructor method to automatically call close(), but only if it wasn't called already, based on $handle being null. Rewrite this by having the destructor always call close(), and making close() safe to call repeatedly. Previously if close() was called twice, the second one would fail due to only 1 of the statements being conditional on $handle, the rest would fail. The close methods are not entirely conditional on $handle. This change is covered by tests, simplifying only the destructor without changing close() leads to failing tests (as expected). Change-Id: I93ed8dd17daaadfc62173938abd7686d7753715a
1 parent 6e921a1 commit 29d86e8

File tree

10 files changed

+68
-69
lines changed

10 files changed

+68
-69
lines changed

‎.phan/config.php‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
<?php
2+
return require __DIR__ . '/../vendor/mediawiki/mediawiki-phan-config/src/config.php';

‎composer.json‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
},
3131
"require-dev": {
3232
"mediawiki/mediawiki-codesniffer": "38.0.0",
33+
"mediawiki/mediawiki-phan-config": "0.11.1",
3334
"mediawiki/minus-x": "1.1.1",
3435
"ockcyp/covers-validator": "1.3.3",
3536
"php-parallel-lint/php-console-highlighter": "0.5.0",
@@ -41,6 +42,7 @@
4142
"parallel-lint . --exclude vendor",
4243
"phpunit",
4344
"covers-validator",
45+
"@phan",
4446
"@phpcs",
4547
"minus-x check ."
4648
],
@@ -49,6 +51,7 @@
4951
"minus-x fix .",
5052
"phpcbf"
5153
],
54+
"phan": "phan --allow-polyfill-parser --no-progress-bar",
5255
"phpcs": "phpcs -sp"
5356
}
5457
}

‎src/Cli.php‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -42,14 +42,14 @@ final class Cli {
4242

4343
/**
4444
* @param resource $out An open output handle for fwrite()
45-
* @param string[] $argv
45+
* @param string[] $args
4646
*/
47-
public function __construct( $out, array $argv ) {
47+
public function __construct( $out, array $args ) {
4848
$this->out = $out;
49-
$this->self = $argv[0] ?? './bin/cdb';
50-
$this->filepath = $argv[1] ?? '';
51-
$this->action = $argv[2] ?? '';
52-
$this->params = array_slice( $argv, 3 );
49+
$this->self = $args[0] ?? './bin/cdb';
50+
$this->filepath = $args[1] ?? '';
51+
$this->action = $args[2] ?? '';
52+
$this->params = array_slice( $args, 3 );
5353
}
5454

5555
/** Main method. */
@@ -72,7 +72,7 @@ public function run() {
7272
}
7373
} catch ( \Throwable $e ) {
7474
$this->exitCode = 1;
75-
$this->output( $e );
75+
$this->output( (string)$e );
7676
}
7777
}
7878

@@ -118,6 +118,7 @@ private function runMatch(): void {
118118
return;
119119
}
120120
$pattern = $this->params[0];
121+
// @phan-suppress-next-line PhanParamSuspiciousOrder
121122
if ( preg_match( $pattern, '' ) === false ) {
122123
$this->error( 'Invalid regular expression pattern.' );
123124
return;

‎src/Reader.php‎

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -25,11 +25,6 @@
2525
* @see http://cr.yp.to/cdb.html
2626
*/
2727
abstract class Reader {
28-
/**
29-
* The file handle
30-
*/
31-
protected $handle;
32-
3328
/**
3429
* Open a file and return a subclass instance
3530
*
@@ -60,13 +55,6 @@ public static function haveExtension() {
6055
return true;
6156
}
6257

63-
/**
64-
* Create the object and open the file
65-
*
66-
* @param string $fileName
67-
*/
68-
abstract public function __construct( $fileName );
69-
7058
/**
7159
* Close the file. Optional, you can just let the variable go out of scope.
7260
*/
@@ -88,11 +76,15 @@ abstract public function exists( $key );
8876

8977
/**
9078
* Fetch first key
79+
*
80+
* @return string|false
9181
*/
9282
abstract public function firstkey();
9383

9484
/**
9585
* Fetch next key
86+
*
87+
* @return string|false
9688
*/
9789
abstract public function nextkey();
9890
}

‎src/Reader/DBA.php‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,11 @@
2525
* Reader class which uses the DBA extension (php-dba)
2626
*/
2727
class DBA extends Reader {
28+
/**
29+
* The file handle
30+
*/
31+
protected $handle;
32+
2833
public function __construct( $fileName ) {
2934
$this->handle = dba_open( $fileName, 'r-', 'cdb' );
3035
if ( !$this->handle ) {
@@ -33,10 +38,10 @@ public function __construct( $fileName ) {
3338
}
3439

3540
public function close() {
36-
if ( isset( $this->handle ) ) {
41+
if ( $this->handle ) {
3742
dba_close( $this->handle );
3843
}
39-
unset( $this->handle );
44+
$this->handle = null;
4045
}
4146

4247
public function get( $key ) {

‎src/Reader/Hash.php‎

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@
2525
* PHP array (a.k.a "hash").
2626
*/
2727
class Hash extends Reader {
28-
/** @var string $data */
28+
/** @var string[] $data */
2929
private $data;
3030

3131
/**
@@ -59,18 +59,16 @@ public function close() {
5959
* Get a value with a given key. Only string values are supported.
6060
*
6161
* @param string $key
62-
*
63-
* @return bool|string The value associated with $key, or false if $key is not known.
62+
* @return string|false The value associated with $key, or false if $key is not known.
6463
*/
6564
public function get( $key ) {
66-
return isset( $this->data[ $key ] ) ? $this->data[ $key ] : false;
65+
return $this->data[ $key ] ?? false;
6766
}
6867

6968
/**
7069
* Check whether key exists
7170
*
7271
* @param string $key
73-
*
7472
* @return bool
7573
*/
7674
public function exists( $key ) {
@@ -80,7 +78,7 @@ public function exists( $key ) {
8078
/**
8179
* Fetch first key
8280
*
83-
* @return string
81+
* @return string|false
8482
*/
8583
public function firstkey() {
8684
$this->keys = array_keys( $this->data );
@@ -90,14 +88,14 @@ public function firstkey() {
9088
/**
9189
* Fetch next key
9290
*
93-
* @return string
91+
* @return string|false
9492
*/
9593
public function nextkey() {
9694
if ( $this->keys === null ) {
9795
return $this->firstkey();
9896
}
9997

100-
return empty( $this->keys ) ? false : array_shift( $this->keys );
98+
return $this->keys ? array_shift( $this->keys ) : false;
10199
}
102100

103101
}

‎src/Reader/PHP.php‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,11 @@ class PHP extends Reader {
3535
*/
3636
protected $fileName;
3737

38+
/**
39+
* The file handle
40+
*/
41+
protected $handle;
42+
3843
/**
3944
* @var string $index
4045
* First 2048 bytes of CDB file, containing pointers to hash table.
@@ -104,10 +109,10 @@ public function __construct( $fileName ) {
104109
* Close the handle on the CDB file.
105110
*/
106111
public function close() {
107-
if ( isset( $this->handle ) ) {
112+
if ( $this->handle ) {
108113
fclose( $this->handle );
109114
}
110-
unset( $this->handle );
115+
$this->handle = null;
111116
}
112117

113118
/**

‎src/Writer.php‎

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -25,11 +25,6 @@
2525
* @see http://cr.yp.to/cdb.html
2626
*/
2727
abstract class Writer {
28-
/**
29-
* The file handle
30-
*/
31-
protected $handle;
32-
3328
/**
3429
* File we'll be writing to when we're done
3530
* @var string $realFileName
@@ -79,9 +74,7 @@ abstract public function close();
7974
* If the object goes out of scope, close it
8075
*/
8176
public function __destruct() {
82-
if ( isset( $this->handle ) ) {
83-
$this->close();
84-
}
77+
$this->close();
8578
}
8679

8780
/**

‎src/Writer/DBA.php‎

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -25,9 +25,13 @@
2525
* Writer class which uses the DBA extension (php-dba)
2626
*/
2727
class DBA extends Writer {
28+
/**
29+
* The file handle
30+
*/
31+
protected $handle;
32+
2833
/**
2934
* @param string $fileName
30-
* @throws Exception
3135
*/
3236
public function __construct( $fileName ) {
3337
$this->realFileName = $fileName;
@@ -42,19 +46,16 @@ public function set( $key, $value ) {
4246
return dba_insert( $key, $value, $this->handle );
4347
}
4448

45-
/**
46-
* @throws Exception
47-
*/
4849
public function close() {
49-
if ( isset( $this->handle ) ) {
50+
if ( $this->handle ) {
5051
dba_close( $this->handle );
52+
if ( $this->isWindows() ) {
53+
unlink( $this->realFileName );
54+
}
55+
if ( !rename( $this->tmpFileName, $this->realFileName ) ) {
56+
throw new Exception( 'Unable to move the new CDB file into place.' );
57+
}
5158
}
52-
if ( $this->isWindows() ) {
53-
unlink( $this->realFileName );
54-
}
55-
if ( !rename( $this->tmpFileName, $this->realFileName ) ) {
56-
throw new Exception( 'Unable to move the new CDB file into place.' );
57-
}
58-
unset( $this->handle );
59+
$this->handle = null;
5960
}
6061
}

‎src/Writer/PHP.php‎

Lines changed: 17 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,11 @@
2929
* appears in PHP 5.3.
3030
*/
3131
class PHP extends Writer {
32+
/**
33+
* The file handle
34+
*/
35+
protected $handle;
36+
3237
protected $hplist;
3338

3439
protected $numentries;
@@ -69,25 +74,22 @@ public function set( $key, $value ) {
6974
$this->addend( strlen( $key ), strlen( $value ), Util::hash( $key ) );
7075
}
7176

72-
/**
73-
* @throws Exception
74-
*/
7577
public function close() {
76-
$this->finish();
77-
if ( isset( $this->handle ) ) {
78+
if ( $this->handle ) {
79+
$this->finish();
7880
fclose( $this->handle );
81+
82+
if ( $this->isWindows() && file_exists( $this->realFileName ) ) {
83+
unlink( $this->realFileName );
84+
}
85+
if ( !rename( $this->tmpFileName, $this->realFileName ) ) {
86+
$this->throwException( 'Unable to move the new CDB file into place.' );
87+
}
7988
}
80-
if ( $this->isWindows() && file_exists( $this->realFileName ) ) {
81-
unlink( $this->realFileName );
82-
}
83-
if ( !rename( $this->tmpFileName, $this->realFileName ) ) {
84-
$this->throwException( 'Unable to move the new CDB file into place.' );
85-
}
86-
unset( $this->handle );
89+
$this->handle = null;
8790
}
8891

8992
/**
90-
* @throws Exception
9193
* @param string $buf
9294
*/
9395
protected function write( $buf ) {
@@ -98,7 +100,6 @@ protected function write( $buf ) {
98100
}
99101

100102
/**
101-
* @throws Exception
102103
* @param int $len
103104
*/
104105
protected function posplus( $len ) {
@@ -128,7 +129,6 @@ protected function addend( $keylen, $datalen, $h ) {
128129
}
129130

130131
/**
131-
* @throws Exception
132132
* @param int $keylen
133133
* @param int $datalen
134134
*/
@@ -143,9 +143,6 @@ protected function addbegin( $keylen, $datalen ) {
143143
$this->write( $buf );
144144
}
145145

146-
/**
147-
* @throws Exception
148-
*/
149146
protected function finish() {
150147
// Hack for DBA cross-check
151148
$this->hplist = array_reverse( $this->hplist );
@@ -194,6 +191,7 @@ protected function finish() {
194191
// Fill the hashtable, using the next empty slot if the hashed slot
195192
// is taken.
196193
for ( $u = 0; $u < $count; ++$u ) {
194+
// @phan-suppress-next-line PhanTypePossiblyInvalidDimOffset
197195
$hp = $packedTables[$starts[$i] + $u];
198196
$where = Util::unsignedMod(
199197
Util::unsignedShiftRight( $hp['h'], 8 ), $len );
@@ -228,6 +226,7 @@ protected function finish() {
228226
* Clean up the temp file and throw an exception
229227
*
230228
* @param string $msg
229+
* @return never
231230
* @throws Exception
232231
*/
233232
protected function throwException( $msg ) {

0 commit comments

Comments
 (0)