T20 housekeeping

CHANGELOG.md

@@ -47,6 +47,7 @@ All notable changes are documented here. The format is based on [Keep a Changelo
 - `route:list` and `make:action` take an optional `ActionResolver`. [#5](https://github.com/phalcon/crest/issues/5)
 - `event:list` reads all listeners with one `getListenerMap()` call. [#1](https://github.com/phalcon/crest/issues/1)
 - `phalcon/talon` moved from `^0.8` to `^1.0.0`.
+- phpcs and php-cs-fixer use the shared Phalcon rules from `phalcon/code-quality`, added to `require-dev`.
 - `stub:publish` without a name skips the `project-*` stubs. Publish them by name; they go to the working directory or `--directory`. [#8](https://github.com/phalcon/crest/issues/8)
 - Rendering a stub fails when a placeholder has no value. The error names the stub. [#8](https://github.com/phalcon/crest/issues/8)
 - The config inference error names the missing psr-4 directories. [#18](https://github.com/phalcon/crest/issues/18)

composer.json

@@ -11,9 +11,9 @@
         "friendsofphp/php-cs-fixer": "^3",
         "pds/composer-script-names": "^1",
         "pds/skeleton": "^1",
+        "phalcon/code-quality": "^1.0",
         "phalcon/phalcon": "v6.0.x-dev",
         "phalcon/talon": "^1.0.0",
-        "phalcon/code-quality": "^1.0",
         "phpstan/phpstan": "^2",
         "phpunit/phpunit": "^10.5",
         "squizlabs/php_codesniffer": "^3 || ^4"
@@ -29,7 +29,7 @@
     },
     "bin": ["bin/crest"],
     "config": {
-    "optimize-autoloader": true,
+        "optimize-autoloader": true,
         "sort-packages": true,
         "allow-plugins": {
             "infection/extension-installer": true,

resources/docker/Dockerfile

@@ -12,10 +12,11 @@ ARG USER=crest
 ARG GROUP=crest
 
 # System packages, PHP extensions and an unprivileged user matching the host UID.
+# -o accepts an ID that the image already uses, for example GID 20 on macOS.
 RUN <<EOT
 set -eux
-groupadd -g "${GID}" "${GROUP}"
-useradd -l -m -u "${UID}" -g "${GID}" -s /bin/bash "${USER}"
+groupadd -o -g "${GID}" "${GROUP}"
+useradd -l -m -o -u "${UID}" -g "${GID}" -s /bin/bash "${USER}"
 apt-get update -y
 apt-get install -yq --no-install-recommends \
     curl git unzip zip libonig-dev libzip-dev locales

resources/phpcs.xml

@@ -15,4 +15,7 @@
     <!-- Files to scan -->
     <file>../src</file>
     <file>../tests</file>
+
+    <!-- Scratch space. Tests and Infection mutants write files here. -->
+    <exclude-pattern>*/tests/_output/*</exclude-pattern>
 </ruleset>

src/Command/NewCommand.php

@@ -86,7 +86,7 @@ final class NewCommand extends Command
      * first. It is a directory name, not a path. It is also the docker
      * container prefix, which must start with a letter or digit.
      */
-    private const NAME = '/^[A-Za-z0-9][A-Za-z0-9_-]*$/';
+    private const NAME = '/^[A-Za-z0-9][A-Za-z0-9_-]*\z/';
 
     /**
      * Variant => the composer requirement and its constraint. v5 needs 5.18,
@@ -101,10 +101,11 @@ final class NewCommand extends Command
     ];
 
     /**
-     * major.minor only. The Dockerfile base image `php:<version>-cli` has no
-     * patch tags, and composer.json uses the same value.
+     * major.minor only, with no leading zeros. The Dockerfile base image
+     * `php:<version>-cli` has no patch tags, and composer.json uses the same
+     * value.
      */
-    private const PHP = '/^\d+\.\d+$/';
+    private const PHP = '/^[1-9]\d*\.(?:0|[1-9]\d*)\z/';
 
     /**
      * The oldest PHP that runs the generated code. It uses readonly promoted

src/Command/Stub/PublishCommand.php

@@ -49,7 +49,7 @@ final class PublishCommand extends ProjectCommand
     /**
      * A packaged stub name. Hyphens are in because `action-view` is one.
      */
-    private const NAME = '/^[A-Za-z0-9_-]+$/';
+    private const NAME = '/^[A-Za-z0-9_-]+\z/';
 
     public function define(): Definition
     {

src/Console/Kernel.php

@@ -72,7 +72,7 @@ public static function globals(): Definition
     {
         return Definition::for('')
             ->option('config=s', 'Path to the project configuration file')
-            ->option('directory=s', 'Project root override')
+            ->option('directory=s', 'Directory to use instead of the working directory')
             ->option('trace', 'Show the full exception trace')
             ->option('help|h', 'Show this help')
             ->option('quiet|q', 'Suppress non-essential output');

src/Generator/ClassName.php

@@ -47,7 +47,7 @@ final class ClassName
      * Deliberately byte-oriented and not /u: that is exactly how PHP itself
      * decides what may name a class.
      */
-    private const PATTERN = '/^[A-Za-z_\x80-\xff][A-Za-z0-9_\x80-\xff]*$/';
+    private const PATTERN = '/^[A-Za-z_\x80-\xff][A-Za-z0-9_\x80-\xff]*\z/';
 
     /**
      * A namespace: one name, or more names that backslashes join. Each name

src/Process/ShellRunner.php

@@ -15,6 +15,7 @@
 
 use Crest\Console\Exceptions\Exception;
 
+use function basename;
 use function explode;
 use function getenv;
 use function is_dir;
@@ -23,7 +24,6 @@
 use function proc_close;
 use function proc_open;
 use function sprintf;
-use function str_contains;
 
 use const PATH_SEPARATOR;
 
@@ -61,18 +61,32 @@ public function run(array $command, ?string $directory = null): int
     /**
      * Whether the program can run. A path must name an executable file. A
      * bare name must be an executable file in a directory on the PATH.
+     *
+     * Windows: basename() also reads `\` as a separator, so PHP_BINARY is a
+     * path. PATHEXT gives the endings to try, so `docker` finds `docker.exe`.
+     * Other systems do not set PATHEXT.
+     *
+     * An empty PATH entry is skipped. It does not name the filesystem root.
      */
     private function exists(string $program): bool
     {
-        if (true === str_contains($program, '/')) {
+        if (basename($program) !== $program) {
             return true === is_file($program) && true === is_executable($program);
         }
 
+        $endings = ['', ...explode(';', (string) getenv('PATHEXT'))];
+
         foreach (explode(PATH_SEPARATOR, (string) getenv('PATH')) as $directory) {
-            $candidate = $directory . '/' . $program;
+            if ('' === $directory) {
+                continue;
+            }
+
+            foreach ($endings as $ending) {
+                $candidate = $directory . '/' . $program . $ending;
 
-            if (true === is_file($candidate) && true === is_executable($candidate)) {
-                return true;
+                if (true === is_file($candidate) && true === is_executable($candidate)) {
+                    return true;
+                }
             }
         }
 

src/Project/Config.php

@@ -104,7 +104,7 @@ private static function defaultPaths(Flavor $flavor): array
     {
         return match ($flavor) {
             Flavor::ADR => [
-                'action' => 'src/Action',
+                'action'     => 'src/Action',
                 // Not an ADR artifact: a crest command is the same class in any
                 // flavor. It sits here because ADR is the only populated set,
                 // and moves to a shared one when cli, mvc and micro arrive.
@@ -162,8 +162,6 @@ private static function fromArray(array $declared, string $root, string $source)
             $namespaces = $declared['namespaces'];
         }
 
-        // crest.php never restates the autoload map; namespaceFor() still needs
-        // it whenever `namespaces` does not answer the question outright.
         $bootstrap = null;
         if (true === isset($declared['bootstrap']) && true === is_string($declared['bootstrap'])) {
             $bootstrap = $declared['bootstrap'];
@@ -175,6 +173,9 @@ private static function fromArray(array $declared, string $root, string $source)
             $root,
             $paths,
             $namespaces,
+            // crest.php never restates the autoload map; namespaceFor()
+            // still needs it whenever `namespaces` does not answer the
+            // question outright.
             self::psr4Map($root),
             $source,
             $stated,
@@ -279,8 +280,8 @@ public function flavor(): Flavor
     }
 
     /**
-     * Whether the config file stated this top-level key, as opposed to it
-     * taking a default. `flavor`, `namespace`, `paths`, `namespaces`.
+     * Whether the config file stated this key, as opposed to it taking a
+     * default. `flavor`, `namespace`, `paths`, or `paths.<name>` for one path.
      */
     public function isDeclared(string $key): bool
     {

tests/Support/Project/NonEnumerableFront.php

@@ -0,0 +1,29 @@
+<?php
+
+/**
+ * This file is part of the Phalcon Crest.
+ *
+ * (c) Phalcon Team <team@phalcon.io>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+declare(strict_types=1);
+
+namespace Crest\Tests\Support\Project;
+
+/**
+ * Boots a container that can find services but cannot list them. No Phalcon
+ * container is like this, so the test sets the object: Bootstrap creates the
+ * front, and a test cannot give it the object in another way.
+ */
+final class NonEnumerableFront
+{
+    public static ?object $container = null;
+
+    public function boot(): ?object
+    {
+        return self::$container;
+    }
+}

tests/Unit/Command/Container/ListCommandTest.php

@@ -22,12 +22,15 @@
 use Crest\Tests\Support\Project\FailingFront;
 use Crest\Tests\Support\Project\NoBootFront;
 use Crest\Tests\Support\Project\NonContainerFront;
+use Crest\Tests\Support\Project\NonEnumerableFront;
 use Crest\Tests\Support\Project\ServicesFront;
 use Crest\Tests\Support\Project\WrongContainerFront;
 use Crest\Tests\Support\ScratchDirectory;
+use Phalcon\Contracts\Container\Service\Collection;
 use PHPUnit\Framework\TestCase;
 
 use function file_put_contents;
+use function get_class;
 use function preg_replace;
 use function str_replace;
 
@@ -47,6 +50,8 @@ protected function setUp(): void
 
     protected function tearDown(): void
     {
+        NonEnumerableFront::$container = null;
+
         $this->closeStreams();
         $this->removeScratchDirectory();
     }
@@ -87,6 +92,24 @@ public function testABootThatThrowsIsReportedAsABootFailure(): void
         );
     }
 
+    public function testAContainerThatCannotListItsServicesIsReported(): void
+    {
+        // Enumerable is an optional contract. A container can find services
+        // by name and still be unable to name them.
+        $container = $this->createStub(Collection::class);
+
+        NonEnumerableFront::$container = $container;
+        $this->declareFront(NonEnumerableFront::class);
+
+        $status = $this->runCommand();
+
+        $this->assertSame(1, $status);
+        $this->assertStringContainsString(
+            get_class($container) . ' cannot list its services',
+            $this->readStderr()
+        );
+    }
+
     public function testAFrontWithNoBootIsReported(): void
     {
         $this->declareFront(NoBootFront::class);

tests/Unit/Command/Make/ActionCommandTest.php

@@ -28,6 +28,8 @@
 use function file_put_contents;
 use function mkdir;
 
+use const PHP_EOL;
+
 final class ActionCommandTest extends TestCase
 {
     use GeneratesInAScratchProject;
@@ -350,10 +352,17 @@ public function testTheViewResponderReportsTheTemplateItAsksFor(): void
         // renderer knows where it lives.
         $this->runCommand(['GET', '/company/all', '--responder=view']);
 
-        $output = $this->readStdout();
-
-        $this->assertStringContainsString('Nothing renders it yet', $output);
-        $this->assertStringContainsString('    company/all/index', $output);
+        // Asserted whole: each blank line and each text line is a separate call.
+        $expected = 'Answers GET /company/all' . PHP_EOL
+            . PHP_EOL
+            . 'Nothing renders it yet. The responder asks for this template:' . PHP_EOL
+            . PHP_EOL
+            . '    company/all/index' . PHP_EOL
+            . PHP_EOL
+            . 'Create it wherever your renderer looks. Renderer::render() takes a name, not a path, '
+            . 'so the directory and the extension belong to the renderer rather than to crest.' . PHP_EOL;
+
+        $this->assertStringEndsWith($expected, $this->readStdout());
     }
 
     public function testUnknownResponderIsRejected(): void

tests/Unit/Command/NewCommandTest.php

@@ -49,6 +49,18 @@ protected function tearDown(): void
         $this->endScratchProject();
     }
 
+    /**
+     * @return iterable<string, array{string}>
+     */
+    public static function unusablePhpVersions(): iterable
+    {
+        // Each one passes version_compare(), but no `php:<version>-cli` image
+        // has that tag.
+        yield 'leading zero in the major' => ['08.4'];
+        yield 'leading zero in the minor' => ['8.01'];
+        yield 'trailing newline' => ["8.4\n"];
+    }
+
     /**
      * @return iterable<string, array{string}>
      */
@@ -142,6 +154,20 @@ public function testAnUnusableNamespaceIsRejected(): void
         $this->assertDirectoryDoesNotExist($this->root . '/my-app');
     }
 
+    /**
+     * @dataProvider unusablePhpVersions
+     */
+    public function testAnUnusablePhpVersionIsRejected(string $version): void
+    {
+        $status = $this->runCommand(['my-app', '--php', $version]);
+
+        $this->assertSame(1, $status);
+        $this->assertStringContainsString(
+            sprintf("'%s' is not a PHP version; expected major.minor, e.g. 8.4", $version),
+            $this->readStderr()
+        );
+    }
+
     public function testAPhpVersionBelowTheFloorIsRejected(): void
     {
         $status = $this->runCommand(['my-app', '--php', '8.0']);
@@ -189,6 +215,14 @@ public function testAProjectNameThatIsAPathIsRejected(): void
         $this->assertDirectoryDoesNotExist(dirname($this->root) . '/elsewhere');
     }
 
+    public function testAProjectNameWithATrailingNewlineIsRejected(): void
+    {
+        $status = $this->runCommand(["my-app\n"]);
+
+        $this->assertSame(1, $status);
+        $this->assertStringContainsString("'my-app\n' is not a usable project name", $this->readStderr());
+    }
+
     public function testAProjectStubFromStubPublishIsUsed(): void
     {
         // stub:publish must write where `new` reads, with the same
@@ -335,6 +369,19 @@ public function testForceWritesIntoANonEmptyDirectory(): void
         $this->assertFileExists($this->root . '/my-app/composer.json');
     }
 
+    public function testHelpDoesNotCallTheDirectoryAProjectRoot(): void
+    {
+        // `new` has no project yet. The option replaces the working directory,
+        // and the project goes into it.
+        $status = $this->runInWorkingDirectory(['--help']);
+
+        $this->assertSame(0, $status);
+        $this->assertMatchesRegularExpression(
+            '/--directory +Directory to use instead of the working directory\R/',
+            $this->readStdout()
+        );
+    }
+
     public function testNameArgumentIsRequired(): void
     {
         $status = $this->runCommand([]);