Feature/yaml service config #27
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughReplaces hardcoded PHP-DI definitions with YAML-driven service configuration, adds YamlDefinitionLoader, several PSR-11 invokable factories, updates Container bootstrap (env-aware, accepts SettingManager), adds extensive services.yaml, docs, and unit tests; also updates composer dependencies for routing, YAML parsing, and testing tools. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Bootstrap as src/Bootstrap.php
participant Container as Neuron\Cms\Container\Container::build
participant Loader as YamlDefinitionLoader
participant PHPDI as PHP-DI\Container
participant Adapter as ContainerAdapter (IContainer)
participant Registry as Registry
note over Bootstrap,Container: Application startup / automatic bootstrap
Bootstrap->>Container: call build(SettingManager, environment?)
Container->>Loader: new Loader(configPath, environment) -> load()
Loader->>Loader: read services.yaml (+ env override) -> parse -> convert to PHP‑DI definitions
Loader-->>Container: definitions array
Container->>PHPDI: addDefinitions(definitions + SettingManager definition) -> build()
PHPDI-->>Container: PSR‑11 container
Container->>Adapter: wrap PHP‑DI container
Container->>Registry: store Adapter for backward compatibility
Registry-->>Bootstrap: container available for application components
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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 |
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/DEPENDENCY_INJECTION.md (1)
322-322: Remove hardcoded local filesystem path.The absolute path
/Users/lee/projects/personal/neuron/cms/examples/bootstrap-with-container.phpis specific to a developer's local machine and should not be committed to documentation.🔎 Proposed fix
-See `/Users/lee/projects/personal/neuron/cms/examples/bootstrap-with-container.php` for complete example. +See `examples/bootstrap-with-container.php` for complete example.
🧹 Nitpick comments (10)
src/Cms/Container/Factories/PasswordHasherFactory.php (1)
27-38: Consider usingconfigure()method for more comprehensive password configuration.The factory currently only applies
min_length, butPasswordHasherhas aconfigure(array $settings)method that can handle multiple password requirements (min_length,require_uppercase,require_lowercase,require_numbers,require_special_chars). Additionally, catching generic\Exceptionis too broad.🔎 Proposed refactor to use configure() method
try { - $minLength = $settings->get( 'password', 'min_length' ); - if( $minLength ) + $passwordSettings = $settings->get( 'password' ); + if( $passwordSettings && is_array( $passwordSettings ) ) { - $hasher->setMinLength( (int)$minLength ); + $hasher->configure( $passwordSettings ); } } - catch( \Exception $e ) + catch( \RuntimeException | \InvalidArgumentException $e ) { // Use defaults }src/Cms/Container/Factories/SessionManagerFactory.php (1)
27-38: Narrow exception handling scope.Catching generic
\Exceptionis too broad. Consider catching more specific exceptions thatSettingManager::get()might throw.🔎 Proposed fix
try { $lifetime = $settings->get( 'session', 'lifetime' ); if( $lifetime ) { $config['lifetime'] = (int)$lifetime; } } - catch( \Exception $e ) + catch( \RuntimeException | \InvalidArgumentException $e ) { // Use defaults if settings not found }docs/SERVICE_CONFIGURATION.md (2)
183-188: Add language specifier to fenced code block.The directory tree structure should specify a language identifier for proper markdown rendering.
🔎 Proposed fix
-``` +```text resources/config/ ├── services.yaml # Base configuration ├── services.testing.yaml # Testing overrides └── services.production.yaml # Production overrides</details> Based on learnings, static analysis tools flagged this issue. --- `190-190`: **Use heading instead of bold text for better document structure.** Using a heading provides better semantic structure and navigation in the documentation. <details> <summary>🔎 Proposed fix</summary> ```diff -**Example: services.testing.yaml** +### Example: services.testing.yamlBased on learnings, static analysis tools flagged this issue.
src/Cms/Container/Factories/RegistrationServiceFactory.php (1)
27-30: Reorder dependency retrieval to match constructor parameter order.Dependencies are retrieved in order:
userRepository,emailVerifier,passwordHasher,settings, but theRegistrationServiceconstructor expects:userRepository,passwordHasher,emailVerifier,settings. While not a bug, this inconsistency reduces readability. Other factories in this PR (EmailVerifierFactory, PasswordResetterFactory) retrieve dependencies in the same order as constructor usage.🔎 Proposed refactor for consistency
$userRepository = $container->get( IUserRepository::class ); - $emailVerifier = $container->get( EmailVerifier::class ); $passwordHasher = $container->get( PasswordHasher::class ); + $emailVerifier = $container->get( EmailVerifier::class ); $settings = $container->get( SettingManager::class );src/Cms/Container/README.md (2)
7-7: Add language identifiers to fenced code blocks.Fenced code blocks should specify a language for proper syntax highlighting. Add language identifiers to the code blocks at lines 7 and 172.
🔎 Proposed fix
For line 7:
-``` +```text Container SystemFor line 172:
-``` +```text 1. Bootstrap (src/Bootstrap.php)Also applies to: 172-172
243-243: Use proper heading syntax instead of emphasis.Lines 243, 268, and 288 use bold emphasis for section titles. Convert these to proper Markdown headings for better document structure.
🔎 Proposed fix
-**Total: 52 unique services, 97 total bindings** +### Total: 52 unique services, 97 total bindings -**Step 1: Create factory** +#### Step 1: Create factory -**Step 2: Register in YAML** +#### Step 2: Register in YAMLAlso applies to: 268-268, 288-288
src/Cms/Container/Factories/PasswordResetterFactory.php (1)
26-62: Factory correctly constructs PasswordResetter with optional token expiration.The factory follows the established pattern for complex service initialization. The try-catch block at lines 48-59 allows graceful degradation to default token expiration if configuration is missing or invalid.
Consider logging configuration errors to aid debugging while maintaining graceful degradation:
🔎 Optional improvement
catch( \Exception $e ) { - // Use default expiration + // Use default expiration - could not read configuration + error_log( "PasswordResetter: Could not read token_expiration config, using default: " . $e->getMessage() ); }src/Cms/Container/Container.php (2)
43-43: Consider enabling compilation for production environments.The container compilation/caching is currently commented out. Enabling this feature can significantly improve performance in production by caching the compiled container definitions.
💡 Suggestion for enabling conditional compilation
- // Enable compilation for production (caching) - // $builder->enableCompilation( __DIR__ . '/../../../var/cache/container' ); + // Enable compilation for production (caching) + if( $environment === 'production' ) + { + $builder->enableCompilation( __DIR__ . '/../../../var/cache/container' ); + }
46-46: Hardcoded configuration path may reduce flexibility.The configuration path uses a relative path from
__DIR__. While this works, consider making it configurable to support different project structures or allow easier testing with custom config locations.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
composer.jsondocs/DEPENDENCY_INJECTION.mddocs/SERVICE_CONFIGURATION.mdresources/config/services.yamlsrc/Cms/Container/Container.phpsrc/Cms/Container/Factories/EmailVerifierFactory.phpsrc/Cms/Container/Factories/PasswordHasherFactory.phpsrc/Cms/Container/Factories/PasswordResetterFactory.phpsrc/Cms/Container/Factories/RegistrationServiceFactory.phpsrc/Cms/Container/Factories/SessionManagerFactory.phpsrc/Cms/Container/README.mdsrc/Cms/Container/YamlDefinitionLoader.phptests/Unit/Cms/Container/YamlDefinitionLoaderTest.php
🧰 Additional context used
🧬 Code graph analysis (7)
src/Cms/Container/Factories/RegistrationServiceFactory.php (7)
src/Cms/Services/Member/RegistrationService.php (1)
RegistrationService(24-321)src/Cms/Auth/PasswordHasher.php (1)
PasswordHasher(13-204)src/Cms/Services/Auth/EmailVerifier.php (1)
EmailVerifier(24-257)src/Cms/Container/Factories/EmailVerifierFactory.php (1)
__invoke(25-43)src/Cms/Container/Factories/PasswordHasherFactory.php (1)
__invoke(22-41)src/Cms/Container/Factories/PasswordResetterFactory.php (1)
__invoke(26-62)src/Cms/Container/Factories/SessionManagerFactory.php (1)
__invoke(22-41)
src/Cms/Container/Factories/SessionManagerFactory.php (6)
src/Cms/Container/Container.php (1)
Container(18-87)src/Cms/Auth/SessionManager.php (1)
SessionManager(13-216)src/Cms/Container/Factories/EmailVerifierFactory.php (1)
__invoke(25-43)src/Cms/Container/Factories/PasswordHasherFactory.php (1)
__invoke(22-41)src/Cms/Container/Factories/PasswordResetterFactory.php (1)
__invoke(26-62)src/Cms/Container/Factories/RegistrationServiceFactory.php (1)
__invoke(25-38)
src/Cms/Container/Factories/PasswordResetterFactory.php (5)
src/Cms/Services/Auth/PasswordResetter.php (1)
PasswordResetter(23-244)src/Cms/Auth/PasswordHasher.php (1)
PasswordHasher(13-204)src/Cms/Container/Factories/EmailVerifierFactory.php (1)
__invoke(25-43)src/Cms/Container/Factories/PasswordHasherFactory.php (1)
__invoke(22-41)src/Cms/Container/Factories/SessionManagerFactory.php (1)
__invoke(22-41)
src/Cms/Container/Container.php (1)
src/Cms/Container/YamlDefinitionLoader.php (2)
YamlDefinitionLoader(17-281)load(43-64)
tests/Unit/Cms/Container/YamlDefinitionLoaderTest.php (1)
src/Cms/Container/YamlDefinitionLoader.php (3)
YamlDefinitionLoader(17-281)load(43-64)getDefinitions(277-280)
src/Cms/Container/Factories/EmailVerifierFactory.php (1)
src/Cms/Services/Auth/EmailVerifier.php (1)
EmailVerifier(24-257)
src/Cms/Container/Factories/PasswordHasherFactory.php (1)
src/Cms/Auth/PasswordHasher.php (2)
PasswordHasher(13-204)setMinLength(128-132)
🪛 markdownlint-cli2 (0.18.1)
src/Cms/Container/README.md
7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
172-172: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
243-243: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
268-268: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
288-288: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
docs/SERVICE_CONFIGURATION.md
183-183: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
190-190: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ 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 (postgres)
- GitHub Check: build-test (mysql)
- GitHub Check: build-test (sqlite)
🔇 Additional comments (16)
tests/Unit/Cms/Container/YamlDefinitionLoaderTest.php (1)
1-377: LGTM! Comprehensive test coverage for YamlDefinitionLoader.The test suite provides excellent coverage:
- Basic loading and all definition types (autowire, create, factory, alias, value, instance)
- Environment-specific overrides and base file requirements
- Error scenarios (invalid YAML, unknown types, missing required fields)
- Parameter resolution and value definitions
- Edge cases like shorthand alias syntax and instance type skipping
The setup/teardown properly manages temporary test files, and assertions are clear and appropriate.
src/Cms/Container/Factories/EmailVerifierFactory.php (1)
25-43: LGTM! Factory correctly resolves dependencies and constructs EmailVerifier.The factory properly:
- Resolves required dependencies from the container
- Handles missing configuration with sensible defaults (localhost for development)
- Constructs the verification URL from site settings
The base path fallback to
getcwd()is reasonable for development environments where Registry may not be initialized.src/Cms/Container/YamlDefinitionLoader.php (6)
29-33: LGTM! Constructor properly initializes loader configuration.The constructor correctly normalizes the config path and stores the optional environment parameter for later use in environment-specific overrides.
43-64: LGTM! Load method correctly implements base + environment override pattern.The method properly:
- Loads base services.yaml (required)
- Conditionally loads environment-specific overrides if environment is specified
- Merges environment definitions over base definitions (line 56)
- Converts to PHP-DI format and stores for later retrieval
This supports the environment-specific configuration use case described in the tests and documentation.
73-89: LGTM! Robust error handling for YAML file loading.The method properly validates file existence before parsing and wraps the YAML parser with appropriate exception handling. Error messages include the file path and parser error details for effective debugging.
97-151: LGTM! Comprehensive conversion of YAML definitions to PHP-DI format.The method correctly handles all supported definition types:
- Simple string aliases (lines 104-108)
- Autowire definitions with optional constructor parameters
- Create definitions with constructor and method calls
- Factory definitions with validation
- Alias definitions with target validation
- Instance types (skipped for runtime handling)
- Value definitions for primitive types
The switch statement provides clear error handling for unknown types, and the code delegates to focused helper methods for each type.
212-234: LGTM! Factory definition supports both static methods and __invoke pattern.The method correctly handles two factory patterns:
- Static factory methods (lines 215-221)
- Invokable factory classes (lines 224-231)
The closure at lines 227-230 properly instantiates the factory class and invokes it with the container, supporting the factory pattern used throughout this PR (EmailVerifierFactory, PasswordHasherFactory, etc.).
Error handling ensures factory_class is always specified.
260-270: LGTM! Parameter resolution correctly converts @service references.The method uses
strpos($param, '@') === 0to reliably detect service references at the start of string parameters and converts them to\DI\get()calls. Non-reference parameters pass through unchanged, supporting mixed constructor arguments (services, strings, numbers, etc.) as demonstrated in the tests.src/Cms/Container/Container.php (2)
51-51: SettingManager instance overrides YAML definition.The SettingManager is added here as a concrete instance, which will override any definition from the YAML file. This is likely intentional to ensure the passed
$settingsinstance is used, but it makes the YAML definition for SettingManager (lines 22-24 in services.yaml) redundant.Consider removing or documenting the redundant SettingManager definition in services.yaml to avoid confusion. The YAML definition declares it as
type: instance, source: registry, but this line ensures the passed parameter is used instead.
46-54: All factory classes referenced in the YAML configuration (SessionManagerFactory, PasswordHasherFactory, EmailVerifierFactory, PasswordResetterFactory, RegistrationServiceFactory) are properly implemented and follow the PHP-DI factory pattern with__invoke()methods. No issues found.resources/config/services.yaml (5)
1-16: Excellent documentation for the YAML configuration structure.The header comments clearly explain the different definition types and parameter reference syntax. This will help maintainers understand and modify the configuration.
34-40: Factory-based service creation pattern looks appropriate.Using factory classes for SessionManager and PasswordHasher is a good approach when services require complex initialization logic or need to pull configuration from multiple sources. This keeps the YAML configuration clean and maintainable.
83-117: Interface-to-implementation bindings follow DI best practices.The extensive use of interface aliases enables dependency injection by interface rather than concrete implementation, promoting loose coupling and testability. The organization and naming conventions are clear and consistent.
174-341: CRUD service definitions are well-organized and consistent.The CRUD services follow a consistent pattern across different domains (User, Post, Page, Event, etc.), with interface bindings and autowired implementations. This consistency will make the codebase easier to maintain and extend.
47-55: No issues found. The YamlDefinitionLoader correctly handles the@prefix notation in constructor parameters through theresolveParameters()method (lines 260-270), which detects strings starting with@and converts them to PHP-DI'sDI\get()format by stripping the prefix. This applies to bothautowireandcreatedefinition types. The implementation is covered by unit tests (e.g.,testCreateDefinitionWithConstructor()).composer.json (1)
31-35: Good addition of Codeception testing framework.The addition of Codeception and its modules aligns well with the PR objectives to test the new YAML-based DI configuration system. The module-db inclusion is particularly appropriate for testing container-managed database services. All specified package versions exist, are stable, and have no known security advisories. The version constraints will resolve to recent, actively maintained releases (Codeception 5.3.3, module-phpbrowser 3.0.2, module-asserts 3.2.0, module-db 3.2.2, and infection/infection 0.32.0).
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 (1)
resources/config/services.yaml
⏰ 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 (postgres)
- GitHub Check: build-test (sqlite)
🔇 Additional comments (4)
resources/config/services.yaml (4)
1-16: LGTM! Clear and comprehensive documentation.The header provides excellent guidance on definition types and parameter references, making the configuration file self-documenting and maintainable.
22-24: Good resolution of previous review feedback.The redundant SettingManager definition has been replaced with a clear comment explaining the programmatic injection. This addresses the previous review concern appropriately.
221-307: The incomplete CRUD patterns in the service definitions are intentional and safe. None of the missing service interfaces (IPageDeleter, IEventDeleter, IEventCategoryDeleter, ICategoryDeleter, ITagUpdater, ITagDeleter) are used anywhere in the codebase, so there is no risk of runtime PSR-11 container exceptions. The application design intentionally limits CRUD operations for certain entities.Likely an incorrect or invalid review comment.
46-54: No issues found. AllDatabase*Repositoryimplementations consistently requireSettingManagerin their constructors—there is no autowiring inconsistency. TheContainer::build()method correctly addsSettingManagerto the definitions array before the builder constructs the container, so no timing dependency exists.Likely an incorrect or invalid review comment.
Note
YAML-based DI config with environment overrides
Containerto load services fromresources/config/services.yamlvia newYamlDefinitionLoader(supportsautowire,create,factory,alias,value,instance) and env-specific filesSessionManagerFactory,PasswordHasherFactory,EmailVerifierFactory,PasswordResetterFactory,RegistrationServiceFactory)services.yamlreplacing previous PHP definitionsDependencies & Tooling
symfony/yamlandneuron-php/routing; expands dev tooling with Codeception modulesDocs & Tests
SERVICE_CONFIGURATION.md, containerREADME.md; updates DI guideYamlDefinitionLoaderTestcovering parsing, overrides, and error casesWritten by Cursor Bugbot for commit 45bc8c3. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.