[3.0] Propose a tighter container implementation

[live627]

Caution

These code sketches are not documentation.

The code shown in these sketches is intended to illustrate an idea, approach, or possible implementation. It is not production-ready code and should not be treated as documentation for how to write code in the project.

Our PSR container implementation feels a bit lackluster to me, and I think the global static access probably shouldn't exist. It is simply too tempting to use it as a service locator from services, entities, value objects, etc.

I've mentioned several times in the past that I dislike static objects because they're essentially global variables wearing a gold chain and trying to look cool.

I'd like to redesign the implementation so that the choice of container becomes an implementation detail. Ideally, only SMF\Forum would know that we're using a container at all. The rest of SMF should depend on an SMF abstraction rather than directly on the container implementation.

For third-party modifications, I'm thinking about adding an integration hook that allows them to provide an array of callables. SMF would process those callables as service factories, passing them a small SMF\Services object.

Something along these lines:

use League\Container\Container;

$container = new Container();
$services = new Services($container);

$factories = [];

// Your services are wanted.
IntegrationHook::call('integrate_services', [&$factories]);

foreach ($factories as $name => $factory) {
    $method = ($factory['shared'] ?? false) ? 'addShared' : 'add';

    $container->$method($factory['name'], \Closure::bind($factory['callback'], $services));
}

function my_integrated_service (array $factories): void {
	$factories[] = [
		'name' => UserRepository::class,
		// The container is bound to $this for easy lookups.
		'callback' => function () {
			// Look up the DatabaseConnection inside the factory
			$db = $this->get(DatabaseConnection::class);

			return new UserRepository($db);
		},
	];
}

The Services object would deliberately have a very small API, perhaps just:

has(string $id): bool
get(string $id): object

Both methods would simply delegate to the underlying container.

The important part is that the container itself would never become part of the public extension API. If we eventually replace League Container with something else, third-party modifications shouldn't need to care.

Injecting services into actions

The end goal is to make these services available to actions through dependency injection, while retaining compatibility with the existing action hooks.

I'm not completely settled on the API here. One possibility is a new method on actions, exposed through a new interface, which tells the action system that the action accepts injectable services.

For example:

interface InjectableActionInterface
{
	// array of strings the container holds.
	public function getDependencyList(): array;
}

The action dispatcher could then resolve those services and pass them as arguments when invoking the action.

I'm not particularly attached to that interface or method name yet, though. InjectableActionInterface, ServiceAwareActionInterface, DependencyAwareActionInterface, etc. all have slightly different implications.

EDIT: When reviewing the spec again, it occurred to me that we could use a setter method instead, either via a factory/callable or by having it make the call itself, although the latter probably isn't portable.

https://github.com/thephpleague/container/blob/6.x/docs/5.x/dependency-injection.md#setter-injection

---

The main goals are:

1. No global/static access to the container.
2. The underlying container remains an implementation detail of SMF.
3. Third-party modifications have a clean way to register services and factories.
4. Services can be injected into actions rather than fetched from a service locator.
5. Existing action hooks remain compatible.
6. The public API stays small enough that we aren't effectively exposing the entire DI container.

I'd be interested in thoughts on both the architecture and, particularly, the naming/API for the action injection part.

[albertlast]

I like the direction, especially keeping League an implementation detail. Right now Infrastructure\Container::__callStatic() hands the whole League API to anyone who calls it, and the cheapest time to close that is before 3.0 ships and mods start using it. Two ideas that build on the proposal:

1. Constructor injection for actions instead of getDependencyList().
2. Service names as declared permissions, so an admin can see what kind of data a mod touches before installing it.

All code below is a sketch against League Container 4.2.5 (the version in composer.lock), not a finished API.

---

1. Constructor injection instead of a dependency list

A getDependencyList(): array of strings says the same thing as the constructor, only a second time and without type checking. If the constructor declares the dependencies, the type hints are the list, and PHPStan and IDEs can check them:

namespace SMF\Actions;

use SMF\ActionInterface;
use SMF\ActionTrait;
use SMF\Services\MemberReader;
use SMF\Services\TopicReader;

final class Unread implements ActionInterface
{
	use ActionTrait;

	public function __construct(
		private readonly TopicReader $topics,
		private readonly MemberReader $members,
	) {}

	public function execute(): void
	{
		$topics = $this->topics->unreadFor($this->members->current());
		// ...
	}
}

The dispatcher takes the new route only for actions that ask for it. Everything that still has the protected, argument-less constructor from ActionTrait goes through load() as it does today, so existing actions and integrate_actions keep working unchanged:

namespace SMF;

final class ActionResolver
{
	public function __construct(
		private readonly Services $services,
	) {}

	public function resolve(string $class): ActionInterface
	{
		$constructor = (new \ReflectionClass($class))->getConstructor();

		// Actions with no dependencies keep the existing singleton route.
		if ($constructor === null || !$constructor->isPublic() || $constructor->getNumberOfParameters() === 0) {
			return $class::load();
		}

		$args = [];

		foreach ($constructor->getParameters() as $param) {
			$type = $param->getType();

			if (!$type instanceof \ReflectionNamedType || $type->isBuiltin()) {
				throw new \LogicException($class . ' can only ask for services in its constructor.');
			}

			$args[] = $this->services->get($type->getName());
		}

		return new $class(...$args);
	}
}

Forum.php:528 would then call $resolver->resolve($current_action) instead of call_user_func([$current_action, 'load']). One open point: load() caches a single instance in static::$obj, and injected actions don't. Either register them as shared, or accept that they are built once per request, which is all a request needs anyway.

This keeps goal 2: only Forum and the resolver know that a container exists.

---

2. Services as permissions

This is where the small Services API from the proposal really pays off: it is the natural place for a permission check.

a) Narrow services instead of broad ones. Rather than a DatabaseConnection service, register services that each name one kind of data and one level of access:

namespace SMF\Services;

interface MemberReader
{
	public function current(): MemberData;

	public function findById(int $id): ?MemberData;
}

interface PostWriter
{
	public function create(NewPost $post): int;
}

b) A mod declares what it needs in package-info.xml, next to the hooks it already declares there:

<install for="3.0 - 3.0.99">
	<services>
		<service id="SMF\Services\MemberReader" />
		<service id="SMF\Services\MailSender" />
	</services>
	<hook hook="integrate_services" function="MyMod\Integration::services" file="$sourcedir/MyMod/Integration.php" />
</install>

c) The Package Manager shows it before install. That screen already lists file operations and hooks, so this fits in naturally. Each service carries a label from Languages/en_US/:

This package requests access to:

• Read member profiles
• Send email

d) Each package gets its own Services, limited to what it declared:

namespace SMF;

use Psr\Container\ContainerInterface;

final class Services
{
	/**
	 * @param array<string, true> $granted Service IDs this consumer may resolve.
	 */
	private function __construct(
		private readonly ContainerInterface $container,
		private readonly string $consumer,
		private readonly array $granted,
	) {}

	public static function forCore(ContainerInterface $container): self
	{
		return new self($container, 'core', ['*' => true]);
	}

	public static function forPackage(ContainerInterface $container, string $package_id, array $service_ids): self
	{
		return new self($container, $package_id, array_fill_keys($service_ids, true));
	}

	public function has(string $id): bool
	{
		return $this->isGranted($id) && $this->container->has($id);
	}

	public function get(string $id): object
	{
		if (!$this->isGranted($id)) {
			throw new ServiceNotGrantedException(sprintf(
				'%s did not declare %s in its package-info.xml.',
				$this->consumer,
				$id,
			));
		}

		return $this->container->get($id);
	}

	private function isGranted(string $id): bool
	{
		return isset($this->granted['*']) || isset($this->granted[$id]);
	}
}

e) Forum wires it up. Each package's factories are bound to that package's Services, not a shared one:

$container = new \League\Container\Container();
$container->delegate(new \League\Container\ReflectionContainer(true));
$container->addServiceProvider(new Infrastructure\ServiceProvider());

foreach (PackageServices::installed() as $package_id => $package) {
	$services = Services::forPackage($container, $package_id, $package['granted']);

	$factories = [];
	\call_user_func_array($package['hook'], [&$factories]);

	foreach ($factories as $factory) {
		$method = ($factory['shared'] ?? false) ? 'addShared' : 'add';

		$container->$method($factory['name'], \Closure::bind($factory['callback'], $services, null));
	}
}

A mod action resolved through ActionResolver with its package's Services gets the same check on its constructor arguments, so undeclared access fails loudly with the mod's name in the error log.

An admin page such as Packages > Installed > Service access can then list every package and what it was granted.

---

The limit, stated plainly

PHP can't isolate code running in the same process. A mod that declares only MailSender can still call Db::$db->query(), read Config::$modSettings, or include Settings.php. So this is a declaration, not a sandbox, and it has to be presented to admins that way, or it will be trusted more than it deserves.

It is still worth having:

• Honest mods get a clean, reviewable way to say what they touch, and admins get a screen that shows it.
• The Package Manager can scan a package at install time for direct Db::$db, Config::, User::$me, eval or exec use, and warn: "This package also accesses the database directly, outside its declared services." It's a heuristic, but cheap.
• Mod review on the SMF site could require that approved mods reach forum data only through their declared services, which turns the declaration into something that is actually checked.

---

Two small things on the current sketch

• In the description's example, function my_integrated_service (array $factories) takes the array by value, so the factories it adds are lost. It needs array &$factories.
• A factory written as static function () {...} can't be bound, and Closure::bind() will fail on it. Worth either documenting or checking with (new \ReflectionFunction($callback))->isStatic() before binding.

The obvious precondition for all of this is that SMF actually grows narrow services beyond ErrorHandlerService. That's why I think the first two concrete steps are the Services class in Sources/ and one read-only service, such as MemberReader, used by one action.

[live627]

Thanks for the suggestions. I'm going to keep to the initial scope. That ActionRegistry code looks funny to me, and it also uses reflection, which I want to avoid. This approach appears to be a form of autowiring, which is a term I just learned about today.

I don't know if actions can override the protected empty constructor in ActionTrait with different arguments, so I'm thinking that setter injection may be a better idea. Not sure if I can make it a contract bound to an interface. I might need to box the args into an array.

[live627]

@tyrsson do you have any thoughts on this plan?

[live627]

This latest commit implements my vision for our new container integration. I have also added a test to prove that it works. Even the integration hook has a test.

Only thing left now is to unwire the previous implementation and migrate the error handling service.

Switching to another container could be a separate PR. The League Container looks complicated.

[albertlast]

Good to see it working end to end, and the hook test is a nice touch. I tried the head of the branch and have a few findings, worst first.

The branch currently fatals on every page

ServiceProvider.php was deleted in 4814f78, but Infrastructure\Container::init() still does addServiceProvider(new ServiceProvider()) at line 46, and index.php calls Container::init() on every request. Pointing the SMF namespace at the branch and booting it:

Error: Class "SMF\Infrastructure\ServiceProvider" not found

I realise this falls under "unwire the previous implementation", so this is only a note that the branch can't be run in its present state. The unit suite stays green because it never loads index.php, which is worth keeping in mind for the rest of this work.

The red build check is a smaller version of the same thing: tests/Unit/Infrastructure/ has no index.php stub, so check-smf-index.php fails.

What the registration loop does with mod input

I ran your loop from Forum::execute() against the kinds of values a mod might pass:

// 1. true, for a class whose constructor needs an argument
$factories[NeedsDb::class] = true;
// ArgumentCountError: Too few arguments to function NeedsDb::__construct()

// 2. a factory written as a static closure
$factories[Db::class] = static fn (): Db => new Db();
// Warning: Cannot bind an instance to a static closure
// Closure::bind() returns null, the factory is dropped, and League builds
// the class from its own name instead. No error, wrong object.

// 3. two registrations of the same id
// The first one wins. DefinitionAggregate::getDefinition() returns the first
// match, so a mod that registers an id before core does keeps it, silently.

// 4. an action asking for a service nobody registered
// League\Container\Exception\NotFoundException, uncaught, so the page dies.

Suggestions, roughly in order of value:

• Catch the resolution failure. A mod that forgets to register something, or is disabled while an action still lists it, shouldn't white-screen the forum. Logging the error and carrying on, or a fatalLang naming the service, would both beat an uncaught exception.
• Reject a duplicate id rather than ignoring it. if ($container->has($name)), then log and skip, so two mods fighting over a name is visible instead of order-dependent.
• Handle the static closure, with (new \ReflectionFunction($factory))->isStatic(), and either bind nothing or tell the mod author. As it stands they get a PHP warning per request and a service that isn't what their factory returns.
• Document what true can do. It works only for classes with no required constructor arguments, which isn't obvious from the hook docblock. Worth saying so in integrationhooks.php.

Where the block sits

Three things follow from it being a local variable inside execute():

• ErrorHandlerService can't use it. Error handling has to work during init() and on every error path, both of which run before this code, and nothing outside execute() can reach $container. That's the migration you listed as remaining, so it may be worth settling where the container lives before doing it.
• Actions dispatched anywhere else get nothing. SSI.php, cron.php and actions that other actions construct internally never reach this block, so setDependencies() is never called and the typed properties stay uninitialised. Reading one throws an Error, which is the failure mode AGENTS.md lists first under things that bite.
• It runs before preflight(), where maintenance mode, the guest-access gate and the agreement redirect are enforced, so mod factories run for requests that are about to be kicked. Moving the block below preflight() costs nothing.

Since only actions consume services today, the whole block could also be skipped unless self::$current_action instanceof DependencyAwareActionInterface. For reference, building the container and registering services costs roughly 12 µs plus about 0.4 µs per service on every request. That's nothing on its own, but it's paid whether or not anything uses it, and League's lookup scans the definition list linearly, so it grows with the number of registered services.

On the interface

You've settled this, so just one small thing rather than reopening it: getDependencyList() and setDependencies() are coupled by array position, and $dependencies[0] / [1] in the action has nothing checking it. String keys in both would remove the ordering hazard without changing the design.

Test hygiene

• IntegrationHook::remove() is commented out in the finally block, so integrate_services leaks into every test that runs afterwards in the same process. ForumTestAction::$obj and Forum::$current_action persist as well.
• $_REQUEST['action'] = '' would be better as unset($_REQUEST['action']).
• ServicesTest.php lives in tests/Unit/Infrastructure/ but declares namespace SMF\Tests\Unit.
• The fixtures have no declare(strict_types=1), and the fixture class ForumTest shares its name with the test class.

[live627]

I'm waiting on @Sesquipedalian to provide feedback on #9699 before I fix the hook removal. He mentioned that he fixed unreported bugs when refactoring the entire codebase to be object-oriented. I don't want to accidentally undo one.

[live627]

• A resolution failure should be fatal and will be caught and logged by SMF's exception handler.
• Actions are only executed through the main path. Any action that loads other action objects (admin, moderate, profile) is responsible for forwarding services. This PR will not be concerned with modifying actions.
• The static closure possibility is, IMO, a thought experiment. None of the other hooks validate the structure of arrays they pass, and I think they should not. The way to validate structural integrity is by using typed objects. This is probably for something like 3.1, 3.2, etc.

I finished migrating the error handler service. Now I just need to run this on my test install. Cheers.