diff --git a/DEPENDENCY_INJECTION_MIGRATION_PLAN.md b/DEPENDENCY_INJECTION_MIGRATION_PLAN.md new file mode 100644 index 0000000000..c8942e4f84 --- /dev/null +++ b/DEPENDENCY_INJECTION_MIGRATION_PLAN.md @@ -0,0 +1,767 @@ +# Dependency Injection Migration Plan for SMF3 + +## Executive Summary + +This document outlines a comprehensive, gradual migration strategy to move SMF3's most-used static variables and global state into proper services using Dependency Injection (DI) with the already-included `League\Container` package. + +**Current State Analysis:** +- `Utils::$context` - **5,733 usages** (highest priority) +- `Config::$modSettings` - **2,270 usages** (high priority) +- `Config::$sourcedir` and other path variables - **~30 usages** (medium priority) +- `Utils::$smcFunc` - **8 usages** (low priority, already being phased out) + +**Migration Benefits:** +- Improved testability through dependency injection +- Better code organization and separation of concerns +- Reduced global state and hidden dependencies +- Easier to track data flow and debug issues +- Maintains backward compatibility during transition + +--- + +## Phase 1: Foundation & High-Impact Services + +### 1.1 ConfigService - Wrapping Config Static Properties + +**Current Usage:** +```php +Config::$sourcedir +Config::$boarddir +Config::$packagesdir +Config::$vendordir +Config::$languagesdir +Config::$cachedir +Config::$scripturl +Config::$boardurl +Config::$db_prefix +Config::$db_type +// ... and many more +``` + +**Proposed Service Interface:** +```php +namespace SMF\Services; + +interface ConfigServiceInterface +{ + // Path getters + public function getSourceDir(): string; + public function getBoardDir(): string; + public function getPackagesDir(): string; + public function getVendorDir(): string; + public function getLanguagesDir(): string; + public function getCacheDir(): string; + + // URL getters + public function getScriptUrl(): string; + public function getBoardUrl(): string; + + // Database config + public function getDbPrefix(): string; + public function getDbType(): string; + public function getDbServer(): string; + public function getDbName(): string; + + // Cache config + public function getCacheAccelerator(): string; + public function getCacheEnable(): int; + + // Maintenance + public function getMaintenanceMode(): int; + public function isInMaintenance(): bool; +} +``` + +**Implementation Strategy:** +- Create `ConfigService` class implementing `ConfigServiceInterface` +- Initially, methods will simply return `Config::$property` values +- Register as shared service in `Container::init()` +- Gradually refactor code to inject `ConfigServiceInterface` instead of accessing `Config::` directly + +**Priority:** Medium (30 usages, but foundational for other services) + +--- + +### 1.2 ModSettingsService - Wrapping Config::$modSettings + +**Current Usage:** +```php +Config::$modSettings['setting_name'] +Config::$modSettings['cache_enable'] +Config::$modSettings['registration_method'] +// ... 2,270+ usages throughout codebase +``` + +**Proposed Service Interface:** +```php +namespace SMF\Services; + +interface ModSettingsServiceInterface +{ + // Generic getter + public function get(string $key, mixed $default = null): mixed; + + // Type-safe getters for common settings + public function getString(string $key, string $default = ''): string; + public function getInt(string $key, int $default = 0): int; + public function getBool(string $key, bool $default = false): bool; + public function getArray(string $key, array $default = []): array; + + // Check existence + public function has(string $key): bool; + + // Setter (for admin operations) + public function set(string $key, mixed $value): void; + + // Bulk operations + public function getAll(): array; + public function setMultiple(array $settings): void; +} +``` + + + + +**Implementation Strategy:** +- Create `ModSettingsService` implementing `ModSettingsServiceInterface` +- Initially wraps `Config::$modSettings` array access +- Register as shared service in `Container::init()` + +**Priority:** HIGH (2,270 usages) + +--- + +### 1.3 ContextService - Wrapping Utils::$context + +**Current Usage:** 5,733+ usages throughout codebase + +**Proposed Service Breakdown:** + +#### AssetService - JavaScript/CSS Management +```php +interface AssetServiceInterface { + public function addJavaScriptFile(string $file, array $options = []): void; + public function addCssFile(string $file, array $options = []): void; + public function addHtmlHeader(string $header): void; +} +``` + +#### PageContextService - Page Metadata +```php +interface PageContextServiceInterface { + public function getPageTitle(): string; + public function setPageTitle(string $title): void; + public function getCurrentAction(): string; + public function getLinktree(): array; +} +``` + +**Priority:** HIGHEST (5,733 usages) + +--- + +## Phase 2: Supporting Services + +### 2.1 PathService +```php +interface PathServiceInterface { + public function getSourcePath(string $relativePath = ''): string; + public function getBoardPath(string $relativePath = ''): string; + public function getCachePath(string $relativePath = ''): string; +} +``` + +### 2.2 UrlService +```php +interface UrlServiceInterface { + public function action(string $action, array $params = []): string; + public function profile(int $userId): string; + public function topic(int $topicId): string; +} +``` + +--- + +## Complete Global/Static Variables Inventory + +### High Priority (Immediate Migration Candidates) + +| Variable | Current Location | Usage Count | Proposed Service | Priority | +|----------|-----------------|-------------|------------------|----------| +| `$context` | `Utils::$context` | 5,733 | `PageContextService`, `AssetService`, `TemplateContextService` | HIGHEST | +| `$modSettings` | `Config::$modSettings` | 2,270 | `ModSettingsService` | HIGH | +| `$sourcedir` | `Config::$sourcedir` | ~30 | `PathService` | MEDIUM | +| `$boarddir` | `Config::$boarddir` | ~25 | `PathService` | MEDIUM | +| `$scripturl` | `Config::$scripturl` | ~200 | `UrlService` | MEDIUM-HIGH | +| `$boardurl` | `Config::$boardurl` | ~50 | `UrlService` | MEDIUM | + +### Medium Priority + +| Variable | Current Location | Proposed Service | Notes | +|----------|-----------------|------------------|-------| +| `$cachedir` | `Config::$cachedir` | `PathService` | Cache path management | +| `$packagesdir` | `Config::$packagesdir` | `PathService` | Package management | +| `$languagesdir` | `Config::$languagesdir` | `PathService` | Language files | +| `$db_prefix` | `Config::$db_prefix` | `ConfigService` | Database config | +| `$db_type` | `Config::$db_type` | `ConfigService` | Database config | +| `$maintenance` | `Config::$maintenance` | `ConfigService` | Site status | + +### Low Priority (Backward Compatibility Layer) + +| Variable | Current Location | Status | Notes | +|----------|-----------------|--------|-------| +| `$smcFunc` | `Utils::$smcFunc` | 8 usages | Already being phased out | +| `$user_info` | `User::$me` | Aliased | Use `User::$me` directly | +| `$user_profile` | `User::$profiles` | Aliased | Use `User::$profiles` directly | +| `$txt` | `Lang::$txt` | Active | Consider `LangService` in future | + +--- + +## Implementation Roadmap + +### Stage 1: Foundation +- [ ] Create service interfaces in `Sources/Services/` +- [ ] Implement `ConfigService` +- [ ] Implement `ModSettingsService` +- [ ] Update `Container::init()` to register new services +- [ ] Create service provider pattern for cleaner registration + +### Stage 2: Context Breakdown +- [ ] Implement `AssetService` +- [ ] Implement `PageContextService` +- [ ] Implement `TemplateContextService` +- [ ] Create facade layer in `Utils::$context` to delegate to services +- [ ] Update 2-3 high-traffic Actions to use DI + +### Stage 3: Path & URL Services +- [ ] Implement `PathService` +- [ ] Implement `UrlService` +- [ ] Refactor file inclusion patterns +- [ ] Refactor URL generation patterns + +### Stage 4: Gradual Migration +- [ ] Migrate Admin actions (high value, contained scope) +- [ ] Migrate Profile actions +- [ ] Migrate Board/Topic actions +- [ ] Update templates to use service-provided data + +### Stage 5: Testing & Refinement +- [ ] Comprehensive unit tests for all services +- [ ] Integration tests for DI container +- [ ] Performance benchmarking +- [ ] Documentation updates + +--- + + +## Technical Implementation Guide + +### Container Configuration + +**Current State** (`Sources/Container.php`): +```php +public static function init(): LeagueContainer +{ + $container = new LeagueContainer(); + + // Enable auto-wiring + $container->delegate(new ReflectionContainer()); + + // Register core services + $container->add(DatabaseServiceInterface::class, DatabaseService::class)->setShared(true); + + self::$instance = $container; + return $container; +} +``` + +**Proposed Enhanced Configuration:** +```php +public static function init(): LeagueContainer +{ + $container = new LeagueContainer(); + + // Enable auto-wiring + $container->delegate(new ReflectionContainer()); + + // Register service providers + $container->addServiceProvider(new CoreServiceProvider()); + $container->addServiceProvider(new ContextServiceProvider()); + $container->addServiceProvider(new ConfigServiceProvider()); + + self::$instance = $container; + return $container; +} +``` + +### Service Provider Pattern + +Create `Sources/Services/Providers/CoreServiceProvider.php`: +```php +getContainer() + ->add(ConfigServiceInterface::class, ConfigService::class) + ->setShared(true); + + // ModSettings Service - Shared singleton + $this->getContainer() + ->add(ModSettingsServiceInterface::class, ModSettingsService::class) + ->setShared(true); + + // Path Service - Shared singleton + $this->getContainer() + ->add(PathServiceInterface::class, PathService::class) + ->setShared(true) + ->addArgument(ConfigServiceInterface::class); + + // URL Service - Shared singleton + $this->getContainer() + ->add(UrlServiceInterface::class, UrlService::class) + ->setShared(true) + ->addArgument(ConfigServiceInterface::class); + } +} +``` + +--- + +## Migration Patterns & Examples + +### Pattern 1: Constructor Injection (Preferred) + +**Before:** +```php +class MyAction implements ActionInterface +{ + public function execute(): void + { + $setting = Config::$modSettings['some_setting']; + $path = Config::$sourcedir . '/SomeFile.php'; + Utils::$context['page_title'] = 'My Page'; + } +} +``` + +**After:** +```php +class MyAction implements ActionInterface +{ + public function __construct( + private ModSettingsServiceInterface $modSettings, + private PathServiceInterface $paths, + private PageContextServiceInterface $pageContext + ) {} + + public function execute(): void + { + $setting = $this->modSettings->get('some_setting'); + $path = $this->paths->getSourcePath('SomeFile.php'); + $this->pageContext->setPageTitle('My Page'); + } +} +``` + +### Pattern 2: Container Resolution for Legacy Code + +**Helper Function** (`Sources/Services/helpers.php`): +```php +get(ModSettingsServiceInterface::class); + + if ($key === null) { + return $service; + } + + return $service->get($key, $default); + } +} + +if (!function_exists('config')) { + function config(?string $key = null): mixed + { + $service = Container::getInstance()->get(ConfigServiceInterface::class); + + if ($key === null) { + return $service; + } + + return match($key) { + 'sourcedir' => $service->getSourceDir(), + 'boarddir' => $service->getBoardDir(), + 'scripturl' => $service->getScriptUrl(), + default => null + }; + } +} +``` + +**Usage in Legacy Code:** +```php +// Old way (still works during transition) +$setting = Config::$modSettings['some_setting']; + +// New helper function way +$setting = modSettings('some_setting'); + +// Or get the service +$modSettings = modSettings(); +$setting = $modSettings->get('some_setting'); +``` + +--- + +## Example Service Implementations + +### ModSettingsService Implementation + +`Sources/Services/ModSettingsService.php`: +```php +settings = &Config::$modSettings; + } + + public function get(string $key, mixed $default = null): mixed + { + return $this->settings[$key] ?? $default; + } + + public function getString(string $key, string $default = ''): string + { + return (string) ($this->settings[$key] ?? $default); + } + + public function getInt(string $key, int $default = 0): int + { + return (int) ($this->settings[$key] ?? $default); + } + + public function getBool(string $key, bool $default = false): bool + { + return !empty($this->settings[$key]); + } + + + public function getArray(string $key, array $default = []): array + { + $value = $this->settings[$key] ?? $default; + return is_array($value) ? $value : $default; + } + + public function has(string $key): bool + { + return isset($this->settings[$key]); + } + + public function set(string $key, mixed $value): void + { + $this->settings[$key] = $value; + } + + public function getAll(): array + { + return $this->settings; + } + + public function setMultiple(array $settings): void + { + foreach ($settings as $key => $value) { + $this->settings[$key] = $value; + } + } +} +``` + +--- + +## Backward Compatibility Strategy + +### Phase 1: Dual Access +Both old and new patterns work simultaneously: +```php +// Old way - still works +$value = Config::$modSettings['setting']; + +// New way - preferred +$value = $this->modSettings->get('setting'); +``` + +### Phase 2: Deprecation Warnings +Add deprecation notices to static access: +```php +class Config +{ + public static function __get($name) + { + if ($name === 'modSettings') { + trigger_error( + 'Direct access to Config::$modSettings is deprecated. Use ModSettingsService instead.', + E_USER_DEPRECATED + ); + return self::$modSettings; + } + } +} +``` + +### Phase 3: Migration Complete +Remove static properties, keep only service-based access. + +--- + +## Testing Strategy + +### Unit Tests for Services + +`Tests/Services/ModSettingsServiceTest.php`: +```php +service = new ModSettingsService(); + } + + public function testGetReturnsValue(): void + { + $this->service->set('test_key', 'test_value'); + $this->assertEquals('test_value', $this->service->get('test_key')); + } + + public function testGetReturnsDefault(): void + { + $this->assertEquals('default', $this->service->get('nonexistent', 'default')); + } + + public function testGetIntReturnsInteger(): void + { + $this->service->set('number', '42'); + $this->assertSame(42, $this->service->getInt('number')); + } + + public function testGetBoolReturnsBool(): void + { + $this->service->set('enabled', 1); + $this->assertTrue($this->service->getBool('enabled')); + + $this->service->set('disabled', 0); + $this->assertFalse($this->service->getBool('disabled')); + } +} +``` + +### Integration Tests + +`Tests/Integration/ContainerTest.php`: +```php +get(ModSettingsServiceInterface::class); + + $this->assertInstanceOf(ModSettingsServiceInterface::class, $service); + } + + public function testServicesAreSingletons(): void + { + $container = Container::getInstance(); + $service1 = $container->get(ModSettingsServiceInterface::class); + $service2 = $container->get(ModSettingsServiceInterface::class); + + $this->assertSame($service1, $service2); + } +} +``` + +--- + +## Performance Considerations + +### Expected Performance Impact + +The overhead of dependency injection is minimal when properly implemented: +- **Constructor injection**: Negligible overhead (services resolved once) +- **Container resolution**: Small overhead (acceptable for non-hot paths) +- **Helper functions**: Slight overhead (convenience vs performance trade-off) + +**Recommendation:** Use constructor injection for hot paths, helper functions for legacy code. + +### Optimization Tips + +1. **Lazy Loading:** Services are only instantiated when needed +2. **Shared Instances:** All services registered as `setShared(true)` +3. **Avoid Container in Loops:** Inject dependencies in constructor +4. **Cache Service References:** Don't resolve from container repeatedly + +--- + +## Migration Checklist + +### For Each Service + +- [ ] Define interface in `Sources/Services/` +- [ ] Implement service class +- [ ] Write unit tests +- [ ] Register in service provider +- [ ] Update container initialization +- [ ] Create helper functions (if needed) +- [ ] Document usage in this plan +- [ ] Migrate 2-3 example usages +- [ ] Performance benchmark +- [ ] Code review + +### For Each Migrated Component + +- [ ] Identify all static/global dependencies +- [ ] Add constructor parameters for services +- [ ] Update instantiation to use container +- [ ] Replace static calls with service calls +- [ ] Update tests to inject mocks +- [ ] Verify functionality +- [ ] Check performance impact + +--- + +## Common Pitfalls & Solutions + +### Pitfall 1: Circular Dependencies +**Problem:** ServiceA needs ServiceB, ServiceB needs ServiceA + +**Solution:** +- Refactor to remove circular dependency +- Use events/hooks for decoupling +- Introduce a third service that both depend on + +### Pitfall 2: Service Instantiation Too Early +**Problem:** Service tries to access Config before it's loaded + +**Solution:** +- Use lazy initialization in service constructor +- Defer heavy operations to first method call +- Use factory pattern for complex initialization + +### Pitfall 3: Template Access to Services +**Problem:** Templates can't use constructor injection + +**Solution:** +```php +// In Action class +public function execute(): void +{ + Utils::$context['page_title'] = $this->pageContext->getPageTitle(); + Utils::$context['services'] = [ + 'assets' => $this->assets, + 'urls' => $this->urls, + ]; +} + +// In template +echo $context['services']['urls']->action('profile', ['u' => $user_id]); +``` + +--- + +## Success Criteria + +### Code Quality Goals +- [ ] Significantly reduce static property access +- [ ] Increase test coverage +- [ ] Zero new static property additions +- [ ] All new Actions use DI + +### Performance Goals +- [ ] Minimal page load time impact +- [ ] Minimal memory usage increase +- [ ] No degradation in database query performance + +### Developer Experience Goals +- [ ] Complete onboarding documentation +- [ ] IDE autocomplete works for all services +- [ ] Clear migration examples for each pattern +- [ ] Positive feedback on new patterns + +--- + +## Conclusion + +This migration plan provides a structured, gradual approach to modernizing SMF3's architecture by: + +1. **Prioritizing high-impact changes** (Utils::$context, Config::$modSettings) +2. **Maintaining backward compatibility** throughout the transition +3. **Using proven patterns** (Service Provider, Constructor Injection) +4. **Providing clear examples** for each migration scenario +5. **Ensuring testability** with comprehensive test coverage + +The migration can be executed incrementally, with each phase building on the previous one. The end result will be a more maintainable, testable, and modern codebase that's easier to extend and debug. + +**Next Steps:** +1. Review this plan +2. Create initial service interfaces +3. Implement first service (ModSettingsService) +4. Migrate one Action as proof of concept +5. Iterate and refine based on learnings diff --git a/DI_MIGRATION_PLAN.md b/DI_MIGRATION_PLAN.md new file mode 100644 index 0000000000..5f74461dc4 --- /dev/null +++ b/DI_MIGRATION_PLAN.md @@ -0,0 +1,73 @@ +# Dependency Injection Migration Plan + +This document outlines global variables identified as candidates for refactoring into services using the dependency injection container. The goal is to reduce reliance on global state, improve testability, and enhance maintainability. + +## Identified Global Variables for Refactoring + +### 1. `$context` + +* **Current Usage:** Managed via `SMF\Utils::$context`. This is a large, mutable array used to pass data to templates and various parts of the application. +* **Reason for Refactoring:** Its global nature makes it difficult to track data flow, leads to tight coupling, and can cause unexpected side effects. It should be broken down into smaller, more focused services or view models. +* **Proposed Service(s):** + * `ContextService`: A service to manage general context data, potentially with methods for specific context areas (e.g., `getContext()->getPageTitle()`, `getContext()->getMetaTags()`). + * More granular services for specific data types currently stored in `$context` (e.g., `MenuService`, `AlertService`, `ThemeDataService`). + +### 2. `$modSettings` + +* **Current Usage:** Managed via `SMF\Config::$modSettings`. This array holds forum-wide settings loaded from the database. +* **Reason for Refactoring:** While already encapsulated in `Config` as a static property, direct access to `Config::$modSettings` still represents a global dependency. Injecting a `SettingsService` would allow for easier mocking in tests and clearer dependency management. +* **Proposed Service(s):** + * `SettingsService`: A service to provide access to forum settings, potentially with methods for specific setting categories (e.g., `getSettings()->getGeneralSetting('boardurl')`, `getSettings()->getMailSetting('webmaster_email')`). + +### 3. `$smcFunc` + +* **Current Usage:** Managed via `SMF\Utils::$smcFunc`. This is an array of backward compatibility aliases for various utility functions. +* **Reason for Refactoring:** This is explicitly a backward compatibility layer. The underlying utility functions should be directly called or encapsulated within dedicated utility services. +* **Proposed Service(s):** + * Individual utility services (e.g., `StringService` for string manipulation, `HtmlService` for HTML-related functions). The `Utils` class itself could become a service. + +### 4. `$scripturl` + +* **Current Usage:** Managed via `SMF\Config::$scripturl`. Represents the base URL for the forum's `index.php`. +* **Reason for Refactoring:** A core application URL that is globally accessible. Could be part of a `UrlService` or `ApplicationConfigService`. +* **Proposed Service(s):** + * `UrlService`: Provides methods for generating various URLs, including the base script URL. + +### 5. `$settings` (Theme-related) + +* **Current Usage:** Managed via `SMF\Theme::$current->settings`. Holds theme-specific settings. +* **Reason for Refactoring:** Similar to `$modSettings`, direct access to `Theme::$current->settings` is a global dependency. Theme settings should be accessible through a dedicated service. +* **Proposed Service(s):** + * `ThemeSettingsService`: Provides access to the current theme's settings. + +### 6. `$options` (Theme-related) + +* **Current Usage:** Managed via `SMF\Theme::$current->options`. Holds user-configurable theme options. +* **Reason for Refactoring:** Similar to `$settings`, these are globally accessible theme options. +* **Proposed Service(s):** + * `UserThemeOptionsService`: Provides access to the current user's theme options. + +### 7. `$user_info` (and related user globals like `$user_profile`, `$memberContext`, `$user_settings`) + +* **Current Usage:** Managed via `SMF\User::$me`, `SMF\User::$profiles`, `SMF\User::$loaded[$id]->formatted`. These represent the current user's data and other loaded user profiles. +* **Reason for Refactoring:** User data is fundamental and frequently accessed. Encapsulating this within a `UserService` or `CurrentUser` service would centralize access and allow for easier management of user state and permissions. +* **Proposed Service(s):** + * `UserService`: Provides methods to retrieve user data, check permissions, and manage user-related operations. + * `CurrentUser`: A specific service or a method on `UserService` to get the currently logged-in user's object. + +### 8. All other public static properties of `SMF\Config` + +* **Current Usage:** Directly accessed as `SMF\Config::$propertyName` (e.g., `$maintenance`, `$boardurl`, `$db_type`). +* **Reason for Refactoring:** These are core application configuration values. While already in a static class, injecting them through a `ConfigurationService` or specific services (e.g., `DatabaseConfigService`) would align with DI principles. +* **Proposed Service(s):** + * `ConfigurationService`: A general service to access all application configuration values. + * More specific services like `DatabaseConfigService`, `CacheConfigService`, `PathsConfigService` for related groups of settings. + +## Next Steps + +1. Prioritize the refactoring based on usage frequency and impact (e.g., `$context` and `$modSettings` are high priority). +2. Define interfaces for the proposed services to ensure loose coupling. +3. Implement the services and register them with the `League\Container` instance in `SMF\Container::init()`. +4. Replace direct static property access (`SMF\Utils::$context`, `SMF\Config::$modSettings`, etc.) with injected service calls. +5. Update relevant code to use the new services. +6. Write comprehensive tests for the new services and updated components. diff --git a/DI_MIGRATION_QUICK_REFERENCE.md b/DI_MIGRATION_QUICK_REFERENCE.md new file mode 100644 index 0000000000..7a5e054b2e --- /dev/null +++ b/DI_MIGRATION_QUICK_REFERENCE.md @@ -0,0 +1,185 @@ +# SMF3 Dependency Injection - Quick Reference Guide + +## Service Lookup Table + +Quick reference for migrating from static/global variables to services. + +### Configuration & Settings + +| Old Pattern | New Service | Method | Example | +|------------|-------------|--------|---------| +| `Config::$modSettings['key']` | `ModSettingsServiceInterface` | `get('key')` | `$this->modSettings->get('key')` | +| `Config::$sourcedir` | `PathServiceInterface` | `getSourcePath()` | `$this->paths->getSourcePath()` | +| `Config::$boarddir` | `PathServiceInterface` | `getBoardPath()` | `$this->paths->getBoardPath()` | +| `Config::$cachedir` | `PathServiceInterface` | `getCachePath()` | `$this->paths->getCachePath()` | +| `Config::$scripturl` | `UrlServiceInterface` | `getScriptUrl()` | `$this->urls->getScriptUrl()` | +| `Config::$boardurl` | `UrlServiceInterface` | `getBoardUrl()` | `$this->urls->getBoardUrl()` | + +### Context Variables + +| Old Pattern | New Service | Method | Example | +|------------|-------------|--------|---------| +| `Utils::$context['page_title']` | `PageContextServiceInterface` | `getPageTitle()` | `$this->pageContext->getPageTitle()` | +| `Utils::$context['page_title'] = 'X'` | `PageContextServiceInterface` | `setPageTitle('X')` | `$this->pageContext->setPageTitle('X')` | +| `Utils::$context['linktree']` | `PageContextServiceInterface` | `getLinktree()` | `$this->pageContext->getLinktree()` | +| `Utils::$context['current_action']` | `PageContextServiceInterface` | `getCurrentAction()` | `$this->pageContext->getCurrentAction()` | +| `Utils::$context['javascript_files'][]` | `AssetServiceInterface` | `addJavaScriptFile()` | `$this->assets->addJavaScriptFile('file.js')` | +| `Utils::$context['css_files'][]` | `AssetServiceInterface` | `addCssFile()` | `$this->assets->addCssFile('style.css')` | +| `Utils::$context['html_headers'] .=` | `AssetServiceInterface` | `addHtmlHeader()` | `$this->assets->addHtmlHeader('templateContext->addLayer('layer')` | +| `Utils::$context['sub_template']` | `TemplateContextServiceInterface` | `setSubTemplate()` | `$this->templateContext->setSubTemplate('tpl')` | + +## Common Migration Patterns + +### Pattern 1: Simple Action Migration + +**Before:** +```php +class MyAction implements ActionInterface +{ + public function execute(): void + { + $enabled = !empty(Config::$modSettings['feature_enabled']); + Utils::$context['page_title'] = 'My Page'; + } +} +``` + +**After:** +```php +class MyAction implements ActionInterface +{ + public function __construct( + private ModSettingsServiceInterface $modSettings, + private PageContextServiceInterface $pageContext + ) {} + + public function execute(): void + { + $enabled = $this->modSettings->getBool('feature_enabled'); + $this->pageContext->setPageTitle('My Page'); + } +} +``` + +### Pattern 2: File Path Construction + +**Before:** +```php +require_once Config::$sourcedir . '/SomeFile.php'; +$file = Config::$cachedir . '/data.cache'; +``` + +**After:** +```php +require_once $this->paths->getSourcePath('SomeFile.php'); +$file = $this->paths->getCachePath('data.cache'); +``` + +### Pattern 3: URL Generation + +**Before:** +```php +$url = Config::$scripturl . '?action=profile;u=' . $userId; +``` + +**After:** +```php +$url = $this->urls->action('profile', ['u' => $userId]); +// or +$url = $this->urls->profile($userId); +``` + +### Pattern 4: Asset Loading + +**Before:** +```php +Utils::$context['javascript_files'][] = 'script.js'; +Utils::$context['css_files'][] = 'style.css'; +Utils::$context['html_headers'] .= ''; +``` + +**After:** +```php +$this->assets->addJavaScriptFile('script.js'); +$this->assets->addCssFile('style.css'); +$this->assets->addHtmlHeader(''); +``` + +## Helper Functions (Transition Period) + +For legacy code that can't easily use DI: + +```php +// Get a setting +$value = modSettings('setting_name', 'default'); + +// Get the service +$modSettings = modSettings(); +$value = $modSettings->get('setting_name'); + +// Get config value +$sourcedir = config('sourcedir'); +``` + +## Service Registration + +Add to `Container::init()` or use Service Providers: + +```php +// Direct registration +$container->add(ModSettingsServiceInterface::class, ModSettingsService::class) + ->setShared(true); + +// With dependencies +$container->add(PathServiceInterface::class, PathService::class) + ->setShared(true) + ->addArgument(ConfigServiceInterface::class); +``` + +## Testing with Services + +```php +class MyActionTest extends TestCase +{ + public function testExecute(): void + { + // Create mocks + $modSettings = $this->createMock(ModSettingsServiceInterface::class); + $pageContext = $this->createMock(PageContextServiceInterface::class); + + // Set expectations + $modSettings->expects($this->once()) + ->method('getBool') + ->with('feature_enabled') + ->willReturn(true); + + $pageContext->expects($this->once()) + ->method('setPageTitle') + ->with('My Page'); + + // Test + $action = new MyAction($modSettings, $pageContext); + $action->execute(); + } +} +``` + +## Priority Order for Migration + +1. **HIGHEST**: `Utils::$context` (5,733 usages) → Break into AssetService, PageContextService, TemplateContextService +2. **HIGH**: `Config::$modSettings` (2,270 usages) → ModSettingsService +3. **MEDIUM-HIGH**: `Config::$scripturl` (~200 usages) → UrlService +4. **MEDIUM**: Path variables (~30-50 usages each) → PathService +5. **LOW**: `Utils::$smcFunc` (8 usages) → Already being phased out + +## Checklist for Each Migration + +- [ ] Identify all static/global dependencies in the class +- [ ] Add constructor parameters for required services +- [ ] Update class instantiation to use container or auto-wiring +- [ ] Replace all static calls with service method calls +- [ ] Update or create tests with mocked services +- [ ] Verify functionality works as expected +- [ ] Check for performance regressions + diff --git a/DI_MIGRATION_README.md b/DI_MIGRATION_README.md new file mode 100644 index 0000000000..62f8244e20 --- /dev/null +++ b/DI_MIGRATION_README.md @@ -0,0 +1,139 @@ +# SMF3 Dependency Injection Migration Documentation + +## 📚 Documentation Overview + +This directory contains comprehensive documentation for migrating SMF3 from static/global variables to modern Dependency Injection using League\Container. + +### Documents + +1. **[DI_MIGRATION_SUMMARY.md](DI_MIGRATION_SUMMARY.md)** - Start here! + - Executive summary + - Quick overview of the problem and solution + - High-level overview and benefits + - Best for: Quick overview + +2. **[DEPENDENCY_INJECTION_MIGRATION_PLAN.md](DEPENDENCY_INJECTION_MIGRATION_PLAN.md)** - Complete technical plan + - Detailed service interfaces and implementations + - Complete global/static variable inventory + - Phase-by-phase implementation roadmap + - Code examples and patterns + - Testing strategy + - Performance considerations + - Best for: Developers implementing the migration + +3. **[DI_MIGRATION_QUICK_REFERENCE.md](DI_MIGRATION_QUICK_REFERENCE.md)** - Daily reference guide + - Quick lookup table for common migrations + - Before/after code examples + - Service method reference + - Migration patterns + - Best for: Developers actively migrating code + +## 🎯 Quick Start + +### For Developers Starting Migration + +1. Read the [Summary](DI_MIGRATION_SUMMARY.md) to understand the "why" +2. Review the [Quick Reference](DI_MIGRATION_QUICK_REFERENCE.md) for common patterns +3. Consult the [Full Plan](DEPENDENCY_INJECTION_MIGRATION_PLAN.md) for detailed implementation + +### For Code Reviewers + +Use the [Quick Reference](DI_MIGRATION_QUICK_REFERENCE.md) to verify migrations follow the correct patterns. + +## 📊 Key Statistics + +- **Total static/global usages**: ~8,300+ +- **Highest priority**: `Utils::$context` (5,733 usages) +- **Second priority**: `Config::$modSettings` (2,270 usages) +- **Risk level**: Low (backward compatible approach) + +## 🏗️ Architecture Overview + +``` +Static Variables (Current) Services (Target) +├── Config::$modSettings → ModSettingsService +├── Config::$sourcedir → PathService +├── Config::$scripturl → UrlService +└── Utils::$context → AssetService + PageContextService + TemplateContextService +``` + +## 🚀 Migration Phases + +1. **Foundation**: Create service interfaces and core services +2. **Context Breakdown**: Split Utils::$context into focused services +3. **Supporting Services**: Path and URL services +4. **Gradual Migration**: Migrate Actions and components +5. **Testing & Refinement**: Comprehensive testing + +## 💡 Example Migration + +**Before:** +```php +class ProfileAction { + public function execute() { + $username = Config::$modSettings['default_username']; + Utils::$context['page_title'] = 'Profile'; + } +} +``` + +**After:** +```php +class ProfileAction { + public function __construct( + private ModSettingsServiceInterface $modSettings, + private PageContextServiceInterface $pageContext + ) {} + + public function execute() { + $username = $this->modSettings->getString('default_username'); + $this->pageContext->setPageTitle('Profile'); + } +} +``` + +## ✅ Benefits + +- **Testability**: Easy to mock dependencies in unit tests +- **Type Safety**: IDE autocomplete and type checking +- **Clear Dependencies**: Constructor shows what a class needs +- **Maintainability**: Changes isolated to service implementations +- **Modern Architecture**: Industry-standard dependency injection + +## 🔧 Tools & Technologies + +- **DI Container**: League\Container (already included) +- **Pattern**: Service Provider pattern +- **Injection**: Constructor injection (preferred) +- **Backward Compatibility**: Helper functions during transition + +## 🤝 Contributing + +When migrating code: +1. Follow patterns in the Quick Reference +2. Write tests for migrated code +3. Update documentation if you discover new patterns +4. Submit PRs with clear before/after examples + +## 📞 Questions? + +- Review the [Full Plan](DEPENDENCY_INJECTION_MIGRATION_PLAN.md) for detailed answers +- Check the [Quick Reference](DI_MIGRATION_QUICK_REFERENCE.md) for common scenarios +- Consult the team lead for architectural decisions + +## 🗺️ Roadmap + +- [x] Document current state and usage statistics +- [x] Design service architecture +- [x] Create comprehensive migration plan +- [ ] Implement proof of concept (ModSettingsService) +- [ ] Migrate first Action as example +- [ ] Execute gradual migration + +--- + +**Last Updated**: 2026-01-27 +**Status**: Planning Phase + diff --git a/DI_MIGRATION_SUMMARY.md b/DI_MIGRATION_SUMMARY.md new file mode 100644 index 0000000000..42a73893e8 --- /dev/null +++ b/DI_MIGRATION_SUMMARY.md @@ -0,0 +1,78 @@ +# SMF3 Dependency Injection Migration - Executive Summary + +## Overview + +This document summarizes the comprehensive plan to migrate SMF3's static and global variables to a modern Dependency Injection (DI) architecture using the already-included `League\Container` package. + +## Current State Analysis + +### Usage Statistics + +| Variable | Location | Usage Count | Impact | +|----------|----------|-------------|--------| +| `$context` | `Utils::$context` | **5,733** | Critical | +| `$modSettings` | `Config::$modSettings` | **2,270** | Critical | +| `$scripturl` | `Config::$scripturl` | ~200 | High | +| `$sourcedir` | `Config::$sourcedir` | ~30 | Medium | + +### Problems +- Hidden dependencies +- Testing difficulty +- Tight coupling +- No type safety + +## Proposed Services + +### Core Services +- **ConfigService**: Config properties +- **ModSettingsService**: Settings with type-safe methods +- **PathService**: Path management +- **UrlService**: URL generation + +### Context Services +- **AssetService**: JS/CSS/HTML headers +- **PageContextService**: Page metadata +- **TemplateContextService**: Template data + +## Example Migration + +**Before:** +```php +class MyAction { + public function execute() { + $value = Config::$modSettings['setting']; + Utils::$context['page_title'] = 'Title'; + } +} +``` + +**After:** +```php +class MyAction { + public function __construct( + private ModSettingsServiceInterface $modSettings, + private PageContextServiceInterface $pageContext + ) {} + + public function execute() { + $value = $this->modSettings->get('setting'); + $this->pageContext->setPageTitle('Title'); + } +} +``` + +## Benefits +- Easy testing with mocks +- Type safety and IDE autocomplete +- Clear dependencies +- Better maintainability +- Backward compatible + +## Documentation +- **DEPENDENCY_INJECTION_MIGRATION_PLAN.md**: Complete detailed plan +- **DI_MIGRATION_QUICK_REFERENCE.md**: Quick lookup guide +- **DI_MIGRATION_SUMMARY.md**: This summary + +## Recommendation +Proceed with proof of concept, then execute full migration. + diff --git a/Sources/Actions/Help.php b/Sources/Actions/Help.php index 122880eb68..2a04783957 100644 --- a/Sources/Actions/Help.php +++ b/Sources/Actions/Help.php @@ -22,6 +22,7 @@ use SMF\IntegrationHook; use SMF\Lang; use SMF\Routable; +use SMF\Services\DatabaseServiceInterface; use SMF\Theme; use SMF\Utils; @@ -62,6 +63,25 @@ class Help implements ActionInterface, Routable * Public methods ****************/ + /** + * Constructor. + * + * Redirect to the user help ;). + * It loads information needed for the help section. + * It is accessed by ?action=help. + * + * Uses Help template and Manual language file. + */ + public function __construct( + private DatabaseServiceInterface $db, + ) { + IntegrationHook::call('integrate_manage_help', [&self::$subactions]); + + if (!empty($_GET['sa']) && isset(self::$subactions[$_GET['sa']])) { + $this->subaction = $_GET['sa']; + } + } + public function isRestrictedGuestAccessAllowed(): bool { return true; @@ -116,27 +136,4 @@ public function index() Utils::$context['page_title'] = Lang::getTxt('manual_smf_user_help', file: 'Manual'); Utils::$context['sub_template'] = 'manual'; } - - - /****************** - * Internal methods - ******************/ - - /** - * Constructor. Protected to force instantiation via self::load(). - * - * Redirect to the user help ;). - * It loads information needed for the help section. - * It is accessed by ?action=help. - * - * Uses Help template and Manual language file. - */ - protected function __construct() - { - IntegrationHook::call('integrate_manage_help', [&self::$subactions]); - - if (!empty($_GET['sa']) && isset(self::$subactions[$_GET['sa']])) { - $this->subaction = $_GET['sa']; - } - } } diff --git a/Sources/Container.php b/Sources/Container.php new file mode 100644 index 0000000000..6ecf666355 --- /dev/null +++ b/Sources/Container.php @@ -0,0 +1,69 @@ +delegate( + new ReflectionContainer() + ); + + // Register core services, this can and should be moved to its own container service provider + // as more and more global variables are migrated to services + $container->add(DatabaseServiceInterface::class, DatabaseService::class)->setShared(true); + + self::$instance = $container; + + return $container; + } + + /** + * Gets the container instance. + * + * @return LeagueContainer + */ + public static function getInstance(): LeagueContainer + { + if (!isset(self::$instance)) { + self::init(); + } + + return self::$instance; + } +} diff --git a/Sources/Forum.php b/Sources/Forum.php index 4bb3f81fda..2078214f96 100644 --- a/Sources/Forum.php +++ b/Sources/Forum.php @@ -523,7 +523,19 @@ public static function getCurrentAction(): ?ActionInterface $current_action = self::findAction($_REQUEST['action'] ?? null); if (is_a($current_action, ActionInterface::class, true)) { - self::$current_action = \call_user_func([$current_action, 'load']); + $container = Container::getInstance(); + + try { + if ($container->has($current_action)) { + self::$current_action = $container->get($current_action); + } else { + // Fallback or auto-wire + self::$current_action = $container->get($current_action); + } + } catch (\League\Container\Exception\NotFoundException $e) { + // Fallback for classes with non-public constructors or other issues + self::$current_action = \call_user_func([$current_action, 'load']); + } } elseif (\is_callable($current_action)) { self::$current_action = Actions\GenericAction::load(); self::$current_action->setCallable($current_action); diff --git a/Sources/Services/DatabaseService.php b/Sources/Services/DatabaseService.php new file mode 100644 index 0000000000..51cb3e7af7 --- /dev/null +++ b/Sources/Services/DatabaseService.php @@ -0,0 +1,66 @@ +db = DatabaseApi::$db; + } + + /** + * Magic method to delegate calls to the underlying DatabaseApi instance. + * + * @param string $name + * @param array $arguments + * @return mixed + */ + public function __call(string $name, array $arguments) + { + return $this->db->$name(...$arguments); + } + + /** + * Get the underlying DatabaseApi instance. + * + * @return DatabaseApi + */ + public function getApi(): DatabaseApi + { + return $this->db; + } + + // Add other frequently used methods explicitly as needed or specific type hinting, otherwise __call handles them. +} diff --git a/Sources/Services/DatabaseServiceInterface.php b/Sources/Services/DatabaseServiceInterface.php new file mode 100644 index 0000000000..7f8d7501ff --- /dev/null +++ b/Sources/Services/DatabaseServiceInterface.php @@ -0,0 +1,31 @@ +