diff --git a/src/Console/Commands/InstallCommand.php b/src/Console/Commands/InstallCommand.php index 24646d8..02c84dc 100644 --- a/src/Console/Commands/InstallCommand.php +++ b/src/Console/Commands/InstallCommand.php @@ -184,9 +184,7 @@ public function envValue(string $key): ?string { $env = @file_get_contents($this->path('.env')); - return ($env !== false && preg_match('/^'.preg_quote($key, '/').'=(.+)$/m', $env, $m) === 1) - ? trim(trim($m[1]), "\"'") - : null; + return $env === false ? null : self::readEnvLine($env, $key); } /** @@ -434,9 +432,7 @@ protected function applyIdentityToEnv(string $env, string $name, string $slug, s ]; foreach ($replaceable as $key => [$default, $value]) { - $current = preg_match('/^'.preg_quote($key, '/').'=(.*)$/m', $env, $m) - ? trim(trim($m[1]), "\"'") - : null; + $current = self::readEnvLine($env, $key); if ($current === null || $current === '' || $current === $default) { $env = self::setEnvLine($env, $key, $value); @@ -458,6 +454,14 @@ protected function applyIdentityToEnv(string $env, string $name, string $slug, s return $env; } + /** A key's value from raw .env text, or null when the key is absent. */ + public static function readEnvLine(string $env, string $key): ?string + { + return preg_match('/^'.preg_quote($key, '/').'=(.*)$/m', $env, $m) === 1 + ? trim(trim($m[1]), "\"'") + : null; + } + /** Replace a key's value, appending the line when the key is absent. */ public static function setEnvLine(string $env, string $key, string $value): string { @@ -556,7 +560,7 @@ protected function doInstallModules(array $selected): void return $ok = $process->isSuccessful(); }); - if (! $ok) { + if (! $this->composerRequireSucceeded($ok, $selected)) { $this->components->warn('Module installation failed — skipping patches, sync, and migrations.'); return; @@ -610,6 +614,54 @@ public function rewriteCrossModuleImports(): void } } + /** + * Whether every requested package is actually installed, per composer.lock. + * + * `composer require` exits non-zero when any post-update-cmd script fails, even + * though the packages resolved and installed perfectly — the skeleton's documented + * `boost:update` hook does exactly that until Boost is set up. Judging module + * installation by the exit code alone turns that into a failed install, so ask + * what actually landed instead. + * + * @param string[] $packages + */ + public function modulesAreInstalled(array $packages): bool + { + $lock = json_decode((string) @file_get_contents($this->path('composer.lock')), true); + + if (! is_array($lock)) { + return false; + } + + $installed = array_column( + array_merge($lock['packages'] ?? [], $lock['packages-dev'] ?? []), + 'name', + ); + + return array_diff(array_map('strtolower', $packages), array_map('strtolower', $installed)) === []; + } + + /** + * Decide whether a non-zero `composer require` really failed, reporting either way. + * + * @param string[] $packages + */ + public function composerRequireSucceeded(bool $exitedCleanly, array $packages): bool + { + if ($exitedCleanly) { + return true; + } + + if (! $this->modulesAreInstalled($packages)) { + return false; + } + + $this->warn('Composer reported an error, but every module is installed — a post-install script failed.'); + $this->line(' If that was `boost:update`, run `php artisan boost:install` to configure Boost.'); + + return true; + } + public function moduleHasSeeder(string $name): bool { $seederFile = 'database/seeders/DatabaseSeeder.php'; diff --git a/src/Environments/DockerEnvironment.php b/src/Environments/DockerEnvironment.php index 922c677..9c82c55 100644 --- a/src/Environments/DockerEnvironment.php +++ b/src/Environments/DockerEnvironment.php @@ -745,7 +745,7 @@ protected function installModules(InstallCommand $command): bool timeout: 300, ); - if (! $ok) { + if (! $command->composerRequireSucceeded($ok, $modules)) { $command->warn('Failed to install one or more modules — skipping patches, sync, and migrations.'); return false; @@ -812,57 +812,55 @@ protected function setDockerEnvDefaults(InstallCommand $command): void /** @param array $ports Env key => port, forced over any existing value. */ protected function applyDockerEnvDefaults(string $env, bool $ssl = true, array $ports = []): string { - $slug = 'saucebase'; - if (preg_match('/^APP_SLUG=([^\s]+)/m', $env, $m)) { - $slug = trim($m[1], "\"'"); - } + // Both bind $env by reference so a read always sees the writes before it; + // an arrow function would capture it by value and go stale after the first set. + $read = function (string $key) use (&$env): ?string { + return InstallCommand::readEnvLine($env, $key); + }; + $set = function (string $key, string $value) use (&$env): void { + $env = InstallCommand::setEnvLine($env, $key, $value); + }; + + $slug = $read('APP_SLUG') ?: 'saucebase'; // The identity pass (InstallCommand::applyAppIdentity) always runs first, so // APP_HOST carries the domain that was chosen for this app. - $host = 'localhost'; - if (preg_match('/^APP_HOST=([^\s]+)/m', $env, $m)) { - $host = trim($m[1], "\"'"); - } + $host = $read('APP_HOST') ?: 'localhost'; - // Docker always needs mysql, not sqlite - if (preg_match('/^DB_CONNECTION=(.*)$/m', $env, $m) && trim($m[1]) !== 'mysql') { - $env = preg_replace('/^DB_CONNECTION=.*$/m', 'DB_CONNECTION=mysql', $env); - } elseif (! preg_match('/^DB_CONNECTION=/m', $env)) { - $env .= "\nDB_CONNECTION=mysql"; + // Docker always needs mysql rather than sqlite, and routes mail through the + // Mailpit container over SMTP. + foreach (['DB_CONNECTION' => 'mysql', 'MAIL_MAILER' => 'smtp'] as $key => $required) { + if ($read($key) !== $required) { + $set($key, $required); + } } - // Docker routes mail through the Mailpit container via SMTP - if (preg_match('/^MAIL_MAILER=(.*)$/m', $env, $m) && trim($m[1]) !== 'smtp') { - $env = preg_replace('/^MAIL_MAILER=.*$/m', 'MAIL_MAILER=smtp', $env); - } elseif (! preg_match('/^MAIL_MAILER=/m', $env)) { - $env .= "\nMAIL_MAILER=smtp"; + // Point mail at the Mailpit container. Compose interpolates ${MAIL_HOST} from + // this file, so a stale "localhost" here is what the app container actually + // receives — and inside that container localhost is the app itself, not Mailpit. + // A real SMTP host the user configured is left alone. + if (in_array((string) $read('MAIL_HOST'), ['', 'localhost', '127.0.0.1', '::1'], true)) { + $set('MAIL_HOST', 'mailpit'); + // The container-internal port, not the published one (FORWARD_MAILPIT_PORT). + $set('MAIL_PORT', '1025'); } - // Set APP_URL to match the chosen host and SSL mode, carrying the published - // port whenever it is not the scheme's default one. + // APP_URL follows the chosen host and SSL mode, carrying the published port + // whenever it is not the scheme's default. With no override this run, fall back + // to a port an earlier run persisted — otherwise a resume drops :8443. $portKey = $ssl ? 'APP_HTTPS_PORT' : 'APP_PORT'; - $appPort = $ports[$portKey] ?? null; + $appPort = $ports[$portKey] ?? (is_numeric($read($portKey)) ? (int) $read($portKey) : null); + $suffix = ($appPort !== null && $appPort !== ($ssl ? 443 : 80)) ? ':'.$appPort : ''; - // No override this run: fall back to a port an earlier run already persisted, - // otherwise a resume would silently drop :8443 from the URL. - if ($appPort === null && preg_match('/^'.$portKey.'=(\d+)/m', $env, $m)) { - $appPort = (int) $m[1]; - } + // Correct a URL for localhost or for the chosen host (scheme and port may have + // changed); leave a genuinely custom one alone. + $url = $read('APP_URL'); - $suffix = ($appPort !== null && $appPort !== ($ssl ? 443 : 80)) ? ':'.$appPort : ''; - $defaultUrl = ($ssl ? 'https' : 'http').'://'.$host.$suffix; - if (preg_match('/^APP_URL=(.*)$/m', $env, $m)) { - $url = trim($m[1], "\"'"); - // Correct a URL for localhost or for the chosen host (scheme and port may - // have changed); leave a genuinely custom one alone. - if (preg_match('#^https?://(localhost|'.preg_quote($host, '#').')(:\d+)?/?$#', $url)) { - $env = preg_replace('/^APP_URL=.*$/m', "APP_URL={$defaultUrl}", $env); - } - } else { - $env .= "\nAPP_URL={$defaultUrl}"; + if ($url === null || preg_match('#^https?://(localhost|'.preg_quote($host, '#').')(:\d+)?/?$#', $url)) { + $set('APP_URL', ($ssl ? 'https' : 'http').'://'.$host.$suffix); } - // Set missing or blank values; respect anything the user has already configured + // Set missing or blank values; respect anything the user has already configured. $defaults = [ 'DB_HOST' => 'mysql', 'DB_PORT' => '3306', @@ -872,19 +870,15 @@ protected function applyDockerEnvDefaults(string $env, bool $ssl = true, array $ ]; foreach ($defaults as $key => $value) { - if (preg_match('/^'.preg_quote($key, '/').'=(.*)$/m', $env, $m)) { - if (trim($m[1]) === '') { - $env = preg_replace('/^'.preg_quote($key, '/').'=.*$/m', "{$key}={$value}", $env); - } - } else { - $env .= "\n{$key}={$value}"; + if (($read($key) ?? '') === '') { + $set($key, $value); } } - // Port overrides are forced, not defaulted: the value already there is the - // one that clashed. + // Port overrides are forced, not defaulted: the value already there is the one + // that clashed. foreach ($ports as $key => $port) { - $env = InstallCommand::setEnvLine($env, $key, (string) $port); + $set($key, (string) $port); } return $env; diff --git a/tests/Feature/Environments/DockerEnvironmentTest.php b/tests/Feature/Environments/DockerEnvironmentTest.php index 611f228..b133f7a 100644 --- a/tests/Feature/Environments/DockerEnvironmentTest.php +++ b/tests/Feature/Environments/DockerEnvironmentTest.php @@ -593,7 +593,8 @@ public function test_replaces_sqlite_connection_with_mysql(): void public function test_leaves_mysql_connection_unchanged(): void { - $input = "APP_URL=https://localhost\nDB_CONNECTION=mysql\nDB_HOST=mysql\nDB_PORT=3306\nDB_DATABASE=myapp\nDB_USERNAME=myapp\nDB_PASSWORD=secret\nMAIL_MAILER=smtp\n"; + // An already-correct Docker .env must come back byte-identical. + $input = "APP_URL=https://localhost\nDB_CONNECTION=mysql\nDB_HOST=mysql\nDB_PORT=3306\nDB_DATABASE=myapp\nDB_USERNAME=myapp\nDB_PASSWORD=secret\nMAIL_MAILER=smtp\nMAIL_HOST=mailpit\nMAIL_PORT=1025\n"; $result = $this->applyDefaults($input); $this->assertSame($input, $result); @@ -676,6 +677,41 @@ public function test_appends_mail_mailer_when_missing(): void $this->assertStringContainsString('MAIL_MAILER=smtp', $result); } + public function test_points_mail_at_the_mailpit_container(): void + { + // Compose interpolates ${MAIL_HOST} from .env, so "localhost" here reaches the + // app container verbatim — where it means the app itself, not Mailpit. + $result = $this->applyDefaults("MAIL_MAILER=smtp\nMAIL_HOST=localhost\nMAIL_PORT=1025\n"); + + $this->assertStringContainsString('MAIL_HOST=mailpit', $result); + $this->assertStringNotContainsString('MAIL_HOST=localhost', $result); + } + + public function test_replaces_the_laravel_default_mail_host_and_port(): void + { + $result = $this->applyDefaults("MAIL_HOST=127.0.0.1\nMAIL_PORT=2525\n"); + + $this->assertStringContainsString('MAIL_HOST=mailpit', $result); + // 1025 is Mailpit's container-internal port, not the published one. + $this->assertStringContainsString('MAIL_PORT=1025', $result); + } + + public function test_appends_mail_host_when_absent(): void + { + $result = $this->applyDefaults("APP_NAME=Test\n"); + + $this->assertStringContainsString('MAIL_HOST=mailpit', $result); + $this->assertStringContainsString('MAIL_PORT=1025', $result); + } + + public function test_leaves_a_real_smtp_host_alone(): void + { + $result = $this->applyDefaults("MAIL_HOST=smtp.mailtrap.io\nMAIL_PORT=2525\n"); + + $this->assertStringContainsString('MAIL_HOST=smtp.mailtrap.io', $result); + $this->assertStringContainsString('MAIL_PORT=2525', $result); + } + public function test_sets_https_url_when_ssl_enabled(): void { $result = $this->applyDefaults("APP_URL=http://localhost\n", ssl: true); @@ -831,6 +867,36 @@ public function test_a_url_for_an_unrelated_host_is_still_left_alone(): void $this->assertStringContainsString('APP_URL=https://staging.example.com', $result); } + public function test_comments_and_blank_lines_survive_a_full_pass(): void + { + // .env is a hand-edited file: the "# DB_HOST=..." hints tell the user what is + // configurable. This is why the writer is line-based rather than a parse of the + // file into an array and back. + $input = implode("\n", [ + '# Application', + 'APP_SLUG=acme', + '', + '# DB_DATABASE=${APP_SLUG}', + 'DB_CONNECTION=sqlite', + '', + ]); + + $result = $this->applyDefaults($input, ssl: true, ports: ['APP_HTTPS_PORT' => 8443]); + + $this->assertStringContainsString('# Application', $result); + $this->assertStringContainsString('# DB_DATABASE=${APP_SLUG}', $result); + $this->assertStringContainsString('DB_CONNECTION=mysql', $result); + } + + public function test_a_commented_key_is_not_mistaken_for_a_real_one(): void + { + // "# DB_HOST=localhost" must not read as DB_HOST being set. + $result = $this->applyDefaults("# DB_HOST=localhost\nDB_CONNECTION=sqlite\n"); + + $this->assertStringContainsString('DB_HOST=mysql', $result); + $this->assertStringContainsString('# DB_HOST=localhost', $result); + } + public function test_real_env_example_pattern_produces_valid_docker_env(): void { $input = implode("\n", [ diff --git a/tests/Feature/InstallCommandModulesTest.php b/tests/Feature/InstallCommandModulesTest.php new file mode 100644 index 0000000..35ba664 --- /dev/null +++ b/tests/Feature/InstallCommandModulesTest.php @@ -0,0 +1,97 @@ +dir = sys_get_temp_dir().'/sb-modules-'.uniqid(); + mkdir($this->dir, 0755, true); + } + + protected function tearDown(): void + { + @unlink($this->dir.'/composer.lock'); + @rmdir($this->dir); + parent::tearDown(); + } + + /** @param string[] $locked */ + private function command(array $locked, bool $writeLock = true): TestableInstallCommand + { + if ($writeLock) { + file_put_contents($this->dir.'/composer.lock', json_encode([ + 'packages' => array_map(fn (string $name) => ['name' => $name], $locked), + 'packages-dev' => [['name' => 'laravel/boost']], + ])); + } + + $command = new class extends TestableInstallCommand + { + /** @var string[] */ + public array $messages = []; + + public function warn($string, $verbosity = null): void + { + $this->messages[] = $string; + } + + public function line($string, $style = null, $verbosity = null): void + { + $this->messages[] = $string; + } + }; + $command->fakeOptions = ['path' => $this->dir]; + + return $command; + } + + public function test_a_failed_post_install_script_does_not_fail_the_module_install(): void + { + $command = $this->command(['saucebase/auth', 'saucebase/settings']); + + $this->assertTrue($command->composerRequireSucceeded(false, ['saucebase/auth', 'saucebase/settings'])); + $this->assertStringContainsString('boost:install', implode("\n", $command->messages)); + } + + public function test_a_genuinely_missing_package_still_fails(): void + { + $command = $this->command(['saucebase/auth']); + + $this->assertFalse($command->composerRequireSucceeded(false, ['saucebase/auth', 'saucebase/never-resolved'])); + } + + public function test_a_clean_exit_is_taken_at_face_value_without_reading_the_lock(): void + { + $command = $this->command([], writeLock: false); + + $this->assertTrue($command->composerRequireSucceeded(true, ['saucebase/auth'])); + $this->assertSame([], $command->messages, 'a clean run must stay quiet'); + } + + public function test_a_missing_or_unreadable_lock_file_is_treated_as_failure(): void + { + $command = $this->command([], writeLock: false); + + $this->assertFalse($command->composerRequireSucceeded(false, ['saucebase/auth'])); + } + + public function test_package_names_are_matched_case_insensitively(): void + { + $command = $this->command(['saucebase/Auth']); + + $this->assertTrue($command->composerRequireSucceeded(false, ['Saucebase/auth'])); + } +}