Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

More modernization #260

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,4 @@
/Vagrantfile
/.vagrant
/node_modules
.phpunit.result.cache
1 change: 0 additions & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,3 @@ deploy:
repo: CouscousPHP/Couscous
tags: true
php: '7.1'

2 changes: 1 addition & 1 deletion bin/compile
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ $builder->buildFromIterator(
->files()
->name('*.php')
->name('*.pem*')
->exclude(['Tests', 'tests', 'phpunit'])
->exclude(['Tests', 'tests'])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the PHAR needs phpunit/phpunit/src/Framework/Assert/Functions.php, autoloaded by Composer in the latest PHPUnit.

->in($couscousDir . '/vendor')
->getIterator(),
$couscousDir
Expand Down
31 changes: 13 additions & 18 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,28 +16,23 @@
],
"require": {
"php": ">=7.1",
"symfony/console": "~3.0|~4.0|~5.0",
"symfony/filesystem": "~3.0|~4.0|~5.0",
"symfony/finder": "~3.0|~4.0|~5.0",
"symfony/process": "~3.0|~4.0|~5.0",
"symfony/yaml": "~3.0|~4.0|~5.0",
"twig/twig": "~1.10",
"erusev/parsedown": "~1.7.4",
"erusev/parsedown-extra": "~0.8",
"phine/phar": "~1.0",
"mnapoli/front-yaml": "~1.5",
"php-di/php-di": "^5.2.1",
"psr/log": "~1.0",
"symfony/console": "^3.0|^4.0|^5.0",
"symfony/filesystem": "^3.0|^4.0|^5.0",
"symfony/finder": "^3.0|^4.0|^5.0",
"symfony/process": "^3.0|^4.0|^5.0",
"symfony/yaml": "^3.0|^4.0|^5.0",
"twig/twig": "^1.10",
"erusev/parsedown": "^1.7",
"erusev/parsedown-extra": "^0.8",
"phine/phar": "^1.0",
"mnapoli/front-yaml": "^1.5",
"php-di/php-di": "^5.2",
"psr/log": "^1.0",
"padraic/phar-updater": "^1.0"
},
"require-dev": {
"phpunit/phpunit": "~7.5",
"phpunit/phpunit": "^7.5|^8.5|^9.3",
"squizlabs/php_codesniffer": "^3.3",
"vimeo/psalm": "^3.16"
},
"config": {
"platform": {
"php": "7.1.3"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why locking the PHP version to 7.1.3, since all versions are tested in the CI?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This config is problematic because it tells Composer the PHP version installed is PHP 7.1.3 (see https://getcomposer.org/doc/06-config.md#platform).
It means that, in the CI, when PHP 7.4 is used, Composer is actually installing packages for PHP 7.1.3.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know the initial reason of why the platform was set to php 7.1, I only specified the bugfix version so I could install psalm.

If there isn't a concrete reason to specify the php version it's better to remove it indeed. That being said I don't think it's problematic that composer install dependencies targeting 7.1, as long as the dependencies themselves are tested against 7.4.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's problematic, because it means you cannot test Couscous against the latest dependencies.
For instance, Symfony 5 was not tested (PHP > 7.2), and, if tested, we could have seen it was failing because of the missing return codes in the commands.

}
}
}
1 change: 0 additions & 1 deletion phpunit.xml.dist
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
convertWarningsToExceptions="true"
beStrictAboutOutputDuringTests="true"
beStrictAboutTestsThatDoNotTestAnything="true"
forceCoversAnnotation="true"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because some tests don't have the @cover annotation (and the latest PHPUnit doesn't like this).

bootstrap="./vendor/autoload.php">

<testsuites>
Expand Down
6 changes: 6 additions & 0 deletions src/Application/Cli/ClearCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,19 @@ public function __construct(Filesystem $filesystem)
parent::__construct();
}

/**
* {@inheritdoc}
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please revert these? (it's adding unnecessary noise)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Removing it everywhere instead then.

protected function configure(): void
{
$this
->setName('clear')
->setDescription('Clear all files generated by Couscous');
}

/**
* {@inheritdoc}
*/
protected function execute(InputInterface $input, OutputInterface $output): int
{
$dir = getcwd().'/.couscous';
Expand Down
9 changes: 1 addition & 8 deletions src/Application/Cli/DeployCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Input\InputOption;
use Symfony\Component\Console\Output\OutputInterface;
use Symfony\Component\Filesystem\Filesystem;

/**
* Generate and deploy the website.
Expand All @@ -31,21 +30,15 @@ class DeployCommand extends Command
*/
private $deployer;

/**
* @var Filesystem
*/
private $filesystem;

/**
* @var Git
*/
private $git;

public function __construct(Generator $generator, Deployer $deployer, Filesystem $filesystem, Git $git)
public function __construct(Generator $generator, Deployer $deployer, Git $git)
{
$this->generator = $generator;
$this->deployer = $deployer;
$this->filesystem = $filesystem;
$this->git = $git;

parent::__construct();
Expand Down
6 changes: 6 additions & 0 deletions src/Application/Cli/GenerateCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ public function __construct(Generator $generator)
parent::__construct();
}

/**
* {@inheritdoc}
*/
protected function configure(): void
{
$this
Expand Down Expand Up @@ -57,6 +60,9 @@ protected function configure(): void
);
}

/**
* {@inheritdoc}
*/
protected function execute(InputInterface $input, OutputInterface $output): int
{
/** @var string */
Expand Down
6 changes: 6 additions & 0 deletions src/Application/Cli/InitTemplateCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@
*/
class InitTemplateCommand extends Command
{
/**
* {@inheritdoc}
*/
protected function configure(): void
{
$this
Expand All @@ -34,6 +37,9 @@ protected function configure(): void
);
}

/**
* {@inheritdoc}
*/
protected function execute(InputInterface $input, OutputInterface $output): int
{
$fileExtension = '.twig';
Expand Down
4 changes: 2 additions & 2 deletions src/Application/Cli/PreviewCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ private function startLivereload(

private function isFound(string $executablePath): bool
{
if (strtoupper(substr(PHP_OS, 0, 3)) === 'WIN') {
if (stripos(PHP_OS, 'WIN') === 0) {
$folders = explode(';', (string) getenv('PATH'));
} else {
$folders = explode(':', (string) getenv('PATH'));
Expand All @@ -237,7 +237,7 @@ private function isFound(string $executablePath): bool

private function fileListToDisplay(array $files, string $sourceDirectory): string
{
$files = array_map(function (string $file) use ($sourceDirectory): string {
$files = array_map(static function (string $file) use ($sourceDirectory): string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for these, they are not useful and add noise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a suggestion from PHP Inspections EA:

[EA] This closure can be declared as static (better scoping; in some cases can improve performance).

Please tell me if you really want to remove them, I will do it.

return substr($file, strlen($sourceDirectory) + 1);
}, $files);

Expand Down
10 changes: 5 additions & 5 deletions src/Application/Cli/TravisAutoDeployCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -93,23 +93,23 @@ protected function execute(InputInterface $input, OutputInterface $output): int
if ($travisBranch !== 'master') {
$output->writeln('<comment>[NOT DEPLOYED] Deploying Couscous only for master branch</comment>');

return 0;
return 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a behavior change, not sure we want that. Why change that? (same for the lines below)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really a behavior change?
The return values have been added in the #259 PR.
Also it means the command has failed (EXIT_FAILURE), isn't it better? (can be useful when chaining commands in Bash for instance).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I added them I used 0 as the output only contains comments and no errors (and because that's the default value set by Symfony).

As far as I understand how the CI is configured this exit code won't change the behaviour of the CI as the command is run on after_success

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, the comments look like errors, and that's why I changed the return codes, but I could be wrong.

}

$isPullRequest = (int) getenv('TRAVIS_PULL_REQUEST') > 0 ? true : false;
$isPullRequest = (int) getenv('TRAVIS_PULL_REQUEST') > 0;

if ($isPullRequest) {
$output->writeln('<comment>[NOT DEPLOYED] Not deploying Couscous for pull requests</comment>');

return 0;
return 1;
}

// getting current php version to only deploy once
$currentPhpVersion = getenv('TRAVIS_PHP_VERSION');
if ($input->getOption('php-version') != $currentPhpVersion) {
if ($input->getOption('php-version') !== $currentPhpVersion) {
$output->writeln('<comment>This version of the documentation is already deployed</comment>');

return 0;
return 1;
}

// set git user data
Expand Down
2 changes: 2 additions & 0 deletions src/CommandRunner/Git.php
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,13 @@ public function getRemoteUrl(string $remote = 'origin'): string
* Check if the git repository in the provided directory has any uncommitted changes
*
* @param string $directory A directory containing a git repository
*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a reason to do this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a CS fix to be coherent with the rest of the codebase (there is a new line before every @return everywhere else).

* @return bool True if there are changes, false otherwise
*/
public function hasUncommittedChanges(string $directory): bool
{
$changes = $this->run($directory, "git diff-index --name-only HEAD");

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is also a CS fix to be coherent with the rest of the codebase.
It's a common CS (for instance in Symfony).

return !(ctype_space($changes) || $changes = '');
}

Expand Down
6 changes: 3 additions & 3 deletions src/Model/ExcludeList.php
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public function toArray(): array
$excluded = $this->excluded;
$excluded = array_filter($excluded, [$this, 'keepEntry']);
$excluded = array_map([$this, 'sanitizeEntry'], $excluded);
$excluded = array_map(function (string $entry): string {
$excluded = array_map(static function (string $entry): string {
return trim($entry, '/');
}, $excluded);

Expand All @@ -69,8 +69,8 @@ private function keepEntry($entry): bool
switch (true) {
case !is_string($entry) && !is_numeric($entry):
case $entry === '':
case is_string($entry) && (preg_match('/^[#!]/', $entry) > 0):
case is_string($entry) && (strpos($entry, '*') !== false):
case preg_match('/^[#!]/', (string) $entry) > 0:
case strpos((string) $entry, '*') !== false:
return false;

default:
Expand Down
2 changes: 1 addition & 1 deletion src/Model/Metadata.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ class Metadata implements \ArrayAccess
/**
* @var array
*/
private $values = [];
private $values;

public function __construct(array $values = [])
{
Expand Down
6 changes: 3 additions & 3 deletions src/Model/Project.php
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ public function findFilesByType(string $class): array
));
}

return array_filter($this->files, function (File $file) use ($class): bool {
return array_filter($this->files, static function (File $file) use ($class): bool {
return $file instanceof $class;
});
}
Expand All @@ -111,7 +111,7 @@ public function findFilesByType(string $class): array
public function sourceFiles(): Finder
{
/** @var list<string> */
$includedDirectories = $this->metadata['include'] ? $this->metadata['include'] : [];
$includedDirectories = $this->metadata['include'] ?: [];

// To be sure that included directories are under the source one
if (!empty($includedDirectories)) {
Expand All @@ -121,7 +121,7 @@ public function sourceFiles(): Finder
}

/** @var list<string> */
$exclude = $this->metadata['exclude'] ? $this->metadata['exclude'] : [];
$exclude = $this->metadata['exclude'] ?: [];
$excludedDirectories = new ExcludeList($exclude);

if (is_file($this->sourceDirectory.'/.gitignore')) {
Expand Down
3 changes: 3 additions & 0 deletions src/Model/WatchList/FileWatch.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ public function __construct(string $filename)
$this->time = time();
}

/**
* @return string[]
*/
public function getChangedFiles(): array
{
if (!file_exists($this->filename)) {
Expand Down
4 changes: 2 additions & 2 deletions src/Model/WatchList/WatchList.php
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,12 @@ public function watchDirectory(string $directory): void

public function getChangedFiles(): array
{
$files = array_map(function (WatchInterface $watch): array {
$files = array_map(static function (WatchInterface $watch): array {
return $watch->getChangedFiles();
}, $this->watches);

/** @var list<string> */
$files = call_user_func_array('array_merge', $files);
$files = array_merge(...$files);

return array_unique($files);
}
Expand Down
2 changes: 1 addition & 1 deletion src/Module/Config/Step/LoadConfig.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
*/
class LoadConfig implements Step
{
const FILENAME = 'couscous.yml';
private const FILENAME = 'couscous.yml';

/**
* @var Filesystem
Expand Down
3 changes: 2 additions & 1 deletion src/Module/Config/Step/OverrideBaseUrlForPreview.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,14 @@
namespace Couscous\Module\Config\Step;

use Couscous\Model\Project;
use Couscous\Step;

/**
* Override the baseUrl if we are in preview.
*
* @author Matthieu Napoli <matthieu@mnapoli.fr>
*/
class OverrideBaseUrlForPreview implements \Couscous\Step
class OverrideBaseUrlForPreview implements Step
{
public function __invoke(Project $project): void
{
Expand Down
3 changes: 2 additions & 1 deletion src/Module/Config/Step/OverrideConfigFromCLI.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,15 @@
namespace Couscous\Module\Config\Step;

use Couscous\Model\Project;
use Couscous\Step;
use Psr\Log\LoggerInterface;

/**
* Override config variables when specified using --config option.
*
* @author D.J. Marcolesco <dj.marcolesco@gmail.com>
*/
class OverrideConfigFromCLI implements \Couscous\Step
class OverrideConfigFromCLI implements Step
{
/**
* @var LoggerInterface
Expand Down
4 changes: 2 additions & 2 deletions src/Module/Markdown/Step/CreateHeadingIds.php
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ private function render(string $content): string
*/
private function addAttributeId(array $matches): string
{
$id = $this->slugfy($matches[2]);
$id = $this->slugify($matches[2]);
if (!isset($this->ids[$id])) {
$this->ids[$id] = 0;
}
Expand All @@ -67,7 +67,7 @@ private function addAttributeId(array $matches): string
return $replacement;
}

private function slugfy(string $text): string
private function slugify(string $text): string
{
$slug = trim($text);
$slug = strtr($slug, ' ', '-');
Expand Down
4 changes: 2 additions & 2 deletions src/Module/Markdown/Step/RewriteMarkdownLinks.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ class RewriteMarkdownLinks implements Step
*
* @link https://regex101.com/
*/
const MARKDOWN_LINK_REGEX = '/\[(?:[^\]]+)\]\(([^\)]+\/)?([A-Za-z0-9_\.\-]+\.md)([^.\)][^\)]*)?\)/';
private const MARKDOWN_LINK_REGEX = '/\[(?:[^\]]+)\]\(([^\)]+\/)?([A-Za-z0-9_\.\-]+\.md)([^.\)][^\)]*)?\)/';

public function __invoke(Project $project): void
{
Expand All @@ -41,7 +41,7 @@ private function replaceFilename(array $matches): string
{
$filename = strtolower($matches[2]);
$filename = str_replace('.md', '.html', $filename);
if ($filename == 'readme.html') {
if ($filename === 'readme.html') {
$filename = 'index.html';
}

Expand Down
2 changes: 1 addition & 1 deletion src/Module/Template/Step/AddLivereloadSnippet.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
*/
class AddLivereloadSnippet implements Step
{
const LIVERELOAD_SNIPPET = '
private const LIVERELOAD_SNIPPET = '
<!-- Livereload -->
<script>
document.write(\'<script src="http://\' + (location.host || \'localhost\').split(\':\')[0] +
Expand Down
Loading