-
Notifications
You must be signed in to change notification settings - Fork 268
[3.0] POC Dependency injection proposal #9092
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
MissAllSunday
wants to merge
5
commits into
SimpleMachines:release-3.0
Choose a base branch
from
MissAllSunday:Dependency-injection-proposal
base: release-3.0
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
c1cfb83
feat: POC for using dependency injection using the help action as exa…
MissAllSunday 7ce5f63
feat: POC create a very basic DB service to serve as an example on ho…
MissAllSunday 7977b0e
chore: move container init to inside closure
MissAllSunday b18068e
feat: Add guide for migration
MissAllSunday 40c26a6
chore: remove metrics
MissAllSunday File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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('<meta...')` | | ||
| | `Utils::$context['template_layers'][]` | `TemplateContextServiceInterface` | `addLayer()` | `$this->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'] .= '<meta name="description" content="...">'; | ||
| ``` | ||
|
|
||
| **After:** | ||
| ```php | ||
| $this->assets->addJavaScriptFile('script.js'); | ||
| $this->assets->addCssFile('style.css'); | ||
| $this->assets->addHtmlHeader('<meta name="description" content="...">'); | ||
| ``` | ||
|
|
||
| ## 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 | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
|
|
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just be sure not to complicated this further in respect to eventually moving to PSR 7. All PSR 7 implementations provide a Url component. We should work to move in that direction whenever possible. Possibly simply by making sure that we are matching signatures with existing interfaces/behavior when possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, as the other comments suggest, this is the initial step, once we have a clear picture of all the global vars used and moved to a more manageable state, we can now determinate which ones of those could be more suitable as something else. Certainly the scripturl and other uris are nice candidates to move to a proper component class (even the league has a nice package already) but we cannot go there without first moving all global state to dependencies.