tonyair
/
silverstripe-framework
mirror of https://github.com/silverstripe/silverstripe-framework
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

Anwser Peer review feedback

This commit is contained in:
Maxime Rainville 2021-11-18 23:16:03 +13:00
parent cbc4593ab4
commit 7c3fddfc8a
19 changed files with 217 additions and 65 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
src/Core/CoreKernel.php
View File

@ -516,7 +516,7 @@ class CoreKernel implements Kernel
*
* @return string[] List of CI types to ignore as defined by `Module`.
*/
protected function getIgnoreCILibraries(): array
protected function getIgnoreCIConfigs(): array
{
return [];
}
@ -532,14 +532,14 @@ class CoreKernel implements Kernel
$this->getClassLoader()->init(
$this->getIncludeTests(),
$flush,
$this->getIgnoreCILibraries()
$this->getIgnoreCIConfigs()
);
// Find modules
$this->getModuleLoader()->init(
$this->getIncludeTests(),
$flush,
$this->getIgnoreCILibraries()
$this->getIgnoreCIConfigs()
);
// Flush config
@ -561,7 +561,7 @@ class CoreKernel implements Kernel
$defaultSet->init(
$this->getIncludeTests(),
$flush,
$this->getIgnoreCILibraries()
$this->getIgnoreCIConfigs()
);
}
}

6
src/Core/Manifest/ClassLoader.php
View File

@ -120,14 +120,14 @@ class ClassLoader
*
* @param bool $includeTests
* @param bool $forceRegen
* @param string[] $ignoredCILibs
* @param string[] $ignoredCIConfigs
*/
public function init($includeTests = false, $forceRegen = false, array $ignoredCILibs = [])
public function init($includeTests = false, $forceRegen = false, array $ignoredCIConfigs = [])
{
foreach ($this->manifests as $manifest) {
/** @var ClassManifest $instance */
$instance = $manifest['instance'];
$instance->init($includeTests, $forceRegen, $ignoredCILibs);
$instance->init($includeTests, $forceRegen, $ignoredCIConfigs);
}
$this->registerAutoloader();

12
src/Core/Manifest/ClassManifest.php
View File

@ -260,9 +260,9 @@ class ClassManifest
*
* @param bool $includeTests
* @param bool $forceRegen
* @param string[] $ignoredCILibs
* @param string[] $ignoredCIConfigs
*/
public function init($includeTests = false, $forceRegen = false, array $ignoredCILibs = [])
public function init($includeTests = false, $forceRegen = false, array $ignoredCIConfigs = [])
{
$this->cache = $this->buildCache($includeTests);
@ -276,7 +276,7 @@ class ClassManifest
}
// Build
$this->regenerate($includeTests, $ignoredCILibs);
$this->regenerate($includeTests, $ignoredCIConfigs);
}
/**
@ -501,9 +501,9 @@ class ClassManifest
* Completely regenerates the manifest file.
*
* @param bool $includeTests
* @param string[] $ignoredCILibs
* @param string[] $ignoredCIConfigs
*/
public function regenerate($includeTests, array $ignoredCILibs = [])
public function regenerate($includeTests, array $ignoredCIConfigs = [])
{
// Reset the manifest so stale info doesn't cause errors.
$this->loadState([]);
@ -515,7 +515,7 @@ class ClassManifest
'name_regex' => '/^[^_].*\\.php$/',
'ignore_files' => ['index.php', 'cli-script.php'],
'ignore_tests' => !$includeTests,
'ignore_ci_libraries' => $ignoredCILibs,
'ignore_ci_configs' => $ignoredCIConfigs,
'file_callback' => function ($basename, $pathname, $depth) use ($includeTests, $finder) {
$this->handleFile($basename, $pathname, $includeTests);
},

35
src/Core/Manifest/ManifestFileFinder.php
View File

@ -2,6 +2,7 @@
namespace SilverStripe\Core\Manifest;
use RuntimeException;
use SilverStripe\Assets\FileFinder;
/**
@ -33,7 +34,7 @@ class ManifestFileFinder extends FileFinder
'ignore_tests' => true,
'min_depth' => 1,
'ignore_dirs' => ['node_modules'],
'ignore_ci_libraries' => []
'ignore_ci_configs' => []
];
public function acceptDir($basename, $pathname, $depth)
@ -74,10 +75,10 @@ class ManifestFileFinder extends FileFinder
return false;
}
// Skip if test dir inside vendor module with unexpected CI library
if ($depth > 3 && $basename === self::TESTS_DIR && $ignoreCILib = $this->getOption('ignore_ci_libraries')) {
$ciLib = $this->findModuleCILib($basename, $pathname, $depth);
if (in_array($ciLib, $ignoreCILib)) {
// Skip if test dir inside vendor module with unexpected CI Configuration
if ($depth > 3 && $basename === self::TESTS_DIR && $ignoreCIConfig = $this->getOption('ignore_ci_configs')) {
$ciLib = $this->findModuleCIPhpConfiguration($basename, $pathname, $depth);
if (in_array($ciLib, $ignoreCIConfig)) {
return false;
}
}
@ -261,21 +262,39 @@ class ManifestFileFinder extends FileFinder
return false;
}
private function findModuleCILib(string $basename, string $pathname, int $depth): string
/**
* Find out the root of the current module and read the PHP CI configuration from tho composer file
*
* @param string $basename Name of the current folder
* @param string $pathname Full path the parent folder
* @param string $depth Depth of the current folder
*/
private function findModuleCIPhpConfiguration(string $basename, string $pathname, int $depth): string
{
if ($depth < 1) {
// We went all the way back to the root of the project
return Module::CI_UNKNOWN;
}
// We pop the current folder and use the next entry the pathname
$newBasename = basename($pathname);
$newPathname = dirname($pathname);
$newDepth = $depth - 1;
if ($this->isDirectoryModule($newBasename, $newPathname, $newDepth)) {
// We've reached the root of the module folder, we can read the PHP CI config now
$module = new Module($newPathname, $this->upLevels($newPathname, $newDepth));
return $module->getCILibrary();
$config = $module->getCIConfig();
if (empty($config['PHP'])) {
// This should never happen
throw new RuntimeException('Module::getCIConfig() did not return a PHP CI value');
}
return $config['PHP'];
}
return $this->findModuleCILib($newBasename, $newPathname, $newDepth);
// We haven't reach our module root yet ... let's look up one more level
return $this->findModuleCIPhpConfiguration($newBasename, $newPathname, $newDepth);
}
}

72
src/Core/Manifest/Module.php
View File

@ -5,6 +5,7 @@ namespace SilverStripe\Core\Manifest;
use Composer\Semver\Semver;
use Exception;
use InvalidArgumentException;
use RuntimeException;
use Serializable;
use SilverStripe\Core\Path;
use SilverStripe\Dev\Deprecation;
@ -21,19 +22,19 @@ class Module implements Serializable
const TRIM_CHARS = ' /\\';
/**
* Return value of getCILibrary() when module uses PHPUNit 9
* Return value of getCIConfig() when module uses PHPUNit 9
*/
const CI_PHPUNIT_NINE = 'PHPUnit9';
const CI_PHPUNIT_NINE = 'CI_PHPUNIT_NINE';
/**
* Return value of getCILibrary() when module uses PHPUNit 5
* Return value of getCIConfig() when module uses PHPUNit 5
*/
const CI_PHPUNIT_FIVE = 'PHPUnit5';
const CI_PHPUNIT_FIVE = 'CI_PHPUNIT_FIVE';
/**
* Return value of getCILibrary() when module does not use any CI
* Return value of getCIConfig() when module does not use any CI
*/
const CI_UNKNOWN = 'NoPHPUnit';
const CI_UNKNOWN = 'CI_UNKNOWN';
@ -295,10 +296,22 @@ class Module implements Serializable
}
/**
* Determine what CI library the module is using.
* Determine what configurations the module is using to run various aspects of its CI. THe only aspect
* that is observed is `PHP`
* @return array List of configuration aspects e.g.: `['PHP' => 'CI_PHPUNIT_NINE']`
* @internal
*/
public function getCILibrary(): string
public function getCIConfig(): array
{
return [
'PHP' => $this->getPhpCiConfig()
];
}
/**
* Determine what CI Configuration the module uses to test its PHP code.
*/
private function getPhpCiConfig(): string
{
// We don't have any composer data at all
if (empty($this->composerData)) {
@ -316,10 +329,18 @@ class Module implements Serializable
// Try to pick which CI we are using based on phpunit constraint
$phpUnitConstraint = $this->requireDevConstraint(['sminnee/phpunit', 'phpunit/phpunit']);
if ($phpUnitConstraint) {
if ($this->satisfiesAtLeastOne(['5.7.0', '5.0.0', '5.x-dev', '5.7.x-dev'], $phpUnitConstraint)) {
if ($this->constraintSatisfies(
$phpUnitConstraint,
['5.7.0', '5.0.0', '5.x-dev', '5.7.x-dev'],
5
)) {
return self::CI_PHPUNIT_FIVE;
}
if ($this->satisfiesAtLeastOne(['9.0.0', '9.5.0', '9.x-dev', '9.5.x-dev'], $phpUnitConstraint)) {
if ($this->constraintSatisfies(
$phpUnitConstraint,
['9.0.0', '9.5.0', '9.x-dev', '9.5.x-dev'],
9
)) {
return self::CI_PHPUNIT_NINE;
}
}
@ -327,10 +348,18 @@ class Module implements Serializable
// Try to pick which CI we are using based on recipe-testing constraint
$recipeTestingConstraint = $this->requireDevConstraint(['silverstripe/recipe-testing']);
if ($recipeTestingConstraint) {
if ($this->satisfiesAtLeastOne(['1.0.0', '1.1.0', '1.2.0', '1.1.x-dev', '1.2.x-dev', '1.x-dev'], $recipeTestingConstraint)) {
if ($this->constraintSatisfies(
$recipeTestingConstraint,
['1.0.0', '1.1.0', '1.2.0', '1.1.x-dev', '1.2.x-dev', '1.x-dev'],
1
)) {
return self::CI_PHPUNIT_FIVE;
}
if ($this->satisfiesAtLeastOne(['2.0.0', '2.0.x-dev', '2.x-dev'], $recipeTestingConstraint)) {
if ($this->constraintSatisfies(
$recipeTestingConstraint,
['2.0.0', '2.0.x-dev', '2.x-dev'],
2
)) {
return self::CI_PHPUNIT_NINE;
}
}
@ -362,9 +391,22 @@ class Module implements Serializable
/**
* Determines if the provided constraint allows at least one of the version provided
*/
private function satisfiesAtLeastOne(array $versions, string $constraint): bool
{
return !empty(Semver::satisfiedBy($versions, $constraint));
private function constraintSatisfies(
string $constraint,
array $possibleVersions,
int $majorVersionFallback
): bool {
// Let's see of any of our possible versions is allowed by the constraint
if (!empty(Semver::satisfiedBy($possibleVersions, $constraint))) {
return true;
}
// Let's see if we are using an exact version constraint. e.g. ~1.2.3 or 1.2.3 or ~1.2 or 1.2.*
if (preg_match("/^~?$majorVersionFallback(\.(\d+)|\*){0,2}/", $constraint)) {
return true;
}
return false;
}
}

6
src/Core/Manifest/ModuleLoader.php
View File

@ -91,12 +91,12 @@ class ModuleLoader
*
* @param bool $includeTests
* @param bool $forceRegen
* @param string[] $ignoredCILibs
* @param string[] $ignoredCIConfigs
*/
public function init($includeTests = false, $forceRegen = false, array $ignoredCILibs = [])
public function init($includeTests = false, $forceRegen = false, array $ignoredCIConfigs = [])
{
foreach ($this->manifests as $manifest) {
$manifest->init($includeTests, $forceRegen, $ignoredCILibs);
$manifest->init($includeTests, $forceRegen, $ignoredCIConfigs);
}
}
}

12
src/Core/Manifest/ModuleManifest.php
View File

@ -121,9 +121,9 @@ class ModuleManifest
/**
* @param bool $includeTests
* @param bool $forceRegen Force the manifest to be regenerated.
* @param string[] $ignoredCILibs
* @param string[] $ignoredCIConfigs
*/
public function init($includeTests = false, $forceRegen = false, array $ignoredCILibs = [])
public function init($includeTests = false, $forceRegen = false, array $ignoredCIConfigs = [])
{
// build cache from factory
if ($this->cacheFactory) {
@ -138,7 +138,7 @@ class ModuleManifest
$this->modules = $this->cache->get($this->cacheKey) ?: [];
}
if (empty($this->modules)) {
$this->regenerate($includeTests, $ignoredCILibs);
$this->regenerate($includeTests, $ignoredCIConfigs);
}
}
@ -163,9 +163,9 @@ class ModuleManifest
* Does _not_ build the actual variant
*
* @param bool $includeTests
* @param string[] $ignoredCILibs
* @param string[] $ignoredCIConfigs
*/
public function regenerate($includeTests = false, array $ignoredCILibs = [])
public function regenerate($includeTests = false, array $ignoredCIConfigs = [])
{
$this->modules = [];
@ -173,7 +173,7 @@ class ModuleManifest
$finder->setOptions([
'min_depth' => 0,
'ignore_tests' => !$includeTests,
'ignore_ci_libraries' => $ignoredCILibs,
'ignore_ci_configs' => $ignoredCIConfigs,
'dir_callback' => function ($basename, $pathname, $depth) use ($finder) {
if ($finder->isDirectoryModule($basename, $pathname, $depth)) {
$this->addModule($pathname);

2
src/Dev/SapphireTest.php
View File

@ -1012,7 +1012,7 @@ if (class_exists(IsEqualCanonicalizing::class)) {
$kernel = new TestKernel(BASE_PATH);
// PHPUnit 9 only logic to exclude old test still targeting PHPUNit 5.7
$kernel->setIgnoreCILibraries([Module::CI_PHPUNIT_FIVE, Module::CI_UNKNOWN]);
$kernel->setIgnoreCIConfigs([Module::CI_PHPUNIT_FIVE, Module::CI_UNKNOWN]);
if (class_exists(HTTPApplication::class)) {
// Mock request

11
src/Dev/TestKernel.php
View File

@ -10,8 +10,8 @@ use SilverStripe\Core\CoreKernel;
class TestKernel extends CoreKernel
{
/** @var string[] $ciLibs */
private $ciLibs = [];
/** @var string[] $ciConfigs */
private $ciConfigs = [];
public function __construct($basePath)
@ -48,15 +48,16 @@ class TestKernel extends CoreKernel
/**
* @param string[] $ciLibs
* Set a list of CI configurations that should cause a module's test not to be added to a manifest
* @param string[] $ciConfigs
*/
public function setIgnoreCILibraries(array $ciLibs): self
public function setIgnoreCIConfigs(array $ciLibs): self
{
$this->ciLibs = $ciLibs;
return $this;
}
protected function getIgnoreCILibraries(): array
protected function getIgnoreCIConfigs(): array
{
return $this->ciLibs;
}

12
src/View/ThemeManifest.php
View File

@ -73,9 +73,9 @@ class ThemeManifest implements ThemeList
/**
* @param bool $includeTests Include tests in the manifest
* @param bool $forceRegen Force the manifest to be regenerated.
* @param string[] $ignoredCILibs
* @param string[] $ignoredCIConfigs
*/
public function init($includeTests = false, $forceRegen = false, array $ignoredCILibs = [])
public function init($includeTests = false, $forceRegen = false, array $ignoredCIConfigs = [])
{
// build cache from factory
if ($this->cacheFactory) {
@ -88,7 +88,7 @@ class ThemeManifest implements ThemeList
if (!$forceRegen && $this->cache && ($data = $this->cache->get($this->cacheKey))) {
$this->themes = $data;
} else {
$this->regenerate($includeTests, $ignoredCILibs);
$this->regenerate($includeTests, $ignoredCIConfigs);
}
}
@ -130,16 +130,16 @@ class ThemeManifest implements ThemeList
* Regenerates the manifest by scanning the base path.
*
* @param bool $includeTests
* @param string[] $ignoredCILibs
* @param string[] $ignoredCIConfigs
*/
public function regenerate($includeTests = false, array $ignoredCILibs = [])
public function regenerate($includeTests = false, array $ignoredCIConfigs = [])
{
$finder = new ManifestFileFinder();
$finder->setOptions([
'include_themes' => false,
'ignore_dirs' => ['node_modules', THEMES_DIR],
'ignore_tests' => !$includeTests,
'ignore_ci_libraries' => $ignoredCILibs,
'ignore_ci_configs' => $ignoredCIConfigs,
'dir_callback' => [$this, 'handleDirectory']
]);

4
tests/php/Core/Manifest/ManifestFileFinderTest.php
View File

@ -91,7 +91,7 @@ class ManifestFileFinderTest extends SapphireTest
$finder = new ManifestFileFinder();
$finder->setOption('name_regex', '/\.txt$/');
$finder->setOption('ignore_tests', false);
$finder->setOption('ignore_ci_libraries', [Module::CI_PHPUNIT_FIVE]);
$finder->setOption('ignore_ci_configs', [Module::CI_PHPUNIT_FIVE]);
$this->assertFinderFinds(
$finder,
@ -115,7 +115,7 @@ class ManifestFileFinderTest extends SapphireTest
$finder = new ManifestFileFinder();
$finder->setOption('name_regex', '/\.txt$/');
$finder->setOption('ignore_tests', false);
$finder->setOption('ignore_ci_libraries', [Module::CI_PHPUNIT_FIVE, Module::CI_UNKNOWN]);
$finder->setOption('ignore_ci_configs', [Module::CI_PHPUNIT_FIVE, Module::CI_UNKNOWN]);
$this->assertFinderFinds(
$finder,

20
tests/php/Core/Manifest/ModuleTest.php
View File

@ -23,16 +23,22 @@ class ModuleTest extends SapphireTest
}
/**
* @dataProvider ciLibraryProvider
* @dataProvider ciConfigProvider
* @param string $fixture The folder containing our test composer file
* @param string $expectedPhpConfig
*/
public function testGetCILibrary($fixture, $expected)
public function testGetCIConfig($fixture, $expectedPhpConfig)
{
$path = __DIR__ . '/fixtures/phpunit-detection/' . $fixture;
$module = new Module($path, $path);
$this->assertEquals($expected, $module->getCILibrary());
$this->assertEquals(
$expectedPhpConfig,
$module->getCIConfig()['PHP'],
'PHP config is set to ' . $expectedPhpConfig
);
}
public function ciLibraryProvider()
public function ciConfigProvider()
{
return [
'empty require-dev' => ['empty-require-dev', Module::CI_UNKNOWN],
@ -43,19 +49,25 @@ class ModuleTest extends SapphireTest
'phpunit 5.0' => ['phpunit-five-zero', Module::CI_PHPUNIT_FIVE],
'phpunit 5.7' => ['phpunit-five-seven', Module::CI_PHPUNIT_FIVE],
'phpunit 5 exact version' => ['phpunit-five-exact-version', Module::CI_PHPUNIT_FIVE],
'phpunit 5 tilde' => ['phpunit-five-tilde', Module::CI_PHPUNIT_FIVE],
'sminnee 5.7' => ['sminnee-five-seven', Module::CI_PHPUNIT_FIVE],
'sminnee 5' => ['sminnee-five-seven', Module::CI_PHPUNIT_FIVE],
'sminnee 5 star' => ['sminnee-five-star', Module::CI_PHPUNIT_FIVE],
'phpunit 9' => ['phpunit-nine', Module::CI_PHPUNIT_NINE],
'phpunit 9.5' => ['phpunit-nine-five', Module::CI_PHPUNIT_NINE],
'future phpunit 9' => ['phpunit-nine-x', Module::CI_PHPUNIT_NINE],
'phpunit 9 exact version' => ['phpunit-nine-exact', Module::CI_PHPUNIT_NINE],
'recipe-testing 1' => ['recipe-testing-one', Module::CI_PHPUNIT_FIVE],
'recipe-testing 1.x' => ['recipe-testing-one-x', Module::CI_PHPUNIT_FIVE],
'recipe-testing 1.2.x' => ['recipe-testing-one-two-x', Module::CI_PHPUNIT_FIVE],
'recipe-testing 1 with stability flag' => ['recipe-testing-one-flag', Module::CI_PHPUNIT_FIVE],
'recipe-testing 2' => ['recipe-testing-two', Module::CI_PHPUNIT_NINE],
'recipe-testing 2.x' => ['recipe-testing-two-x', Module::CI_PHPUNIT_NINE],
'recipe-testing 2 exact' => ['recipe-testing-two-x', Module::CI_PHPUNIT_NINE],
];
}
}

13
tests/php/Core/Manifest/fixtures/phpunit-detection/phpunit-five-exact-version/composer.json Normal file
View File

@ -0,0 +1,13 @@
{
"name": "silverstripe/empty-require-dev",
"type": "silverstripe-vendor-module",
"description": "This fake module uses an exact version of phpunit 5",
"homepage": "https://www.silverstripe.org",
"license": "BSD-3-Clause",
"require": {
"silverstripe/recipe-cms": "4.4.x-dev as 4.4.0"
},
"require-dev": {
"phpunit/phpunit": "5.7.1234"
}
}

4
tests/php/Core/Manifest/fixtures/phpunit-detection/phpunit-five-seven/composer.json
View File

@ -1,13 +1,13 @@
{
"name": "silverstripe/empty-require-dev",
"type": "silverstripe-vendor-module",
"description": "This fake module uses any version of sminnee/PHPUnit 5",
"description": "This fake module uses any version of PHPUnit 5",
"homepage": "https://www.silverstripe.org",
"license": "BSD-3-Clause",
"require": {
"silverstripe/recipe-cms": "4.4.x-dev as 4.4.0"
},
"require-dev": {
"sminnee/phpunit": "^5"
"phpunit/phpunit": "^5"
}
}

13
tests/php/Core/Manifest/fixtures/phpunit-detection/phpunit-five-tilde/composer.json Normal file
View File

@ -0,0 +1,13 @@
{
"name": "silverstripe/empty-require-dev",
"type": "silverstripe-vendor-module",
"description": "This fake module uses a tilde constraint of phpunit 5",
"homepage": "https://www.silverstripe.org",
"license": "BSD-3-Clause",
"require": {
"silverstripe/recipe-cms": "4.4.x-dev as 4.4.0"
},
"require-dev": {
"phpunit/phpunit": "~5.9.1234"
}
}

13
tests/php/Core/Manifest/fixtures/phpunit-detection/phpunit-nine-exact/composer.json Normal file
View File

@ -0,0 +1,13 @@
{
"name": "silverstripe/empty-require-dev",
"type": "silverstripe-vendor-module",
"description": "This fake module uses an exact version of phpunit 9",
"homepage": "https://www.silverstripe.org",
"license": "BSD-3-Clause",
"require": {
"silverstripe/recipe-cms": "4.4.x-dev as 4.4.0"
},
"require-dev": {
"phpunit/phpunit": "9.9.9"
}
}

13
tests/php/Core/Manifest/fixtures/phpunit-detection/recipe-testing-one-flag/composer.json Normal file
View File

@ -0,0 +1,13 @@
{
"name": "silverstripe/empty-require-dev",
"type": "silverstripe-vendor-module",
"description": "This fake module uses a version of recipe testing 1 with a stability flag",
"homepage": "https://www.silverstripe.org",
"license": "BSD-3-Clause",
"require": {
"silverstripe/recipe-cms": "4.4.x-dev as 4.4.0"
},
"require-dev": {
"silverstripe/recipe-testing": "~1.8.0@stable"
}
}

13
tests/php/Core/Manifest/fixtures/phpunit-detection/recipe-testing-two-exact/composer.json Normal file
View File

@ -0,0 +1,13 @@
{
"name": "silverstripe/empty-require-dev",
"type": "silverstripe-vendor-module",
"description": "This fake module uses an exact version of recipe testing 2",
"homepage": "https://www.silverstripe.org",
"license": "BSD-3-Clause",
"require": {
"silverstripe/recipe-cms": "4.4.x-dev as 4.4.0"
},
"require-dev": {
"silverstripe/recipe-testing": "2.34.56"
}
}

13
tests/php/Core/Manifest/fixtures/phpunit-detection/sminnee-five-star/composer.json Normal file
View File

@ -0,0 +1,13 @@
{
"name": "silverstripe/empty-require-dev",
"type": "silverstripe-vendor-module",
"description": "This fake module uses a star constraint of sminnee 5",
"homepage": "https://www.silverstripe.org",
"license": "BSD-3-Clause",
"require": {
"silverstripe/recipe-cms": "4.4.x-dev as 4.4.0"
},
"require-dev": {
"sminnee/phpunit": "5.9.*"
}
}