Skip to content

Commit

Permalink
autoloader: Fix phan issues (#37608)
Browse files Browse the repository at this point in the history
No real functionality changes.

* `$io` parameter to `AutoloadGenerator` constructor is no longer
  optional. The class didn't work when omitted anyway.
* Improve some phpdocs.
* `PHP_Autoloader::load_class()` now always returns bool as documented.
* Use `??` where applicable.
* Replace `->withConsecutive()` in tests, PHPUnit deprecated it without
  any replacement besides rolling our own.
* Fix some test logic.
  • Loading branch information
anomiex authored May 28, 2024
1 parent f8d5a25 commit 8227b16
Show file tree
Hide file tree
Showing 16 changed files with 172 additions and 84 deletions.
30 changes: 1 addition & 29 deletions projects/packages/autoloader/.phan/baseline.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,37 +8,9 @@
* (can be combined with --load-baseline)
*/
return [
// # Issue statistics:
// PhanDeprecatedFunction : 10+ occurrences
// PhanTypeMismatchArgumentProbablyReal : 6 occurrences
// PhanTypeMismatchArgument : 4 occurrences
// PhanTypeMismatchReturnProbablyReal : 3 occurrences
// PhanTypePossiblyInvalidDimOffset : 3 occurrences
// PhanPluginDuplicateConditionalNullCoalescing : 2 occurrences
// PhanPossiblyNullTypeMismatchProperty : 1 occurrence
// PhanTypeMismatchArgumentInternal : 1 occurrence
// PhanTypeMismatchArgumentNullable : 1 occurrence
// PhanTypeMismatchArgumentNullableInternal : 1 occurrence
// PhanTypeMismatchDeclaredParamNullable : 1 occurrence
// PhanTypeMismatchForeach : 1 occurrence

// This baseline has no suppressions
// Currently, file_suppressions and directory_suppressions are the only supported suppressions
'file_suppressions' => [
'src/AutoloadFileWriter.php' => ['PhanPluginDuplicateConditionalNullCoalescing'],
'src/AutoloadGenerator.php' => ['PhanPossiblyNullTypeMismatchProperty', 'PhanTypeMismatchDeclaredParamNullable'],
'src/AutoloadProcessor.php' => ['PhanTypeMismatchForeach', 'PhanTypeMismatchReturnProbablyReal'],
'src/class-php-autoloader.php' => ['PhanTypeMismatchReturnProbablyReal'],
'src/class-version-loader.php' => ['PhanPluginDuplicateConditionalNullCoalescing'],
'tests/php/bootstrap.php' => ['PhanTypeMismatchArgumentInternal'],
'tests/php/lib/class-acceptance-test-case.php' => ['PhanTypePossiblyInvalidDimOffset'],
'tests/php/lib/class-test-plugin-factory.php' => ['PhanTypeMismatchArgument', 'PhanTypeMismatchArgumentNullable', 'PhanTypeMismatchArgumentNullableInternal'],
'tests/php/tests/acceptance/AutoloaderTest.php' => ['PhanTypeMismatchArgument'],
'tests/php/tests/acceptance/CacheTest.php' => ['PhanTypeMismatchArgument'],
'tests/php/tests/unit/AutoloadProcessorTest.php' => ['PhanTypeMismatchArgumentProbablyReal'],
'tests/php/tests/unit/AutoloaderHandlerTest.php' => ['PhanDeprecatedFunction'],
'tests/php/tests/unit/PHPAutoloaderTest.php' => ['PhanDeprecatedFunction'],
'tests/php/tests/unit/PluginLocatorTest.php' => ['PhanDeprecatedFunction'],
'tests/php/tests/unit/PluginsHandlerTest.php' => ['PhanDeprecatedFunction'],
],
// 'directory_suppressions' => ['src/directory_name' => ['PhanIssueName1', 'PhanIssueName2']] can be manually added if needed.
// (directory_suppressions will currently be ignored by subsequent calls to --save-baseline, but may be preserved in future Phan releases)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: fixed

`AutoloadGenerator::__construct` no longer pretends `$io` is nullable. That never worked.
2 changes: 1 addition & 1 deletion projects/packages/autoloader/src/AutoloadFileWriter.php
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ public static function copyAutoloaderFiles( $io, $outDir, $suffix ) {
continue;
}

$newFile = isset( $renameList[ $file ] ) ? $renameList[ $file ] : $file;
$newFile = $renameList[ $file ] ?? $file;
$content = self::prepareAutoloaderFile( $file, $suffix );

$written = file_put_contents( $outDir . '/' . $newFile, $content );
Expand Down
4 changes: 2 additions & 2 deletions projects/packages/autoloader/src/AutoloadGenerator.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
*/
class AutoloadGenerator {

const VERSION = '3.0.7';
const VERSION = '3.0.8-alpha';

/**
* IO object.
Expand All @@ -42,7 +42,7 @@ class AutoloadGenerator {
*
* @param IOInterface $io IO object.
*/
public function __construct( IOInterface $io = null ) {
public function __construct( IOInterface $io ) {
$this->io = $io;
$this->filesystem = new Filesystem();
}
Expand Down
5 changes: 3 additions & 2 deletions projects/packages/autoloader/src/AutoloadProcessor.php
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ public function __construct( $classmapScanner, $pathCodeTransformer ) {
* @param array $autoloads The autoloads we are processing.
* @param bool $scanPsrPackages Whether or not PSR packages should be converted to a classmap.
*
* @return array $processed
* @return array|null $processed
* @phan-param array{classmap:?array{path:string,version:string}[],psr-4:?array<string,array{path:string,version:string}[]>,psr-0:?array<string,array{path:string,version:string}[]>} $autoloads
*/
public function processClassmap( $autoloads, $scanPsrPackages ) {
// We can't scan PSR packages if we don't actually have any.
Expand Down Expand Up @@ -123,7 +124,7 @@ public function processClassmap( $autoloads, $scanPsrPackages ) {
* @param array $autoloads The autoloads we are processing.
* @param bool $scanPsrPackages Whether or not PSR packages should be converted to a classmap.
*
* @return array $processed
* @return array|null $processed
*/
public function processPsr4Packages( $autoloads, $scanPsrPackages ) {
if ( $scanPsrPackages || empty( $autoloads['psr-4'] ) ) {
Expand Down
2 changes: 1 addition & 1 deletion projects/packages/autoloader/src/class-php-autoloader.php
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public function unregister_autoloader() {
public static function load_class( $class_name ) {
global $jetpack_autoloader_loader;
if ( ! isset( $jetpack_autoloader_loader ) ) {
return;
return false;
}

$file = $jetpack_autoloader_loader->find_class_file( $class_name );
Expand Down
2 changes: 1 addition & 1 deletion projects/packages/autoloader/src/class-version-loader.php
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public function __construct( $version_selector, $classmap, $psr4_map, $filemap )
*/
public function find_class_file( $class_name ) {
$data = $this->select_newest_file(
isset( $this->classmap[ $class_name ] ) ? $this->classmap[ $class_name ] : null,
$this->classmap[ $class_name ] ?? null,
$this->find_psr4_file( $class_name )
);
if ( ! isset( $data ) ) {
Expand Down
6 changes: 3 additions & 3 deletions projects/packages/autoloader/tests/php/bootstrap.php
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@

// Load all of the test dependencies.
require_once TEST_PACKAGE_DIR . '/vendor/autoload.php';
require_once __DIR__ . '/lib/with-consecutive.php';
require_once __DIR__ . '/lib/functions-wordpress.php';
require_once __DIR__ . '/lib/functions.php';
require_once __DIR__ . '/lib/class-test-plugin-factory.php';
Expand All @@ -49,7 +50,7 @@ function ( $class ) {
// We're only going to autoload the test autoloader files.
$check = substr( $class, 0, strlen( $namespace ) );
if ( $namespace !== $check ) {
return false;
return;
}

// Remove the namespace.
Expand All @@ -66,11 +67,10 @@ function ( $class ) {
)
);
if ( ! is_file( $path ) ) {
return false;
return;
}

require_once $path;
return true;
}
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ protected function install_autoloaders( $version_or_versions ) {
* Installs a symlink to a plugin version.
*
* @param string $version The version of the autoloader we want to symlink to.
* @param string $is_mu_plugin Whether or not the symlink should be an mu-plugin.
* @param bool $is_mu_plugin Whether or not the symlink should be an mu-plugin.
* @param string $symlink_key The key for the symlink in the installed plugin list.
*/
protected function install_autoloader_symlink( $version, $is_mu_plugin, $symlink_key ) {
Expand Down Expand Up @@ -143,7 +143,7 @@ protected function load_plugin_autoloader( $version ) {
// This isn't perfect (it won't catch successive resets from a new autoloader discovering newer autoloaders) but
// it will at least catch the most common reset scenarios that we can build assertions on.
global $jetpack_packages_classmap;
$reset_count = isset( $jetpack_packages_classmap ) ? $jetpack_packages_classmap['reset_count'] : null;
$reset_count = $jetpack_packages_classmap['reset_count'] ?? null;

require_once $plugin_dir . '/vendor/autoload_packages.php';

Expand All @@ -154,7 +154,7 @@ protected function load_plugin_autoloader( $version ) {

// Since we can assume after every load we set the count we know a null value
// means this was the first time the autoloader was executed.
if ( ! isset( $reset_count ) ) {
if ( $reset_count === null ) {
$jetpack_packages_classmap['reset_count'] = 0;
} else {
$jetpack_packages_classmap['reset_count'] = $reset_count + 1;
Expand Down Expand Up @@ -297,7 +297,7 @@ protected function cache_plugin( $version_or_versions ) {
*/
protected function assertAutoloaderResetCount( $count ) {
global $jetpack_packages_classmap;
$reset_count = isset( $jetpack_packages_classmap ) ? $jetpack_packages_classmap['reset_count'] : 0;
$reset_count = $jetpack_packages_classmap['reset_count'] ?? 0;
$this->assertEquals( $count, $reset_count, 'The number of autoloader resets did not match what was expected.' );
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,9 @@ private function __construct( $is_mu_plugin, $slug, $autoloads ) {
/**
* Creates a new factory for the plugin and returns it.
*
* @param bool $is_mu_plugin Indicates whether or not the plugin is an mu-plugin.
* @param string $slug The slug of the plugin we're building.
* @param string[] $autoloads The composer autoloads for the plugin we're building.
* @param bool $is_mu_plugin Indicates whether or not the plugin is an mu-plugin.
* @param string $slug The slug of the plugin we're building.
* @param array $autoloads The composer autoloads for the plugin we're building.
* @return Test_Plugin_Factory
* @throws \InvalidArgumentException When the slug is invalid.
*/
Expand Down Expand Up @@ -177,7 +177,7 @@ public function with_class( $autoload_type, $fqn, $content ) {
$namespace = substr( $fqn, 0, -strlen( $class_name ) - 1 );
} else {
$class_name = $fqn;
$namespace = null;
$namespace = '';
}

$path = null;
Expand Down Expand Up @@ -209,7 +209,7 @@ public function with_class( $autoload_type, $fqn, $content ) {
break;
}

if ( ! isset( $path ) ) {
if ( $path === null ) {
throw new \InvalidArgumentException( 'The namespace for this class is not in the factory\'s autoloads.' );
}

Expand All @@ -222,12 +222,12 @@ public function with_class( $autoload_type, $fqn, $content ) {
}

$file_content = "<?php\n\n";
if ( isset( $namespace ) ) {
if ( $namespace !== '' ) {
$file_content .= "namespace $namespace;\n\n";
}
$file_content .= "class $class_name {\n$content\n}";

return $this->with_file( $path, $file_content );
return $this->with_file( (string) $path, $file_content );
}

/**
Expand All @@ -245,7 +245,7 @@ public function with_autoloader_version( $version ) {
/**
* Adds options that will be passed to the plugin's composer.json file.
*
* @param string[] $options The options that we want to set in the composer config.
* @param array $options The options that we want to set in the composer config.
* @return $this
*/
public function with_composer_config( $options ) {
Expand Down
73 changes: 73 additions & 0 deletions projects/packages/autoloader/tests/php/lib/with-consecutive.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
<?php
/**
* A function to replace PHPUnit's `->withConsecutive()`.
*
* @package automattic/jetpack-autoloader
*/

use PHPUnit\Framework\Constraint\Callback;
use PHPUnit\Framework\Constraint\Constraint;
use PHPUnit\Framework\Constraint\IsEqual;

/**
* Reimplement `withConsecutive` for PHPUnit.
*
* Unfortunately PHPUnit deprecated withConsecutive with no replacement, so we
* have to roll our own version.
*
* @see https://github.com/sebastianbergmann/phpunit/issues/4026
*
* Where previously you'd have done like
* ```
* ->withConsecutive(
* [ first, call, args ],
* [ second, call, args ],
* [ and, so, on ]
* )
* ```
* you can do like this now
* ```
* ->with( ...with_consecutive(
* [ first, call, args ],
* [ second, call, args ],
* [ and, so, on ]
* ) )
* ```
*
* @param array ...$args Sets of arguments as you'd have passed to `->withConsecutive()`.
* @return Constraint[] Array of constraints to pass to `->with()`.
* @throws InvalidArgumentException If arguments are invalid.
*/
function with_consecutive( ...$args ) {
if ( ! $args ) {
throw new InvalidArgumentException( 'Must pass at least one set of arguments' );
}

$ct = count( $args[0] );
$value_sets = array();
foreach ( $args as $group ) {
if ( count( $group ) !== $ct ) {
throw new InvalidArgumentException( 'All sets of arguments must be the same length' );
}
for ( $i = 0; $i < $ct; $i++ ) {
$value_sets[ $i ][] = $group[ $i ] instanceof Constraint ? $group[ $i ] : new IsEqual( $group[ $i ] );
}
}
$funcs = array();
for ( $i = 0; $i < $ct; $i++ ) {
$funcs[] = new Callback(
function ( $value ) use ( $value_sets, $i ) {
static $set = null;
$set = $set ?? $value_sets[ $i ]; // @phan-suppress-current-line PhanTypePossiblyInvalidDimOffset -- False positive.
if ( ! $set ) {
$n = count( $value_sets[ $i ] ); // @phan-suppress-current-line PhanTypePossiblyInvalidDimOffset -- False positive.
throw new InvalidArgumentException( "More calls than argument sets. Use `->expects( \$this->exactly( $n ) )` or the like when mocking the method to avoid this." );
}
$expect = array_shift( $set );
$expect->evaluate( $value );
return true;
}
);
}
return $funcs;
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,23 @@
*/
class AutoloadProcessorTest extends TestCase {

/**
* Callback that should not be called.
*
* @return never
* @throws BadFunctionCallException Always.
*/
public static function never_call_callback() {
throw new BadFunctionCallException( 'Callback should not have been called' );
}

/**
* Tests that all of the process functions are safe when not given an autoload they're expecting.
*
* @phan-suppress PhanTypeMismatchArgument -- Intentionally checking error cases.
*/
public function test_process_functions_return_null_when_empty() {
$processor = new AutoloadProcessor( null, null );
$processor = new AutoloadProcessor( array( static::class, 'never_call_callback' ), array( static::class, 'never_call_callback' ) );

$this->assertNull( $processor->processClassmap( array(), false ) );
$this->assertNull( $processor->processClassmap( array(), true ) );
Expand Down Expand Up @@ -52,6 +64,8 @@ public function test_process_classmap_does_not_scan_psr_packages() {
'version' => 'dev-test',
),
),
'psr-4' => null,
'psr-0' => null,
);

$processed = $processor->processClassmap( $autoloads, false );
Expand Down Expand Up @@ -91,14 +105,16 @@ public function test_process_classmap_scans_psr_packages_when_requested() {
$processor = new AutoloadProcessor( $classmap_scanner, $path_code_transformer );

$autoloads = array(
'psr-4' => array(
'psr-4' => array(
'Jetpack\\Autoloader\\' => array(
array(
'path' => 'src',
'version' => 'dev-test2',
),
),
),
'classmap' => null,
'psr-0' => null,
);

$processed = $processor->processClassmap( $autoloads, true );
Expand Down Expand Up @@ -144,6 +160,8 @@ public function test_process_classmap_uses_blacklist() {
'version' => 'dev-test',
),
),
'psr-4' => null,
'psr-0' => null,
);

$autoloads['exclude-from-classmap'] = array( 'TestClass' );
Expand Down Expand Up @@ -172,7 +190,7 @@ public function test_process_psr_packages() {
$path_code_transformer = function ( $path ) {
return 'converted-' . $path;
};
$processor = new AutoloadProcessor( null, $path_code_transformer );
$processor = new AutoloadProcessor( array( static::class, 'never_call_callback' ), $path_code_transformer );

$autoloads = array(
'psr-4' => array(
Expand Down Expand Up @@ -202,7 +220,7 @@ public function test_process_psr_packages() {
* Tests that `processPsr4Packages` does not when we're indicating that we want to make them a classmap.
*/
public function test_process_psr_packages_does_nothing_when_converting_to_classmap() {
$processor = new AutoloadProcessor( null, null );
$processor = new AutoloadProcessor( array( static::class, 'never_call_callback' ), array( static::class, 'never_call_callback' ) );

$autoloads = array(
'psr-4' => array(
Expand All @@ -227,7 +245,7 @@ public function test_process_files() {
$path_code_transformer = function ( $path ) {
return 'converted-' . $path;
};
$processor = new AutoloadProcessor( null, $path_code_transformer );
$processor = new AutoloadProcessor( array( static::class, 'never_call_callback' ), $path_code_transformer );

$autoloads = array(
'files' => array(
Expand Down
Loading

0 comments on commit 8227b16

Please sign in to comment.