Repository navigation
Conversation
📝 WalkthroughWalkthroughAggregates configuration from YAML, encrypted secrets, and environment variables via a multi-source SettingManager; adds encrypted-secrets support and gitignore template; introduces URL-based database configuration and parsing; updates installer to optionally persist secrets; adapts bootstrap and ConnectionFactory; adds tests and a small view form-action change. Changes
Sequence Diagram(s)sequenceDiagram
participant Bootstrap as Bootstrap
participant SMF as SettingManagerFactory
participant YAML as YAML Files
participant Secrets as Encrypted Secrets
participant Env as Environment
participant SM as SettingManager
participant App as Application
Bootstrap->>SMF: createCustom(baseYamlPath, env)
activate SMF
SMF->>YAML: Load neuron.yaml / neuron.{env}.yaml
YAML-->>SMF: base + env config
SMF->>Secrets: Load secrets.yml.enc (and secrets.{env}.yml.enc)
Secrets-->>SMF: decrypted secrets (scoped)
SMF->>Env: Read environment variables
Env-->>SMF: env values (highest priority)
rect rgb(220,235,245)
note over SMF: Merge sources (env > secrets > yaml)
end
SMF->>SM: Construct SettingManager with merged sources
SM-->>SMF: SettingManager instance
SMF-->>Bootstrap: return SettingManager
Bootstrap->>App: new Application(version, SettingManager)
App-->>Bootstrap: Application ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Bootstrap.php (1)
49-103: Undefined$settingsif configuration loading fails early.If
SettingManagerFactory::createCustom()throws an exception (e.g., missing neuron.yaml), the catch block only logs the error but doesn't return or re-throw. Execution continues to line 92 (Container::build($settings)) and line 101 ($settings->get(...)) where$settingsis undefined, causing a fatal error.Either re-throw the exception or ensure a graceful failure:
🔎 Proposed fix
catch( \Exception $e ) { \Neuron\Log\Log::error( 'Container initialization failed: ' . $e->getMessage() ); + throw $e; // Configuration is critical - cannot continue without it }Or alternatively, move the
$settingscreation outside the try block and only wrap optional operations.
🤖 Fix all issues with AI Agents
In @src/Bootstrap.php:
- Around line 73-84: The encrypted config sources are added without defensive
existence checks; update the arrays passed to createCustom() so each encrypted
source mirrors the YAML source pattern by wrapping the environment-specific
encrypted entry in a file_exists() conditional and using array_filter() to drop
nulls (i.e., replace the plain second encrypted array with
file_exists("$configPath/secrets/$environment.yml.enc") ? [
'type'=>'encrypted','path'=>"$configPath/secrets/$environment.yml.enc",'key'=>"$configPath/secrets/$environment.key",'name'=>"secrets:$environment"
] : null ), ensuring the created list only includes existing files before
calling createCustom().
In @src/Cms/Cli/Commands/Install/InstallCommand.php:
- Around line 1050-1054: Remove the unused temporary variable $testConfig in
InstallCommand.php: locate the try block where $testConfig is assigned (the
variable name $testConfig) and delete that assignment since it is never used;
ensure no other logic depends on $testConfig and run tests/static analysis to
confirm the warning is resolved.
- Around line 978-984: The email public config is reading $email['transport']
while configureEmail() sets the key as 'driver', causing transport to always
default; update the public config assembly in InstallCommand (the block setting
$publicConfig['email']) to read $email['driver'] (or otherwise normalize $email
keys from configureEmail()) and ensure the keys emitted remain non-sensitive
(keep from_address and from_name) so the 'driver' value is preserved instead of
always falling back to 'mail'.
In @src/Cms/Database/ConnectionFactory.php:
- Around line 184-192: The code assigns raw parsed['user'] and parsed['pass']
into $config without URL-decoding, so percent-encoded credentials can fail;
update the assignment in ConnectionFactory (the block handling $parsed['user']
and $parsed['pass']) to pass values through urldecode() (e.g., $config['user'] =
urldecode($parsed['user']); and $config['pass'] = urldecode($parsed['pass']);)
so credentials like p%40ssword decode correctly.
- Around line 138-152: The SQLite path handling currently checks for '//' before
'///', causing paths like '///storage/db.sqlite3' to match the wrong branch and
strip only two slashes; update the logic in ConnectionFactory (the block that
examines $path) to check for the '///' prefix first and strip three characters
for that case (substr($path, 3)), and only then handle the '//' case by
stripping two characters (substr($path, 2)), so absolute triple-slash paths are
normalized correctly.
In @tests/Unit/Cms/Database/ConnectionFactoryTest.php:
- Around line 211-222: Remove the unused private helper method
assertDoesNotThrow from the test class: locate the private function
assertDoesNotThrow(callable $callable): void in the test file and delete the
entire method (including its try/catch body) to eliminate dead code and satisfy
static analysis. Confirm no other tests reference assertDoesNotThrow before
removal.
- Around line 92-107: The test currently only checks SettingManager values;
modify it to call ConnectionFactory::createFromConfig with the $config produced
from SettingManager (after creating $source = new Memory() and setting the URL
plus overrides) and then assert the resulting connection's configuration (or
DSN) reflects the overrides: that the connection created by
ConnectionFactory::createFromConfig has host == 'overridehost' and port == 9999;
use the ConnectionFactory::createFromConfig return value or its accessible
methods/properties to inspect the resolved host/port and assert equality.
🧹 Nitpick comments (5)
tests/Unit/Cms/Controllers/Member/RegistrationTest.php (1)
51-79: Consider consolidating duplicate mock callbacks.The
mockSettingSourcecallback (lines 53-63) duplicates the logic inmockSettings.get()callback (lines 66-76). While this works correctly, you could simplify by reusing the same callback function or referencing one from the other to reduce duplication.🔎 Proposed refactor to reduce duplication
+ // Create a shared callback for settings values + $settingsCallback = function( $section, $key = null ) { + if( $section === 'site' && $key === 'name' ) return 'Test Site'; + if( $section === 'site' && $key === 'title' ) return 'Test Site'; + if( $section === 'site' && $key === 'description' ) return 'Test Description'; + if( $section === 'site' && $key === 'url' ) return 'https://test.com'; + if( $section === 'site' && $key === 'theme' ) return 'flatly'; + if( $section === 'views' && $key === 'path' ) return __DIR__ . '/../../../../../resources/views'; + if( $section === 'cache' && $key === 'enabled' ) return false; + if( $section === 'member' && $key === 'require_email_verification' ) return true; + return null; + }; + // Create a mock setting source for the SettingManager $mockSettingSource = $this->createMock( \Neuron\Data\Settings\Source\ISettingSource::class ); - $mockSettingSource->method( 'get' )->willReturnCallback( function( $section, $key = null ) { - if( $section === 'site' && $key === 'name' ) return 'Test Site'; - if( $section === 'site' && $key === 'title' ) return 'Test Site'; - if( $section === 'site' && $key === 'description' ) return 'Test Description'; - if( $section === 'site' && $key === 'url' ) return 'https://test.com'; - if( $section === 'site' && $key === 'theme' ) return 'flatly'; - if( $section === 'views' && $key === 'path' ) return __DIR__ . '/../../../../../resources/views'; - if( $section === 'cache' && $key === 'enabled' ) return false; - if( $section === 'member' && $key === 'require_email_verification' ) return true; - return null; - }); + $mockSettingSource->method( 'get' )->willReturnCallback( $settingsCallback ); // Setup default mock settings - $this->mockSettings->method( 'get' )->willReturnCallback( function( $section, $key = null ) { - if( $section === 'site' && $key === 'name' ) return 'Test Site'; - if( $section === 'site' && $key === 'title' ) return 'Test Site'; - if( $section === 'site' && $key === 'description' ) return 'Test Description'; - if( $section === 'site' && $key === 'url' ) return 'https://test.com'; - if( $section === 'site' && $key === 'theme' ) return 'flatly'; - if( $section === 'views' && $key === 'path' ) return __DIR__ . '/../../../../../resources/views'; - if( $section === 'cache' && $key === 'enabled' ) return false; - if( $section === 'member' && $key === 'require_email_verification' ) return true; - return null; - }); + $this->mockSettings->method( 'get' )->willReturnCallback( $settingsCallback ); // IMPORTANT: Mock the getSource() method to return the mock setting source $this->mockSettings->method( 'getSource' )->willReturn( $mockSettingSource );resources/config/.gitignore.template (1)
1-4: Consider the scope of the*.keypattern.The pattern
*.keyon line 4 will ignore all.keyfiles in the config directory and subdirectories. If there are any legitimate non-secret.keyfiles (e.g., public keys for verification) that should be tracked, this pattern might be too broad.Consider being more specific if needed, such as:
/master.key /secrets/*.key /encryption*.keyHowever, if all
.keyfiles in this directory are indeed sensitive, the current pattern is appropriate.resources/config/neuron.yaml (1)
48-50: Approve the defensive default, but consider a clearer comment.The
enabled: trueplaceholder is a reasonable approach to prevent null errors in PHP 8.4's stricter handling. However, the comment "Dummy value" might confuse future maintainers. Consider rephrasing to clarify its purpose:# Security Configuration security: - enabled: true # Dummy value to prevent null errors in PHP 8.4 + enabled: true # Placeholder to ensure security section is not null (required for PHP 8.4+ compatibility)tests/Unit/Cms/Database/ConnectionFactoryTest.php (1)
77-86: Consider using try/finally for temp file cleanup.If
ConnectionFactory::createFromSettingsthrows an exception, the temp file won't be cleaned up. Using try/finally ensures cleanup even on failure:🔎 Proposed fix
// Test with absolute path $tempFile = tempnam( sys_get_temp_dir(), 'test_db_' ) . '.sqlite3'; -$settings = $this->createSettingsWithUrl( 'sqlite:///' . $tempFile ); -$pdo = ConnectionFactory::createFromSettings( $settings ); -$this->assertInstanceOf( PDO::class, $pdo ); - -// Clean up -if( file_exists( $tempFile ) ) -{ - unlink( $tempFile ); -} +try +{ + $settings = $this->createSettingsWithUrl( 'sqlite:///' . $tempFile ); + $pdo = ConnectionFactory::createFromSettings( $settings ); + $this->assertInstanceOf( PDO::class, $pdo ); +} +finally +{ + if( file_exists( $tempFile ) ) + { + unlink( $tempFile ); + } +}src/Cms/Cli/Commands/Install/InstallCommand.php (1)
943-948: Consider splitting multiple statements for readability.Multiple statements on one line reduce readability and make debugging harder:
🔎 Proposed fix
// Put username and password in secrets if( isset( $db['user'] ) || isset( $db['pass'] ) ) { $secretsConfig['database'] = []; - if( isset( $db['user'] ) ) $secretsConfig['database']['user'] = $db['user']; - if( isset( $db['pass'] ) ) $secretsConfig['database']['pass'] = $db['pass']; + if( isset( $db['user'] ) ) + { + $secretsConfig['database']['user'] = $db['user']; + } + if( isset( $db['pass'] ) ) + { + $secretsConfig['database']['pass'] = $db['pass']; + } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
resources/config/.gitignore.templateresources/config/neuron.yamlresources/config/neuron.yaml.exampleresources/views/auth/password_reset/forgot-password.phpsrc/Bootstrap.phpsrc/Cms/Cli/Commands/Install/InstallCommand.phpsrc/Cms/Database/ConnectionFactory.phptests/Unit/Cms/Controllers/Admin/PostsTest.phptests/Unit/Cms/Controllers/Auth/PasswordResetTest.phptests/Unit/Cms/Controllers/Member/RegistrationTest.phptests/Unit/Cms/Database/ConnectionFactoryTest.php
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Unit/Cms/Database/ConnectionFactoryTest.php (1)
src/Cms/Database/ConnectionFactory.php (2)
ConnectionFactory(17-280)createFromSettings(26-36)
🪛 PHPMD (2.15.0)
tests/Unit/Cms/Database/ConnectionFactoryTest.php
211-222: Avoid unused private methods such as 'assertDoesNotThrow'. (undefined)
(UnusedPrivateMethod)
src/Cms/Cli/Commands/Install/InstallCommand.php
1053-1053: Avoid unused local variables such as '$testConfig'. (undefined)
(UnusedLocalVariable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Cursor Bugbot
- GitHub Check: build-test (sqlite)
- GitHub Check: build-test (postgres)
- GitHub Check: build-test (mysql)
🔇 Additional comments (9)
tests/Unit/Cms/Controllers/Admin/PostsTest.php (1)
39-44: LGTM! Test setup properly updated for multi-source configuration.The addition of
ISettingSourcemocking and wiring it throughSettingManager.getSource()aligns with the PR's refactoring of the configuration pipeline to support multiple sources. This ensures tests have a controlled configuration surface and properly mock the new dependency chain.tests/Unit/Cms/Controllers/Auth/PasswordResetTest.php (1)
52-57: LGTM! Consistent test setup for multi-source configuration.The mock setup for
ISettingSourceand its integration withSettingManageris consistent with the pattern used in other test files (e.g.,PostsTest.php). This properly supports the new configuration pipeline introduced in the PR.resources/config/.gitignore.template (1)
1-23: Well-structured gitignore template for security and local overrides.The template comprehensively covers:
- Encryption keys (critical for security)
- Local environment overrides
- Environment-specific configurations
- Temporary files
- Optional IDE settings
The organization with clear section comments makes it easy to understand and maintain.
resources/views/auth/password_reset/forgot-password.php (1)
9-9: No action needed — the route is properly registered and configured.The new
/forgot-passwordroute is correctly registered:
- GET route at
#[Get('/forgot-password', name: 'forgot_password')]displays the form- POST route at
#[Post('/forgot-password', name: 'forgot_password_post', filters: ['csrf'])]processes the form with CSRF protection- The handler validates form data through a DTO and properly redirects on success or error
- No remaining references to the old route
/auth/password-reset/sendexist in the codebaseresources/config/neuron.yaml.example (2)
101-121: LGTM! Well-documented URL-based configuration.The documentation clearly explains the URL format, provides examples for all supported database adapters, documents environment variable precedence, and shows how to mix URL with individual overrides. This aligns well with the
ConnectionFactory::parseUrlimplementation.
127-132: LGTM!The SQLite URL format example is correctly placed and follows the standard
sqlite:///pathconvention.src/Cms/Database/ConnectionFactory.php (1)
51-60: LGTM! URL merge logic is correct.The merge correctly gives precedence to explicitly provided config values over URL-parsed values by placing
$urlConfigfirst inarray_mergeand filtering out nulls and the URL key itself from the override.src/Cms/Cli/Commands/Install/InstallCommand.php (2)
1252-1331: LGTM! Solid encrypted secrets implementation.The
saveConfigWithSecretsmethod properly:
- Generates a master key if it doesn't exist
- Uses
SecretManagerfor encryption- Provides clear security instructions to the user
- Updates
.gitignoreto prevent accidental key commits
1336-1392: LGTM! Proper .gitignore handling.The method correctly:
- Avoids duplicate entries by checking existing patterns
- Creates both root and config-level
.gitignorefiles- Includes appropriate patterns for keys and local env files
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Bootstrap.phpsrc/Cms/Cli/Commands/Install/InstallCommand.phpsrc/Cms/Database/ConnectionFactory.phptests/Unit/Cms/Database/ConnectionFactoryTest.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/Bootstrap.php (1)
src/Cms/Container/Container.php (2)
Container(18-87)build(33-66)
src/Cms/Cli/Commands/Install/InstallCommand.php (2)
src/Cms/Email/helpers.php (1)
src/Cms/Maintenance/MaintenanceManager.php (1)
getMessage(171-176)
🪛 PHPMD (2.15.0)
tests/Unit/Cms/Database/ConnectionFactoryTest.php
244-255: Avoid unused private methods such as 'assertDoesNotThrow'. (undefined)
(UnusedPrivateMethod)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Cursor Bugbot
- GitHub Check: build-test (mysql)
- GitHub Check: build-test (sqlite)
- GitHub Check: build-test (postgres)
🔇 Additional comments (8)
src/Bootstrap.php (2)
45-50: Bootstrap settings/container initialization is now robustInitializing
$settings/$containerup-front and constructing a minimalSettingManagerin the catch block cleanly avoids the previous undefined-variable fatal and still allows the app to boot with basic YAML config when the advanced multi-source setup fails. The subsequent application construction and base-path resolution from settings/env look consistent with the new configuration model.Also applies to: 95-107, 108-113
59-76: Multi-source configuration wiring looks consistent with new secrets flowThe
SettingManagerFactory::createCustom()call correctly layers base YAML, optional environment-specific YAML, encrypted secrets (global and environment-specific), and environment variables, matching the installer and docs story. The testing-only override forneuron.testing.yamlis a reasonable special case and the default toneuron.yamlfor other environments keeps behavior predictable.Also applies to: 77-93
src/Cms/Cli/Commands/Install/InstallCommand.php (3)
61-72: use-secrets option and plumbing into setupDatabase/saveCompleteConfig look goodThe new
--use-secrets/-sflag is correctly parsed once inexecute(), passed only intosetupDatabase(), and then threaded intosaveCompleteConfig(). The conditional branch in the installation loop that special-casessetupDatabasewhile leaving other steps untouched keeps the execution model simple and backward compatible.Also applies to: 78-82, 124-143, 542-603
337-365: Gitignore template and secrets/key handling are safe and idempotent
copyGitignoreTemplate()defensively createsconfig/.gitignorefrom the component template only when missing, andupdateGitignore()extends the root.gitignoreplus creates a minimalconfig/.gitignoreif needed. Combined withsaveConfigWithSecrets()generatingmaster.keyandsecrets.yml.enc, this gives a reasonable out-of-the-box security posture without overwriting existing ignore rules.Also applies to: 372-374, 1264-1343, 1348-1404
1041-1090: configureDatabaseUrl keeps installer UX simple and defers strict validation to ConnectionFactoryThe URL-based database configuration prompt clearly communicates expected formats, stores only the URL under
database.url, and leaves detailed validation/parsing toConnectionFactory::parseUrl(). That separation of concerns keeps the installer logic straightforward while leveraging the central factory for scheme-specific handling.src/Cms/Database/ConnectionFactory.php (2)
49-61: URL merge behavior and DSN construction align with the new config model
createFromConfig()’s pattern of parsingurlfirst, then overlaying non-null explicit config values (excludingurlitself) gives a clear precedence model while reusing existing DSN construction for sqlite/mysql/pgsql. The requirement thatnamebe non-empty across adapters, and the reuse of the same initialization path after URL merging, keeps behavior predictable for both legacy and URL-based configurations.Also applies to: 62-73, 74-91, 93-107
121-155: parseUrl correctly handles sqlite, scheme mapping, query params, and encoded credentialsThe private
parseUrl()helper:
- Handles sqlite URLs (including
:memory:and triple-slash absolute paths) without relying onparse_url.- Maps
mysqlandpostgresql/postgres/pgsqlschemes to the existing adapter names.- Extracts host/port/user/pass/name and decodes user/pass via
rawurldecode, which is important for special characters in credentials.- Recognizes common query parameters like
charsetfor mysql.This matches the installer’s documented URL formats and the new unit tests’ expectations.
Also applies to: 157-220
tests/Unit/Cms/Database/ConnectionFactoryTest.php (1)
20-88: Good coverage of URL parsing, error paths, and sqlite specificsThe tests exercise mysql/pgsql/sqlite URL parsing via reflection, malformed/unsupported schemes, query parameters (charset), encoded credentials, and sqlite-specific formats (including in-memory and triple-slash absolute paths). The backward-compatibility check for non-URL sqlite configs ensures existing configurations remain supported.
Also applies to: 112-163, 168-213, 218-229
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
tests/Unit/Cms/Database/ConnectionFactoryUrlSecretsTest.php (2)
67-86: Clarify the test intent or add exception expectations.This test demonstrates a broken configuration scenario but doesn't validate that it actually fails. The comment on line 84-85 mentions it "would cause" an error, but the test doesn't assert this. Consider either:
- Adding
$this->expectException()and attempting to callConnectionFactory::createFromConfig($config)to verify the error is thrown, or- Renaming the test to indicate it's documenting the broken state without testing runtime behavior (e.g.,
testBrokenConfigurationStructure)♻️ Option to add exception validation
public function testBrokenConfigurationWithPlaceholderAdapter(): void { $source = new Memory(); // This simulates the BROKEN configuration that would cause the error $source->set( 'database', 'url', 'mysql://user:pass@localhost:3306/testdb' ); $source->set( 'database', 'adapter', 'configured' ); // This would break it! $settings = new SettingManager( $source ); $config = $settings->getSection( 'database' ); // Both keys would be present $this->assertArrayHasKey( 'url', $config ); $this->assertArrayHasKey( 'adapter', $config ); // The adapter would be the broken placeholder $this->assertEquals( 'configured', $config['adapter'] ); - // This configuration would cause "Unsupported database adapter: configured" error - // because the placeholder would override the URL-parsed adapter + // Verify this configuration causes the expected error + $this->expectException( \Exception::class ); + $this->expectExceptionMessage( 'Unsupported database adapter: configured' ); + ConnectionFactory::createFromConfig( $config ); }
91-116: Consider testing the public API instead of replicating internal logic.This test duplicates the internal merging logic from
ConnectionFactory::createFromConfig(lines 104-109), creating tight coupling between test and implementation. If the merging algorithm changes but produces the same results, this test would need updates.Consider refactoring to test the public
createFromConfigmethod directly with various input combinations and verify the resulting PDO connection properties, rather than reimplementing the merge logic in the test.♻️ Alternative approach testing the public API
public function testCreateFromConfigMergingLogic(): void { - // Test the exact merging that happens in ConnectionFactory::createFromConfig - $config = [ 'url' => 'postgresql://user:pass@host:5432/dbname' ]; - - $reflection = new \ReflectionClass( ConnectionFactory::class ); - $parseMethod = $reflection->getMethod( 'parseUrl' ); - $parseMethod->setAccessible( true ); - - // Parse URL - $urlConfig = $parseMethod->invokeArgs( null, [ $config['url'] ] ); - - // Simulate the merge that happens in createFromConfig - $mergedConfig = array_merge( - $urlConfig, - array_filter( $config, function( $value, $key ) { - return $key !== 'url' && $value !== null; - }, ARRAY_FILTER_USE_BOTH ) - ); - - // The adapter from URL should be preserved (no override) - $this->assertEquals( 'pgsql', $mergedConfig['adapter'] ); - $this->assertEquals( 'host', $mergedConfig['host'] ); - $this->assertEquals( 5432, $mergedConfig['port'] ); - $this->assertEquals( 'dbname', $mergedConfig['name'] ); + // Test that URL-based config creates a working PDO with correct driver + $config = [ 'url' => 'sqlite::memory:' ]; + + $pdo = ConnectionFactory::createFromConfig( $config ); + + $this->assertInstanceOf( \PDO::class, $pdo ); + $this->assertEquals( 'sqlite', $pdo->getAttribute( \PDO::ATTR_DRIVER_NAME ) ); }Note: This is just one approach. The existing URL parsing validation is already covered by
ConnectionFactoryUrlSecretsTest::testUrlFromSecretsWithoutAdapterConflict.tests/Unit/Cms/Database/NoConfiguredAdapterTest.php (1)
18-44: Simplify the test by removing the try-catch block.The try-catch pattern (lines 32-43) is unnecessary and makes the test harder to understand. Since the goal is to verify that the configuration succeeds without throwing the "configured" adapter error, simply call the method and assert success. If an unexpected exception is thrown, PHPUnit will automatically fail the test.
♻️ Simplified test structure
public function testNoConfiguredAdapterError(): void { // Simulate the EXACT scenario: URL in secrets, nothing in public $publicSource = new Memory(); // Public has NO database section at all $secretsSource = new Memory(); $secretsSource->set( 'database', 'url', 'sqlite::memory:' ); // Merge like the app does $settings = new SettingManager( $publicSource ); $settings->addSource( $secretsSource ); - // This should NOT throw "Unsupported database adapter: configured" - try { - $pdo = ConnectionFactory::createFromSettings( $settings ); - $this->assertNotNull( $pdo ); - $this->assertEquals( 'sqlite', $pdo->getAttribute( \PDO::ATTR_DRIVER_NAME ) ); - } catch ( \Exception $e ) { - // If we get here, check it's NOT the "configured" error - $this->assertStringNotContainsString( - 'Unsupported database adapter: configured', - $e->getMessage(), - 'Should not get "configured" adapter error' - ); - } + // This should succeed without throwing any exception + $pdo = ConnectionFactory::createFromSettings( $settings ); + $this->assertNotNull( $pdo ); + $this->assertEquals( 'sqlite', $pdo->getAttribute( \PDO::ATTR_DRIVER_NAME ) ); }tests/Unit/Cms/Cli/Commands/Install/DatabaseUrlValidationTest.php (1)
15-31: Consider whether testingparse_urlbehavior adds value.This test method primarily validates PHP's standard
parse_urlfunction behavior rather than application-specific logic. Sinceparse_urlis a well-tested standard library function, these assertions may not provide significant value. Consider focusing tests on how your application uses and validates URLs throughConnectionFactory::parseUrlinstead.tests/Unit/Cms/Database/ConnectionFactoryTest.php (1)
111-165: Remove unused variable and unreachable catch block.The static analysis correctly identifies that
$createMethod(line 114) is assigned but never used. Additionally, thecatch (\PDOException $e)block (lines 162-165) is unreachable since no PDO connection is attempted within the try block.♻️ Proposed cleanup
// Now test that createFromConfig properly applies the overrides // We'll use a mock PDO to test the DSN construction without actual connection - try { - // Call createFromConfig through reflection to catch the DSN - $reflection = new \ReflectionClass( ConnectionFactory::class ); - $createMethod = $reflection->getMethod( 'createFromConfig' ); - - // Instead of creating actual PDO, let's verify the merge logic - // by inspecting what createFromConfig would do - $parseMethod = $reflection->getMethod( 'parseUrl' ); - $parseMethod->setAccessible( true ); + // Instead of creating actual PDO, let's verify the merge logic + // by inspecting what createFromConfig would do + $reflection = new \ReflectionClass( ConnectionFactory::class ); + $parseMethod = $reflection->getMethod( 'parseUrl' ); + $parseMethod->setAccessible( true ); - // Parse the URL to get base config - $urlConfig = $parseMethod->invokeArgs( null, [ $config['url'] ] ); + // Parse the URL to get base config + $urlConfig = $parseMethod->invokeArgs( null, [ $config['url'] ] ); - // Apply the same merge logic that createFromConfig uses - $finalConfig = array_merge( - $urlConfig, - array_filter( $config, function( $value, $key ) { - return $key !== 'url' && $value !== null; - }, ARRAY_FILTER_USE_BOTH ) - ); + // Apply the same merge logic that createFromConfig uses + $finalConfig = array_merge( + $urlConfig, + array_filter( $config, function( $value, $key ) { + return $key !== 'url' && $value !== null; + }, ARRAY_FILTER_USE_BOTH ) + ); - // Verify that overrides were applied - $this->assertEquals( 'overridehost', $finalConfig['host'], 'Host should be overridden' ); - $this->assertEquals( 9999, $finalConfig['port'], 'Port should be overridden' ); + // Verify that overrides were applied + $this->assertEquals( 'overridehost', $finalConfig['host'], 'Host should be overridden' ); + $this->assertEquals( 9999, $finalConfig['port'], 'Port should be overridden' ); - // Verify other values from URL are preserved - $this->assertEquals( 'user', $finalConfig['user'], 'User from URL should be preserved' ); - $this->assertEquals( 'pass', $finalConfig['pass'], 'Password from URL should be preserved' ); - $this->assertEquals( 'urldb', $finalConfig['name'], 'Database name from URL should be preserved' ); - $this->assertEquals( 'mysql', $finalConfig['adapter'], 'Adapter from URL should be preserved' ); + // Verify other values from URL are preserved + $this->assertEquals( 'user', $finalConfig['user'], 'User from URL should be preserved' ); + $this->assertEquals( 'pass', $finalConfig['pass'], 'Password from URL should be preserved' ); + $this->assertEquals( 'urldb', $finalConfig['name'], 'Database name from URL should be preserved' ); + $this->assertEquals( 'mysql', $finalConfig['adapter'], 'Adapter from URL should be preserved' ); - // Verify the DSN that would be constructed uses the overrides - $expectedDsn = sprintf( - "mysql:host=%s;port=%s;dbname=%s;charset=%s", - 'overridehost', // Override host - 9999, // Override port - 'urldb', // From URL - 'utf8mb4' // Default charset - ); + // Verify the DSN that would be constructed uses the overrides + $expectedDsn = sprintf( + "mysql:host=%s;port=%s;dbname=%s;charset=%s", + 'overridehost', // Override host + 9999, // Override port + 'urldb', // From URL + 'utf8mb4' // Default charset + ); - // Build actual DSN using same logic as ConnectionFactory - $actualDsn = sprintf( - "mysql:host=%s;port=%s;dbname=%s;charset=%s", - $finalConfig['host'] ?? 'localhost', - $finalConfig['port'] ?? 3306, - $finalConfig['name'], - $finalConfig['charset'] ?? 'utf8mb4' - ); + // Build actual DSN using same logic as ConnectionFactory + $actualDsn = sprintf( + "mysql:host=%s;port=%s;dbname=%s;charset=%s", + $finalConfig['host'] ?? 'localhost', + $finalConfig['port'] ?? 3306, + $finalConfig['name'], + $finalConfig['charset'] ?? 'utf8mb4' + ); - $this->assertEquals( $expectedDsn, $actualDsn, 'DSN should use overridden host and port' ); - - } catch ( \PDOException $e ) { - // If PDO fails (no actual MySQL), that's expected - we're testing config merge logic - $this->markTestSkipped( 'Cannot test actual PDO connection without MySQL server' ); - } + $this->assertEquals( $expectedDsn, $actualDsn, 'DSN should use overridden host and port' ); }src/Cms/Cli/Commands/Install/InstallCommand.php (2)
1304-1307: Consider security implications of displaying master key.The encryption key is output to the console (line 1306). While helpful for initial setup, this could pose risks if terminal output is logged or screen is shared. Since the key is already saved to
config/master.key, consider making the console output optional or requiring an explicit flag.♻️ Suggested approach
$this->output->success( "Generated master key: {$keyPath}" ); $this->output->warning( "IMPORTANT: Add this file to .gitignore - NEVER commit it!" ); - $this->output->info( "Key value (save this securely): {$key}" ); + $this->output->info( "Key saved to: {$keyPath}" ); + $this->output->info( "To view the key: cat {$keyPath}" ); $this->output->writeln( "" );
1296-1322: Minor: SecretManager instance reuse opportunity.Two separate
SecretManagerinstances are created (lines 1301 and 1317). If the class is stateless, the same instance could be reused. This is a minor optimization.♻️ Suggested optimization
+ $secretManager = new SecretManager(); + // Generate master key if it doesn't exist if( !file_exists( $keyPath ) ) { $this->output->writeln( "\nGenerating encryption key..." ); - $secretManager = new SecretManager(); $key = $secretManager->generateKey( $keyPath ); // ... rest of key generation } // Save encrypted secrets if( !empty( $secretsConfig ) ) { $this->output->writeln( "Encrypting sensitive configuration..." ); - $secretManager = new SecretManager(); $yamlContent = YamlParser::dump( $secretsConfig, 4, 2 ); // ... rest of encryption }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Bootstrap.phpsrc/Cms/Cli/Commands/Install/InstallCommand.phptests/Unit/Cms/Cli/Commands/Install/DatabaseUrlValidationTest.phptests/Unit/Cms/Database/ConnectionFactoryIntegrationTest.phptests/Unit/Cms/Database/ConnectionFactoryTest.phptests/Unit/Cms/Database/ConnectionFactoryUrlSecretsTest.phptests/Unit/Cms/Database/NoConfiguredAdapterTest.php
🧰 Additional context used
🧬 Code graph analysis (4)
tests/Unit/Cms/Database/NoConfiguredAdapterTest.php (1)
src/Cms/Database/ConnectionFactory.php (2)
ConnectionFactory(17-284)createFromSettings(26-36)
tests/Unit/Cms/Database/ConnectionFactoryIntegrationTest.php (1)
src/Cms/Database/ConnectionFactory.php (2)
ConnectionFactory(17-284)createFromSettings(26-36)
tests/Unit/Cms/Database/ConnectionFactoryUrlSecretsTest.php (1)
src/Cms/Database/ConnectionFactory.php (1)
ConnectionFactory(17-284)
tests/Unit/Cms/Database/ConnectionFactoryTest.php (1)
src/Cms/Database/ConnectionFactory.php (2)
ConnectionFactory(17-284)createFromSettings(26-36)
🪛 PHPMD (2.15.0)
tests/Unit/Cms/Database/ConnectionFactoryTest.php
114-114: Avoid unused local variables such as '$createMethod'. (undefined)
(UnusedLocalVariable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Cursor Bugbot
- GitHub Check: build-test (sqlite)
- GitHub Check: build-test (mysql)
- GitHub Check: build-test (postgres)
🔇 Additional comments (12)
tests/Unit/Cms/Database/ConnectionFactoryUrlSecretsTest.php (1)
21-61: LGTM! Well-structured test for URL-from-secrets scenario.The test correctly simulates the runtime behavior with separate public and secrets sources, and appropriately uses reflection to verify the internal URL parsing logic. The assertions comprehensively validate all expected URL components.
tests/Unit/Cms/Database/ConnectionFactoryIntegrationTest.php (1)
19-88: LGTM! Comprehensive integration test coverage.The test suite effectively covers the main configuration scenarios:
- URL from secrets (with driver verification)
- Mixed public and secrets configuration
- Backward compatibility with traditional non-URL config
- URL-only configuration
All tests use SQLite in-memory databases, which is appropriate for integration testing without external dependencies.
tests/Unit/Cms/Database/NoConfiguredAdapterTest.php (1)
49-79: LGTM! Clear test cases for broken and fixed scenarios.Lines 49-62 properly use
expectExceptionto validate the broken scenario, and lines 67-79 provide a clean positive test case for the URL-only configuration.tests/Unit/Cms/Cli/Commands/Install/DatabaseUrlValidationTest.php (2)
107-160: LGTM! Good coverage of URL requirement validation.This test method appropriately validates application-specific requirements for different database URL types, ensuring that MySQL/PostgreSQL URLs include necessary components (host and database) while handling SQLite's special URL format.
36-102: The assertion at line 58 is correct.parse_url('sqlite:///path/to/database.db')returnsfalsein PHP 8.2.29. There is no evidence of version-dependent behavior across supported PHP versions, and the test accurately reflects the actual behavior of PHP'sparse_urlfunction. No changes needed.Likely an incorrect or invalid review comment.
src/Bootstrap.php (3)
47-111: LGTM! Undefined variable issue resolved with proper fallback.The initialization of
$settingstonull(line 48) combined with the fallback creation in the catch block (lines 109-110) ensures$settingsis never undefined when accessed at line 115. This addresses the critical issue raised in past review comments while maintaining application resilience.
78-98: LGTM! Encrypted sources now include defensive file existence checks.The encrypted source configurations (lines 78-90) now properly check for file existence before inclusion, matching the defensive pattern recommended in past review comments. The use of ternary operators with
nullfollowed byarray_filter(line 98) cleanly handles optional encrypted sources.
69-74: Environment configuration uses base YAML for development and production.The environment-specific YAML selection only provides an override for
testing(lines 70-72), whiledevelopmentandproductionenvironments use the baseneuron.yaml. This appears intentional given the PR's approach of using environment-specific encrypted secrets for environment differentiation rather than separate config files.tests/Unit/Cms/Database/ConnectionFactoryTest.php (1)
1-36: Good test structure for URL parsing.The test file provides comprehensive coverage for the new URL-based database configuration feature. The use of reflection to test the private
parseUrlmethod is appropriate for unit testing internal logic.src/Cms/Cli/Commands/Install/InstallCommand.php (3)
923-933: URL+secrets handling now correctly avoids adapter conflict.The previous implementation set
adapter => 'configured'in public config which conflicted with URL-parsed adapters. This has been fixed - when using URL configuration with secrets, only the URL is stored in secrets with no placeholder in public config.
1062-1103: URL validation improvements address previous concerns.The URL validation now properly handles:
- SQLite URLs with special
sqlite:prefix handling- Malformed URLs where
parse_urlreturns null/false for scheme- Unsupported database schemes with clear error messages
- Missing required components (host, database name) for MySQL/PostgreSQL
991-1018: Email secrets separation correctly implemented.The
driverkey mismatch from previous review has been addressed. Non-sensitive SMTP settings (host, port, encryption) are kept in public config while credentials (username, password) are properly moved to secrets.
1 participant
Note
Introduces secure, flexible configuration and improves bootstrapping.
ConnectionFactorynow supportsdatabase.urlparsing/overrides (SQLite/MySQL/PostgreSQL) and merges with individual params--use-secretsmode encrypts sensitive settings (master.key,secrets.yml.enc), saves public vs secrets config, validates DB URLs, and provisionsconfig/.gitignore; adds DB URL setup flowSettingManagerFactoryto load base YAML, environment-specific files, encrypted secrets, and env vars; constructsApplicationdirectly and initializes ORM viaConnectionFactorysecurity.enabled: true; expandsneuron.yaml.examplewith DB URL examples and precedence notes/forgot-passwordWritten by Cursor Bugbot for commit 5bbbdd6. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.