Fall back to autoloader when class is not in container

[samdark]

Currently there's a need to declare everything that is repetitive if we need to set a single class without an interface into container:

\App\ConsoleCommand\CreateUser::class => \App\ConsoleCommand\CreateUser::class,

In such cases we can try to fall back to class autoloading.

[rustamwin]

using aliases?

[samdark]

No.

[rugabarbo]

@samdark it seems already implemented:

di/tests/Unit/ContainerTest.php

Lines 64 to 69 in 7dd3ce1

	public function testWithoutDefinition(): void
	{
	$container = new Container();
	$engine = $container->get(EngineMarkOne::class);
	$this->assertInstanceOf(EngineMarkOne::class, $engine);
	}

[samdark]

Does that work for sub-dependencies i.e.

class Test
{
    public function __construct(TestDependency $testDependency)
    {
    }
}

class TestDependency
{
}

$container->get(Test::class);

[rugabarbo]

There is the incomplete test about it:

di/tests/Unit/ContainerTest.php

Lines 42 to 51 in 7dd3ce1

	public function testOptionalClassDependency(): void
	{
	$this->markTestIncomplete('TODO: implement optional dependencies');
	$container = new Container();
	$container->set(A::class, A::class);
	$a = $container->get(A::class);
	// Container can not create instance of B since we have not provided a definition.
	$this->assertNull($a->b);
	}

The test passes successfully in this form:

    public function testOptionalClassDependency(): void
    {
        $container = new Container();
        $container->set(A::class, A::class);

        $a = $container->get(A::class);
        $this->assertInstanceOf(B::class, $a->b);
    }

Console output:

$ ./vendor/bin/phpunit --filter=testOptionalClassDependency
PHPUnit 7.5.16 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.2.22-1+ubuntu16.04.1+deb.sury.org+1 with Xdebug 2.7.2
Configuration: /tmp-for-test/yii-dev/dev/di/phpunit.xml.dist

.                                                                   1 / 1 (100%)

Time: 79 ms, Memory: 4.00 MB

OK (1 test, 1 assertion)

Therefore, it is not clear what exactly needs to be changed.

[rugabarbo]

It seems like we can just adjust the tests and close this task. Do you agree?

[samdark]

Yes. I've re-checked it and it works.

[rugabarbo]

@samdark, it seems that things are not so simple. I see that design solutions are needed here.

Now autoload is triggered by calling method class_exists here:

di/src/Container.php

Line 128 in 7dd3ce1

if (class_exists($class)) {

If you add a new test to the test suite, then it passes:

    public function testDependenciesWithoutDefinition(): void
    {
        $container = new Container();
        $a = $container->get(A::class);
        $this->assertInstanceOf(A::class, $a);
        $this->assertInstanceOf(B::class, $a->b);
    }

Thus, autoload is triggered for undeclared classes and their dependencies, BUT for consistency, method Container::has() should also return true for undeclared classes. Now it doesn't:

di/src/Container.php

Lines 182 to 191 in 7dd3ce1

	/**
	* Returns a value indicating whether the container has the definition of the specified name.
	* @param string $id class name, interface name or alias name
	* @return bool whether the container is able to provide instance of class specified.
	* @see set()
	*/
	public function has($id): bool
	{
	return isset($this->definitions[$id]);
	}

Moreover, there is a test suite that relies on this behavior of has() method:

di/tests/Unit/ServiceProviderTest.php

Lines 30 to 41 in 7dd3ce1

	protected function ensureProviderRegisterDefinitions($provider): void
	{
	$container = new Container();
	$this->assertFalse(
	$container->has(Car::class),
	'Container should not have Car registered before service provider added.'
	);
	$this->assertFalse(
	$container->has(CarFactory::class),
	'Container should not have CarFactory registered before service provider added.'
	);

So, it looks like the implementation of the container is now inconsistent. We should discuss this and make some decisions about how it should work.

[samdark]

has() should behave same as get() now. Could be achieved by triggering autoloading for class_exists() in case there's nothing directly registered in container.

[rugabarbo]

has() should behave same as get() now. Could be achieved by triggering autoloading for class_exists() in case there's nothing directly registered in container.

But this approach can cause performance problems. What do you think about it?

[samdark]

Why should it cause performance problems?

[rugabarbo]

Why should it cause performance problems?

Typically, method Container::has() is massively used to determine the availability of services in a project. When some services are unavailable, many misses occur. In the proposed implementation each miss will cause autoload. For example, composer requires optimization of the second level in order to quickly work with misses: https://getcomposer.org/doc/articles/autoloader-optimization.md#trade-offs

The only issue is it does not keep track of autoload misses (i.e. when it can not find a given class), so those fallback to PSR-4 rules and can still result in slow filesystem checks. To solve this issue two Level 2 optimization options exist, and you can decide to enable either if you have a lot of class_exists checks that are done for classes that do not exist in your project.

Calling method isset() is quick. But calling method isset() in combination with method class_exists(), it seems to me, can cause performance problems in typical configurations. Do you think we don’t need to worry about it now?

[rugabarbo]

@samdark I just created benchmarks for method Container::has().
See #90.

Test N1

Method Containter::has() implementation (current):

    public function has($id): bool
    {
        return isset($this->definitions[$id]);
    }

Bench results:

$ ./vendor/bin/phpbench run --filter=MethodHas
PhpBench 0.14.0 (@git_version@). Running benchmarks.
Using configuration file: /yii-dev/dev/di/phpbench.json

\Yiisoft\Di\Tests\Benchmark\ContainerMethodHasBench

    benchPredefinedExisting       I4 P0 	[μ Mo]/r: 0.070 0.070 (μs) 	[μSD μRSD]/r: 0.001μs 1.66%
    benchUndefinedExisting        R2 I3 P0 	[μ Mo]/r: 0.071 0.071 (μs) 	[μSD μRSD]/r: 0.001μs 1.38%
    benchUndefinedNonexistent     R3 I4 P0 	[μ Mo]/r: 0.069 0.068 (μs) 	[μSD μRSD]/r: 0.002μs 2.25%

3 subjects, 15 iterations, 3,000 revs, 0 rejects, 0 failures, 0 warnings
(best [mean mode] worst) = 0.068 [0.070 0.070] 0.072 (μs)
⅀T: 1.052μs μSD/r 0.001μs μRSD/r: 1.761%

Test N2

Method Containter::has() implementation (proposed):

    public function has($id): bool
    {
        return isset($this->definitions[$id]) || (is_string($id) && class_exists($id));
    }

Bench results:

$ ./vendor/bin/phpbench run --filter=MethodHas
PhpBench 0.14.0 (@git_version@). Running benchmarks.
Using configuration file: /yii-dev/dev/di/phpbench.json

\Yiisoft\Di\Tests\Benchmark\ContainerMethodHasBench

    benchPredefinedExisting       I4 P0 	[μ Mo]/r: 0.071 0.071 (μs) 	[μSD μRSD]/r: 0.001μs 1.65%
    benchUndefinedExisting        R2 I3 P0 	[μ Mo]/r: 0.361 0.364 (μs) 	[μSD μRSD]/r: 0.008μs 2.21%
    benchUndefinedNonexistent     R1 I2 P0 	[μ Mo]/r: 0.538 0.527 (μs) 	[μSD μRSD]/r: 0.016μs 2.93%

3 subjects, 15 iterations, 3,000 revs, 0 rejects, 0 failures, 0 warnings
(best [mean mode] worst) = 0.069 [0.323 0.321] 0.072 (μs)
⅀T: 4.846μs μSD/r 0.008μs μRSD/r: 2.261%

[samdark]

1. Were the tests conducted with composer autoloader optimized?
2. Triggering autoload is, of course, slower but I think we can ignore it in this case because has() is rarely used by itself. Usually it's get().
3. There's a usage that should be checked in Factory: https://github.com/yiisoft/factory/blob/master/src/Factory.php#L55

[rugabarbo]

1. Were the tests conducted with composer autoloader optimized?

Yes. I used:

composer install --optimize-autoloader

2. Triggering autoload is, of course, slower but I think we can ignore it in this case because has() is rarely used by itself. Usually it's get().

Ok, I also think that we can ignore this, because these are fractions of microseconds.

3. There's a usage that should be checked in Factory: https://github.com/yiisoft/factory/blob/master/src/Factory.php#L55

Ok, I'll check it too.

[rugabarbo]

@samdark, mark the item completed in the roadmap: https://github.com/yiisoft/docs/blob/master/003-roadmap.md#psr-11-container

[samdark]

Done.

[nd4c]

Is the Factory issue https://github.com/yiisoft/factory/issues/3 also closed?

[rugabarbo]

Is the Factory issue https://github.com/yiisoft/factory/issues/3 also closed?

No, because the future of package factory is unclear. Now class Factory is not used anywhere.