Skip to content

Commit f1c736f

Browse files
stronk7stronk7
committed
Move phpcs command from library mode to binary mode
Until now we were using the phpcs in "library mode", i.e. like it was a library and using its API to run it within PHP. That had a few advantages but some problems, having to "cheat" here and there to get everything working. More noticeably, the include-paths and other details were conflicting badly with the, now isolated and installed by composer moodle-cs standard, and the phpcs.xml files now present in core. So this commit just changes that "unsupported" library use by a more natural use as binary, so we just run it from shell, with proper CLI options and so on. The only detail a little bit tricky is that now, in order to continue supporting the max-warnings option in moodle-plugin-ci, instead of getting the number of errors and warnings from the API, we have to generate the json report and extract totals information from there. Anyway, it seems to be stable enough. Covered with tests.
1 parent b28a957 commit f1c736f

File tree

6 files changed

+144
-227
lines changed

6 files changed

+144
-227
lines changed

‎docs/CHANGELOG.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ The format of this change log follows the advice given at [Keep a CHANGELOG](htt
1414
- ACTION SUGGESTED: Ensure that the `pcov` or `xdebug` extensions are installed in your environment to get 'moodle-plugin-ci' using them automatically.
1515

1616
### Changed
17-
- Switched from [local_codechecker]() to [moodle-cs]() for checking the coding style. Previously, `moodle-plugin-ci` (and other tools) required `local_codechecker` (that includes both `PHP_Codesniffer` and the `moodle` standard) to verify the coding style. Now, the `moodle` standard has been moved to be a standalone repository and all the tools will be using it and installing `PHP_Codesniffer` via composer. No changes in behavior are expected.
17+
- Switched from [local_codechecker](https://github.com/moodlehq/moodle-local_codechecker) to [moodle-cs](https://github.com/moodlehq/moodle-cs) for checking the coding style. Previously, `moodle-plugin-ci` (and other tools) required `local_codechecker` (that includes both `PHP_Codesniffer` and the `moodle` standard) to verify the coding style. Now, the `moodle` standard has been moved to be a standalone repository and all the tools will be using it and installing `PHP_Codesniffer` via composer. No changes in behavior are expected.
1818

1919
## [3.2.6] - 2022-05-10
2020
### Added

‎src/Command/CodeCheckerCommand.php‎

Lines changed: 56 additions & 111 deletions
Original file line numberDiff line numberDiff line change
@@ -12,49 +12,31 @@
1212

1313
namespace MoodlePluginCI\Command;
1414

15-
use MoodlePluginCI\StandardResolver;
16-
use PHP_CodeSniffer\Config;
17-
use PHP_CodeSniffer\Reporter;
18-
use PHP_CodeSniffer\Runner;
19-
use PHP_CodeSniffer\Util\Timing;
2015
use Symfony\Component\Console\Input\InputInterface;
2116
use Symfony\Component\Console\Input\InputOption;
2217
use Symfony\Component\Console\Output\OutputInterface;
18+
use Symfony\Component\Filesystem\Filesystem;
2319
use Symfony\Component\Finder\Finder;
24-
25-
// Cannot be autoload from composer, because this autoloader is used for both
26-
// phpcs and any standard Sniff, so it must be loaded at the end. For more info, see:
27-
// https://github.com/squizlabs/PHP_CodeSniffer/issues/1463#issuecomment-300637855
28-
//
29-
// The alternative is to, instead of using PHP_CodeSniffer like a library, just
30-
// use the binaries bundled with it (phpcs, phpcbf...) but we want to use as lib for now.
31-
require_once __DIR__.'/../../vendor/squizlabs/php_codesniffer/autoload.php';
20+
use Symfony\Component\Process\ProcessBuilder;
3221

3322
/**
34-
* Run Moodle Code Checker on a plugin.
23+
* Run Moodle CodeSniffer standard on a plugin.
3524
*/
3625
class CodeCheckerCommand extends AbstractPluginCommand
3726
{
38-
/**
39-
* The coding standard to use.
40-
*
41-
* @var string
42-
*/
43-
protected $standard;
27+
use ExecuteTrait;
4428

4529
/**
46-
* Used to find the files to process.
47-
*
48-
* @var Finder
30+
* @var string Path to the temp file where the json report results will be stored
4931
*/
50-
protected $finder;
32+
protected $tempFile;
5133

5234
protected function configure()
5335
{
5436
parent::configure();
5537

5638
$this->setName('codechecker')
57-
->setDescription('Run Moodle Code Checker on a plugin')
39+
->setDescription('Run Moodle CodeSniffer standard on a plugin')
5840
->addOption('standard', 's', InputOption::VALUE_REQUIRED, 'The name or path of the coding standard to use', 'moodle')
5941
->addOption('max-warnings', null, InputOption::VALUE_REQUIRED,
6042
'Number of warnings to trigger nonzero exit code - default: -1', -1);
@@ -63,106 +45,69 @@ protected function configure()
6345
protected function initialize(InputInterface $input, OutputInterface $output)
6446
{
6547
parent::initialize($input, $output);
66-
67-
// Resolve the coding standard.
68-
$resolver = new StandardResolver();
69-
$this->standard = $input->getOption('standard');
70-
if ($resolver->hasStandard($this->standard)) {
71-
$this->standard = $resolver->resolve($this->standard);
72-
}
73-
74-
$this->finder = Finder::create()->name('*.php');
48+
$this->initializeExecute($output, $this->getHelper('process'));
49+
$this->tempFile = sys_get_temp_dir().'/moodle-plugin-ci-code-checker-summary-'.time();
7550
}
7651

7752
protected function execute(InputInterface $input, OutputInterface $output)
7853
{
79-
$this->outputHeading($output, 'Moodle Code Checker on %s');
54+
$this->outputHeading($output, 'Moodle CodeSniffer standard on %s');
8055

81-
$files = $this->plugin->getFiles($this->finder);
56+
$files = $this->plugin->getFiles(Finder::create()->name('*.php'));
8257
if (count($files) === 0) {
8358
return $this->outputSkip($output);
8459
}
8560

86-
// Needed constant.
87-
if (defined('PHP_CODESNIFFER_CBF') === false) {
88-
define('PHP_CODESNIFFER_CBF', false);
61+
$builder = ProcessBuilder::create()
62+
->setPrefix('php')
63+
->add(__DIR__.'/../../vendor/squizlabs/php_codesniffer/bin/phpcs')
64+
->add('--standard='.($input->getOption('standard') ?: 'moodle'))
65+
->add('--extensions=php')
66+
->add('-p')
67+
->add('-w')
68+
->add('-s')
69+
->add('--no-cache')
70+
->add($output->isDecorated() ? '--colors' : '--no-colors')
71+
->add('--report-full')
72+
->add('--report-width=132')
73+
->add('--encoding=utf-8')
74+
->setWorkingDirectory($this->plugin->directory)
75+
->setTimeout(null);
76+
77+
// If we aren't using the max-warnings option, then we can forget about warnings and tell phpcs
78+
// to ignore them for exit-code purposes (still they will be reported in the output).
79+
if ($input->getOption('max-warnings') < 0) {
80+
$builder->add('--runtime-set')->add('ignore_warnings_on_exit')->add(' 1');
81+
} else {
82+
// If we are using the max-warnings option, we need the summary report somewhere to get
83+
// the total number of errors and warnings from there.
84+
$builder->add('--report-json='.$this->tempFile);
8985
}
9086

91-
Timing::startTiming();
92-
93-
$runner = new Runner();
94-
$runner->config = new Config(['--parallel=1']); // Pass a param to shortcut params coming from caller CLI.
95-
96-
// This is not needed normally, because phpcs loads the CodeSniffer.conf from its
97-
// root directory. But when this is run from within a .phar file, it expects the
98-
// config file to be out from the phar, in the same directory.
99-
//
100-
// While this approach is logic and enabled to configure phpcs, we don't want that
101-
// in this case, we just want to ensure phpcs knows about our standards,
102-
// so we are going to force the installed_paths config here.
103-
//
104-
// And it needs need to do it BEFORE the runner init! (or it's lost)
105-
//
106-
// Note: the "moodle" one is not really needed, because it's autodetected normally,
107-
// but the PHPCompatibility one is. There are some issues about version PHPCompatibility 10
108-
// to stop requiring to be configured here, but that's future version. Revisit this when
109-
// available.
110-
//
111-
// Note: the paths are relative to the base CodeSniffer directory, aka, the directory
112-
// where "src" sits.
113-
$runner->config->setConfigData('installed_paths', './../../phpcompatibility/php-compatibility/PHPCompatibility');
114-
$runner->config->standards = [$this->standard]; // Also BEFORE init() or it's lost.
115-
116-
$runner->init();
117-
118-
$runner->config->parallel = 1;
119-
$runner->config->reports = ['full' => null];
120-
$runner->config->colors = $output->isDecorated();
121-
$runner->config->verbosity = 0;
122-
$runner->config->encoding = 'utf-8';
123-
$runner->config->showProgress = true;
124-
$runner->config->showSources = true;
125-
$runner->config->interactive = false;
126-
$runner->config->cache = false;
127-
$runner->config->extensions = ['php' => 'PHP'];
128-
$runner->config->reportWidth = 132;
129-
130-
$runner->config->files = $files;
131-
132-
$runner->config->setConfigData('testVersion', PHP_MAJOR_VERSION.'.'.PHP_MINOR_VERSION, true);
133-
134-
// Create the reporter to manage all the reports from the run.
135-
$runner->reporter = new Reporter($runner->config);
136-
137-
// And build the file list to iterate over.
138-
/** @var object[] $todo */
139-
$todo = new \PHP_CodeSniffer\Files\FileList($runner->config, $runner->ruleset);
140-
$numFiles = count($todo);
141-
$numProcessed = 0;
142-
143-
foreach ($todo as $file) {
144-
if ($file->ignored === false) {
145-
try {
146-
$runner->processFile($file);
147-
++$numProcessed;
148-
$runner->printProgress($file, $numFiles, $numProcessed);
149-
} catch (\PHP_CodeSniffer\Exceptions\DeepExitException $e) {
150-
echo $e->getMessage();
151-
152-
return $e->getCode();
153-
} catch (\Exception $e) {
154-
$error = 'Problem during processing; checking has been aborted. The error message was: '.$e->getMessage();
155-
$file->addErrorOnLine($error, 1, 'Internal.Exception');
156-
}
157-
$file->cleanUp();
158-
}
87+
// Add the files to process.
88+
foreach ($files as $file) {
89+
$builder->add($file);
15990
}
16091

161-
// Have finished, generate the final reports.
162-
$runner->reporter->printReports();
92+
$process = $this->execute->passThroughProcess($builder->getProcess());
93+
94+
// If we aren't using the max-warnings option, process exit code is enough for us.
95+
if ($input->getOption('max-warnings') < 0) {
96+
return $process->isSuccessful() ? 0 : 1;
97+
}
16398

164-
$maxwarnings = (int) $input->getOption('max-warnings');
99+
// Arrived here, we are playing with max-warnings, so we have to decide the exit code
100+
// based on the existence of errors and the number of warnings compared with the threshold.
101+
$totalErrors = 0;
102+
$totalWarnings = 0;
103+
$jsonFile = trim(file_get_contents($this->tempFile));
104+
if ($json = json_decode($jsonFile, false)) {
105+
$totalErrors = (int) $json->totals->errors;
106+
$totalWarnings = (int) $json->totals->warnings;
107+
}
108+
(new Filesystem())->remove($this->tempFile); // Remove the temporal summary file.
165109

166-
return ($runner->reporter->totalErrors > 0 || ($maxwarnings >= 0 && $runner->reporter->totalWarnings > $maxwarnings)) ? 1 : 0;
110+
// With errors or warnings over the max-warnings threshold, fail the command.
111+
return ($totalErrors > 0 || ($totalWarnings > $input->getOption('max-warnings'))) ? 1 : 0;
167112
}
168113
}

‎src/Command/CodeFixerCommand.php‎

Lines changed: 24 additions & 91 deletions
Original file line numberDiff line numberDiff line change
@@ -12,21 +12,11 @@
1212

1313
namespace MoodlePluginCI\Command;
1414

15-
use PHP_CodeSniffer\Config;
16-
use PHP_CodeSniffer\Reporter;
17-
use PHP_CodeSniffer\Runner;
18-
use PHP_CodeSniffer\Util\Timing;
1915
use Symfony\Component\Console\Input\InputInterface;
2016
use Symfony\Component\Console\Input\InputOption;
2117
use Symfony\Component\Console\Output\OutputInterface;
22-
23-
// Cannot be autoload from composer, because this autoloader is used for both
24-
// phpcs and any standard Sniff, so it must be loaded at the end. For more info, see:
25-
// https://github.com/squizlabs/PHP_CodeSniffer/issues/1463#issuecomment-300637855
26-
//
27-
// The alternative is to, instead of using PHP_CodeSniffer like a library, just
28-
// use the binaries bundled with it (phpcs, phpcbf...) but we want to use as lib for now.
29-
require_once __DIR__.'/../../vendor/squizlabs/php_codesniffer/autoload.php';
18+
use Symfony\Component\Finder\Finder;
19+
use Symfony\Component\Process\ProcessBuilder;
3020

3121
/**
3222
* Run PHP Code Beautifier and Fixer on a plugin.
@@ -46,90 +36,33 @@ protected function execute(InputInterface $input, OutputInterface $output)
4636
{
4737
$this->outputHeading($output, 'Code Beautifier and Fixer on %s');
4838

49-
$files = $this->plugin->getFiles($this->finder);
39+
$files = $this->plugin->getFiles(Finder::create()->name('*.php'));
5040
if (count($files) === 0) {
51-
return $this->outputSkip($output, 'No files found to process.');
52-
}
53-
54-
// Needed constant.
55-
if (defined('PHP_CODESNIFFER_CBF') === false) {
56-
define('PHP_CODESNIFFER_CBF', true);
41+
return $this->outputSkip($output);
5742
}
5843

59-
Timing::startTiming();
60-
61-
$runner = new Runner();
62-
$runner->config = new Config(['--parallel=1']); // Pass a param to shortcut params coming from caller CLI.
63-
64-
// This is not needed normally, because phpcs loads the CodeSniffer.conf from its
65-
// root directory. But when this is run from within a .phar file, it expects the
66-
// config file to be out from the phar, in the same directory.
67-
//
68-
// While this approach is logic and enabled to configure phpcs, we don't want that
69-
// in this case, we just want to ensure phpcs knows about our standards,
70-
// so we are going to force the installed_paths config here.
71-
//
72-
// And it needs need to do it BEFORE the runner init! (or it's lost)
73-
//
74-
// Note: the "moodle" one is not really needed, because it's autodetected normally,
75-
// but the PHPCompatibility one is. There are some issues about version PHPCompatibility 10
76-
// to stop requiring to be configured here, but that's future version. Revisit this when
77-
// available.
78-
//
79-
// Note: the paths are relative to the base CodeSniffer directory, aka, the directory
80-
// where "src" sits.
81-
$runner->config->setConfigData('installed_paths', './../../phpcompatibility/php-compatibility/PHPCompatibility');
82-
$runner->config->standards = [$this->standard]; // Also BEFORE init() or it's lost.
83-
84-
$runner->init();
85-
86-
$runner->config->parallel = 1;
87-
$runner->config->reports = ['cbf' => null];
88-
$runner->config->colors = $output->isDecorated();
89-
$runner->config->verbosity = 0;
90-
$runner->config->encoding = 'utf-8';
91-
$runner->config->showProgress = true;
92-
$runner->config->showSources = true;
93-
$runner->config->interactive = false;
94-
$runner->config->cache = false;
95-
$runner->config->extensions = ['php' => 'PHP'];
96-
$runner->config->reportWidth = 132;
97-
98-
$runner->config->files = $files;
99-
100-
$runner->config->setConfigData('testVersion', PHP_MAJOR_VERSION.'.'.PHP_MINOR_VERSION, true);
101-
102-
// Create the reporter to manage all the reports from the run.
103-
$runner->reporter = new Reporter($runner->config);
104-
105-
// And build the file list to iterate over.
106-
/** @var object[] $todo */
107-
$todo = new \PHP_CodeSniffer\Files\FileList($runner->config, $runner->ruleset);
108-
$numFiles = count($todo);
109-
$numProcessed = 0;
110-
111-
foreach ($todo as $file) {
112-
if ($file->ignored === false) {
113-
try {
114-
$runner->processFile($file);
115-
++$numProcessed;
116-
$runner->printProgress($file, $numFiles, $numProcessed);
117-
} catch (\PHP_CodeSniffer\Exceptions\DeepExitException $e) {
118-
echo $e->getMessage();
119-
120-
return $e->getCode();
121-
} catch (\Exception $e) {
122-
$error = 'Problem during processing; checking has been aborted. The error message was: '.$e->getMessage();
123-
$file->addErrorOnLine($error, 1, 'Internal.Exception');
124-
}
125-
$file->cleanUp();
126-
}
44+
$builder = ProcessBuilder::create()
45+
->setPrefix('php')
46+
->add(__DIR__.'/../../vendor/squizlabs/php_codesniffer/bin/phpcbf')
47+
->add('--standard='.($input->getOption('standard') ?: 'moodle'))
48+
->add('--extensions=php')
49+
->add('-p')
50+
->add('-w')
51+
->add('-s')
52+
->add('--no-cache')
53+
->add($output->isDecorated() ? '--colors' : '--no-colors')
54+
->add('--report-full')
55+
->add('--report-width=132')
56+
->add('--encoding=utf-8')
57+
->setWorkingDirectory($this->plugin->directory)
58+
->setTimeout(null);
59+
60+
// Add the files to process.
61+
foreach ($files as $file) {
62+
$builder->add($file);
12763
}
12864

129-
// Have finished, generate the final reports and timing.
130-
$runner->reporter->printReports();
131-
echo PHP_EOL;
132-
Timing::printRunTime();
65+
$this->execute->passThroughProcess($builder->getProcess());
13366

13467
return 0;
13568
}

‎src/Installer/VendorInstaller.php‎

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -88,15 +88,6 @@ public function install()
8888
}
8989

9090
$this->execute->mustRun(new Process('npx grunt ignorefiles', $this->moodle->directory, null, null, null));
91-
92-
// Remove the .phpcs files, recently introduced for 311_STABLE and up. At this stage moodle-plugin-ci
93-
// is not ready to have them around, because of the way we run codechecker (from local_codechecker
94-
// dependency).
95-
// Once we remove that dependency and move to composer install of both phpcs and moodle-cs... there
96-
// shouldn't be any more problems with these files and the following lines can be safely removed.
97-
// So this is just an interim hack to allow moodle-plugin-ci to continue working the "legacy" way.
98-
$this->execute->mustRun(new Process('rm -fr .phpcs.xml', $this->moodle->directory, null, null, null));
99-
$this->execute->mustRun(new Process('rm -fr .phpcs.xml.dist', $this->moodle->directory, null, null, null));
10091
}
10192

10293
public function stepCount()

0 commit comments

Comments
 (0)