[BUG]: cannot register "injectable" services in DI - infinite loop

[flaviu-chelaru]

Questions? Forum: https://phalcon.link/forum or Discord: https://phalcon.link/discord

Describe the bug
I create a new custom validation, which i wish to register as a shared service in the DI. This results in immediate exception

To Reproduce

Provide output if related. Provide coredump if any. Use https://docs.phalcon.io/en/latest/generating-backtrace as reference.

Fatal error: Uncaught Error: Maximum function nesting level of '256' reached, aborting! in test.php on line 12

Error: Maximum function nesting level of '256' reached, aborting! in test.php on line 12

Steps to reproduce the behavior:

$di = new Phalcon\Di();

class MyService extends \Phalcon\Di\Injectable
{
}

class CustomValidation extends Phalcon\Validation
{
}

$di->setShared(MyService::class, MyService::class);
$di->setShared(CustomValidation::class, CustomValidation::class);

//$service = $di->getShared(MyService::class);
$service = $di->getShared(CustomValidation::class);

Expected behavior
A clear and concise description of what you expected to happen.

I would expect to be able to call this service using getShared method

$service = $di->getShared(CustomValidation::class);

Screenshots
If applicable, add screenshots to help explain your problem.

Details

• Phalcon version: (php --ri phalcon) 4.0.4
• PHP Version: (php -v) PHP 7.4.2
• Operating System: windows 10
• Installation type: download extension from github releases
• Zephir version (if any): none
• Server: apache, plain php execution (directly)
• Other related info (Database, table schema):

Additional context

See attached file
test.txt

Additional Info: this error was identified when migrating a project from phalcon 3.4.x (it was previously working in that version)

[flaviu-chelaru]

Issue further investigation

Phalcon\Di::get calls for Phalcon\Di\Service::resolve at line 202

if typeof instance != "object" {
            if service !== null {
                // The service is registered in the DI.
                try {
                    let instance = service->resolve(parameters, this); // HERE
                } catch ServiceResolutionException {
                    throw new Exception(
                        "Service '" . name . "' cannot be resolved"
                    );
                }

                // If the service is shared then we'll cache the instance.
                if isShared {
                    let this->sharedInstances[name] = instance;
                }
            } else {
                /**
                 * The DI also acts as builder for any class even if it isn't
                 * defined in the DI
                 */
                if unlikely !class_exists(name) {
                    throw new Exception(
                        "Service '" . name . "' wasn't found in the dependency injection container"
                    );
                }

                if typeof parameters == "array" && count(parameters) {
                    let instance = create_instance_params(name, parameters);
                } else {
                    let instance = create_instance(name);
                }
            }
        }

Phalcon\Di\Service::resolve calls for Phalcon\Di::get at line 138

if typeof definition == "string" {
            /**
             * String definitions can be class names without implicit parameters
             */
            if container !== null {
                let instance = container->get(definition, parameters); // HERE
            } elseif class_exists(definition) {
                if typeof parameters == "array" && count(parameters) {
                    let instance = create_instance_params(
                        definition,
                        parameters
                    );
                } else {
                    let instance = create_instance(definition);
                }
            } else {
                let found = false;
            }
        }

Which goes in an infinite loop

[ZhangRuiMingZRM]

I think there is no need to add this feature

772a002#diff-65d61901f4ab5f1161d4c42a99a483fdefda443080527a1094a34a01e3b32c2bR140-R142

cphalcon/phalcon/Di/Service.zep

Lines 137 to 139 in 523b539

	if container !== null {
	let instance = container->get(definition, parameters);
	} elseif class_exists(definition) {

It is confusing and may make the code difficult to maintain if someone does like this

class Foo
{

}

use Phalcon\Di;

$container = new Di();
$container->set("foo", foo::class);
$container->set("bar", 'foo');
$container->set("baz", 'bar');
$container->set("qux", 'baz');

try {

    $container->get("qux");

} catch (\Exception $e) {

}

Expect object Foo but object SomeOne given if does like this

use Phalcon\Di;

$container = new Di();
$container->set("Foo", function () {
    return new SomeClass();
});


$container->set("bar", foo::class);

try {

    $container->getShared("bar");


} catch (\Exception $e) {

}

When registering a registered service in container, should be like this

$container->set("foo", foo::class);
$container->set("bar", $container->getRaw('foo'));

When the type of the parameter (definition) is string, it must be the class name and not the registered service name

$container->set("foo", foo::class);

[Fenikkusu]

A little late to the game here, but simply by removing

$di->setShared(MyService::class, MyService::class);
$di->setShared(CustomValidation::class, CustomValidation::class);

The OP's issue should go away. You're aliasing your class name as itself, so when Phalcon's DI system activates, it'll naturally go into an infinite loop. It's basically the same as you doing:

$di->set(MyService::class, function () {
    return $this->get(MyService::class);
});

$di->getShared(MyService::class);

Using getShared should be enough to accomplish what you are wanting to do, as it will save it internally as a shared object all the time, returning the same instance anyway. Note however that you have no choice but to always call getShared. Using setShared allows you to use get and still get the shared object. This is just me, but I would use Interfaces anyways. IE:

$di->setShared(MyServiceInterface::class, MyService::class);
$di->setShared(CustomValidationInterface::class, CustomValidation::class);

$di->getShared(CustomValidationInterface::class);

This further abstracts the code and gives you better loosely coupled designs.

[flaviu-chelaru]

@Fenikkusu - i believe you missunderstand. First of all, both Di::get and Service::resolve attempt to perform the same task - resolve the service based on a definition. Simply having the same functionality in 2 places it's a bad PHP design. Split the responsabilities

Di is simply a service container - it will keep a bucket of services (some initialized, some not).
When you try to get a service from it, you need to check if the service is shared and initialized. If it isn't, initialize the service. Either here or in the service itself.

Your suggestion is not ok, because I don't want to have aliases. I want to simply register a service in the container.
I also don't want to use anonymous functions if i don't need to (shorter syntax).
I also don't want to declare interfaces for every class i write (overhead).

[Fenikkusu]

I wonder if you can help clear something up for me. Why are you trying to register something as itself? According to your original posted code, you put $this->setShared(SomeClass::class, SomeClass::class). This is quite literally saying 'Register SomeClass::class' as SomeClass::class. Where is the benefit in doing this? The cause of the infinite loop is because of this. Perhaps I'm missing something here, but from my perspective, whether you want to or not, you are aliasing in that you are aliasing the class as itself. If you did something to the affect of $this->setShared('service', SomeClass::class), then certainly that's true DIC. Whenever you call the DIC with a class name, it'll automatically create said class instance for you, provided it's not registered in the DIC already. This is what makes automated testing easy in Phalcon while keeping your code clean:

   //...
   public function testSomething()
  {
     $mock = $this->getMockBuilder(SomeClass::class)->setMethods(['getValue'])->getMock();
     $mock->method('getValue')->willReturn(true);
     $di = new Di();
     $di->setShared(SomeClass::class, $mock);
     $testSubject = new OtherClass();
     $this->assertTrue($testSubject->doSomething);
  }

  //...

  public function doSomething() {
    return $this->getDi()->get(SomeClass::class)->getValue();
  }

To be clear, I'm not advocating aliasing, or anonymous functions, I am just saying that what you have is the equivalent of that. While I am advocating the use of the Interfaces, inline with S.O.L.I.D. coding standards, you are certainly welcome and encouraged to handle it how you want. That's part the beauty of Phalcon in comparison to the other frameworks available.

[flaviu-chelaru]

The Phalcon\Di is basically a services container. All the services should be registered there (as stateless or statefull / shared or not).
"Why are you trying to register something as itself?" => the alternative would be to register it as something else. Something that is not self. "you can register it as anything, but not as itself. It MUST be different". By basic logic, this does not make sense.

If you do not register a service in the container, then the container will try to resolve it automatically - service locator antipattern here. https://blog.ploeh.dk/2010/02/03/ServiceLocatorisanAnti-Pattern/

It will still resolve it, but the definition of the service will never be in the container until it is resolved automatically.
I would expect to have access to all service definitions, even before a service is resolved.

As for your SOLID / interfaces and advocating for it, I would agree with the mention: you don't write code for the principles and the standards. You write code for projects, and you make use of the principles to help you in that endeavor. Use interfaces and anything else that helps you do your job, but don't abuse them just for the sake of it. Instead of having a project where you have to maintain 100+ classes, you have a project where you MUST maintain 100+ classes and 100+ interfaces.

Don't forget that at the moment, the problem reported is about having the same feature in 2 separate locations. This is a duplicate.
Principle#1: DRY
Principle#2: KISS

[Jeckerson]

/cc @phalcon/core-team

[SamAntUA]

Hello, all! How can I avoid setting (MyClass::class, MyClass::class)-like service giving it out for autoresolving if some of this class parameters are not classes, but interfaces or custom named service, or a scalar at all?

$di->setShared(
    MyService::class, 
    ['className' => MyService::class, 
    'arguments' => [
            ['type' => 'service', 'name' => MyDependencyAClass::class, 'shared' => true],
            ['type' => 'service', 'name' => MyDependencyBInterface::class, 'shared' => true],

[niden]

I have never used a class string as the name of a service(MyService::class) - I guess there might be a use for it at some point.

The autowiring of our current Di container is very rudimentary and needs extensive testing. We just never had the chance to create tests for that and ensure that everything works as expected. One of our todos is to rework the Di container for full autowiring and that includes the documentation :)

The only auto-resolving that we have in the framework is the one in the FactoryDefault

use Phalcon\Html\Escaper;
use Phalcon\Html\TagFactory;

// Setup with `setShared` and class string
$container->setShared('escaper', Escaper::class);

// Setup with `new Service()` and injected service (shared = true)
$container->set(
    "tag",
    new Service(
    [
        "className" => TagFactory::class,
        "arguments" => [
            [
                "type" => "service",
                "name" => "escaper"
            ]
        ]
    ],
    true
);

[SamAntUA]

Nikolaos, thank you for your reply!

The reason why I use class names as service names is to not to search and think about what name I gave to a specific DI-service (maybe somewhere in another php file).
For example, I need to add a dependency to my MyService. Let say some UserRepository class. I add it to class constructor, then I go to my php file containing DI-service definitions and add to arguments section ['type' => 'service', 'name' => starting typing here UserRepo... IDE autocomplete helps me, ::class and ready! I don't need to search for a place I defined a DI-service of UserRepository. And this way I'm defended from mistake typing letter by letter some custom string name (e.g. 'escaper')

So, the problem of exceeding max function nesting level is still present and I don't know how to fix it :(

[niden]

I am going to try and see if I can figure this out.

[flaviu-chelaru]

@niden - if you believe you need assistance in reproducing the issue, please feel free to contact me

[niden]

@niden - if you believe you need assistance in reproducing the issue, please feel free to contact me

Yes please. Either post a sample here or send me a message on Discord so that I can recreate this.

[flaviu-chelaru]

<?php

use Phalcon\Http\Request;
use Phalcon\Http\RequestInterface;

$container = new Phalcon\Di();

## case 1 - works
$container->setShared('request', Request::class);
$object = $container->getShared('request');
print 'case 1 - worked' . PHP_EOL;

$container->setShared(RequestInterface::class, Request::class);
$object = $container->getShared(RequestInterface::class); 
print 'case 2 - worked' . PHP_EOL;

$container->setShared(Request::class, Request::class);
$object = $container->getShared(Request::class); // this throws exception
print 'case 3 - worked';

Try to execute this
PHP Version: 7.4
Phalcon version: 4.1.2

[niden]

Resolved in #16242

Thank you @cfv1000 for reporting it and all the help reproducing