Skip to content

Dependency Injection migration: Service Provider and first migration#9108

Open
MissAllSunday wants to merge 9 commits intoSimpleMachines:release-3.0from
MissAllSunday:di_first_migration
Open

Dependency Injection migration: Service Provider and first migration#9108
MissAllSunday wants to merge 9 commits intoSimpleMachines:release-3.0from
MissAllSunday:di_first_migration

Conversation

@MissAllSunday
Copy link
Contributor

@MissAllSunday MissAllSunday commented Feb 10, 2026

This PR adds a way to register services as dependencies and migrates ErrorHandler to be served via the Container

Heres a Graph TB for reference on how ErrorHandler is been used and how backward compatibility is achieved by leaving the Original ErrorHandler as a Facade and mark it as deprecated in favor of the new Service. I'm aware original ErrorHandler has some duplicated code for calling the container, however, this is done to maximize backward compatibility.

graph TB
    subgraph "Legacy Code"
        A[Legacy Code Calls]
        A1["ErrorHandler::log()"]
        A2["ErrorHandler::fatal()"]
        A3["ErrorHandler::catch()"]
        A4["new ErrorHandler()"]
    end
    
    subgraph "Facade Layer"
        B[ErrorHandler Class]
        B1[getService Method]
        B2[Static Methods]
    end
    
    subgraph "DI Container"
        C[Container::getInstance]
        D[ServiceProvider]
        E[ServicesList.php]
    end
    
    subgraph "Service Layer"
        F[ErrorHandlerService]
        F1[handleError]
        F2[log]
        F3[fatal]
        F4[catch]
    end
    
    A --> B
    A1 --> B2
    A2 --> B2
    A3 --> B2
    A4 --> B
    
    B --> B1
    B2 --> B1
    
    B1 -->|Try Container First| C
    C --> D
    D --> E
    E -->|Returns Shared Instance| F
    
    B1 -->|Fallback if Container Unavailable| F
    
    F --> F1
    F --> F2
    F --> F3
    F --> F4
    
    style B fill:#e1f5ff
    style F fill:#fff4e1
    style C fill:#e8f5e9
    style A fill:#fce4ec
Loading

Of course arch and folders/naming convention should be subject to modifications to better suit current patterns if needed.

@dragomano
Copy link
Contributor

Examples of two different container implementations to demonstrate just how powerful this thing is. Both versions are fully working:

Copy link
Member

@Sesquipedalian Sesquipedalian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to fixing the noted minor nit, please run PHP-CS-Fixer on all modified and added files in order to make them compliant with the SMF code standard.

(It appears that our CI script that is supposed to run PHP-CS-Fixer as a linter is not working right now. If it were, this issue would have been flagged automatically.)

index.php Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
\SMF\Infrastructure\Container::init();
SMF\Infrastructure\Container::init();

nit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I applied the change. As for the lint, unfortunately, it seems composer lint has been setup to work with a git diff command to get the staged changes and since I already committed them the linter no longer sees them.

Running php-cs-fixer without the git diff constrain yields more than 1k files needed to be corrected so perhaps it would be a good idea to create a new branch just for linting and start fresh.

That or change the instruction to git diff release-3.0 to make sure the linter works on all changes against the latest release-3.0 commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to manually execute php-cs-fixer on the modified files of this PR but it seems there are some discrepancies between SMF needed PHP version (8.0) and what the linter needs (8.1)

When I tried to force run the linter on php 8 via docker:

docker run --rm -it -v "$(pwd)":/app -w /app php:8.0-cli sh -c "
    apt-get update -qq && apt-get install -y git unzip -qq > /dev/null && \
    curl -sS https://getcomposer.org/installer | php -- --install-dir=/usr/local/bin --filename=composer -q && \
    git config --global --add safe.directory /app && \
    composer install --no-interaction --quiet && \
    ./vendor/bin/php-cs-fixer fix --config=.php-cs-fixer.dist.php --allow-risky=yes --show-progress=dots -vvv \
    Sources/Infrastructure/Container.php

I get the following error:

[Error]                                                  
        Undefined constant "SMF\Fixer\ClassNotation\T_READONLY" 

Which indicates some of the rules for php-cs-fixer are too new for the PHP version SMF needs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments