fix: BaseService getSharedInstance should properly cache null

[najdanovicivan]

Description
When we have service which returns null the getSharedInstance will not return the cached value. We should have the null values cached. One example is when using service to return currently logged in user. In case there is no user logged in the return value will be null. With current behavior each time user service is called getSharedInstance will always call the non shared variant. This will make the service method access the session data every time checking if there is an active user session.

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[kenjis]

In my understanding, a service is an object. Null is not a service.
How do you define such a service?

[najdanovicivan]

This is sample service which returns Myth Auth User but will return null when there is no user logged in

    public static function loggedInUser(bool $getShared = true): ?UserEntity
    {
        if ($getShared) {
            return static::getSharedInstance('currentUser');
        }

        if (is_cli() || service('request')->getUserAgent()->isRobot()) {
            //No user for robots
            return null;
        }

        $authenticate = service('authentication');
        $authenticate->check();

        //This can return UserEntity or null if user is not logged in
        $user = $authenticate->user();

        //I have some additional initialization logic here based on user settings eg. setting locale ....

        return $user;
    }

[paulbalandan]

I think this change will be a breaking change.

Consider this contrived example assuming app does not use Composer.

public function getBarService(): object
{
        if (service('bar') === null) {
                require 'path/to/bar/service.php';
        }

        return service('bar');
}

Old behavior will work as expected because 2nd call will return the fresh instance.
New behavior will throw a TypeError because all calls to bar service will return null.

IMO why null is not cached is because it is not the desired state of a service. It is an indication that currently nothing is there for the service. If we will be keeping null as the state, then if we want subsequent calls to be an object we need to set it first, like:

// on 4.5.x
service('bar'); // null

Services::set('bar', new Bar());
service('bar'); // Bar

It seems counter productive to me.

[najdanovicivan]

In #8572 was stated that service should be kind of container. So having mixed as return type makes null also a valid type.

In my case the user service will always return null until user is logged in so the service will have same value in single request.

From performance endpoint there is no sense trying to load the service multiple times in a single request when value will always be null

Possible solution will be using a bit different approach instead of modifying getSharedInstance will be to have additional method getSharedNullableInstance() that way we can have needed functionality without introducing a BC

[kenjis]

If we change Services to a container, I think it will be a breaking change.
At least, the current Services is NOT a container.
It returns an object or null, and null means the service is not found.
If it is a container, it should throw an exception if the entry is not found.

Personally, I think Services is a slow and complex service locator.
And Service Locator is an anti-pattern.
I would abolish it if could, but it is difficult.

[lonnieezell]

I did a bunch of researching before implementing this and I can say it's not a service locator by the traditional definitions. It's faster than an IoC would be and much easier to understand, though some layers were added to it since I originally put it in place. I mean heck all it's doing is defining how to configure a class, caching it, and returning an instance. There's very little happening there that wouldn't happen in your controller or library except that you always have just the one instance and don't have to pass it down through multiple layers of code.

And remember- the tests you've shown with speed improvements are in the thousandths or ten-thousandths of a second. So it's not exactly slow. You have managed to make it faster but it's still fraction of a millisecond here :)

There's a link in the BaseService to an article that explains why it was originally used. And I still stand by that reasoning. If anything I think we've gone wrong in other places since then but the time for discussions is way past.

[kenjis]

I also thought Services was not bad in the past, and it was simpler than now.
I agree it is easier to understand, but after all now it is not faster.

Test 			Time 	Memory
services::request() 	0.2077 	0 Bytes
services::get() 	0.0588 	0 Bytes
php-di 			0.0593 	0 Bytes

<?php

namespace App\Controllers;

use Config\Services;

class Home extends BaseController
{
    public function index(): string
    {
        $iterator = new \CodeIgniter\Debug\Iterator();

        $container = new \DI\Container(); // php-di/php-di (7.0.6)
        $container->set('request', $this->request);

        $iterator->add('Services::request()', static function () {
            Services::request();
        });

        $iterator->add('Services::get()', static function () {
            Services::get('request');
        });

        $iterator->add('PHP-DI', static function () use ($container) {
            $container->get('request');
        });

        return $iterator->run(300000);
    }
}

I haven't tried it because it's not easy, but I guess PHP-DI (PHP-DI is just an example) would be faster than Services if all services were properly defined and compiled.
That said, I don't know if there is enough benefit to replace Services with another container.

[lonnieezell]

@kenjis I'm not sure why you think a DI would be any faster than what we currently have. It would only provide the same features we have now, with the added responsibility of trying to intelligently guess which class it should use. Other than that it still:

• creates an instance of the requested class
• returns either a shared instance or a new one
• provide a way to insert mocks for testing
• has to have a way to configure the class, which is typically done in another location separate from anything else.

So all of the features are identical between a DI/IoC container and what we provide. But it would do it in a way that's more difficult to understand and debug and has its parts in different areas instead of all together. Oh, and it is less flexible because people cannot do things with it that we didn't anticipate but that works for their needs.

Ignoring all of that even pondering the thought of replacing something we currently offer with another method that does the same thing in an arguably worse way that will completely break every single app ever written on CI4 for no good reason is crazy :)

@najdanovicivan Sorry for completely derailing the conversation around this PR lol

In #8572 was stated that service should be kind of container.

Actually, what I said was "My original idea for it was to replace IoC containers in a simpler way" which doesn't make it a container. It's specifically not a container though it fulfills a similar purpose. I already stated over there that I didn't see anything wrong with it returning null, which it currently does.

IMO why null is not cached is because it is not the desired state of a service.

I both agree and disagree with this. If the service is an external service, like a connection to an third-party API and is used to setup the credentials, etc before returning, there is the possibility that the external API could be down, the server could have a misconfiguration and the network is down, meaning there is no valid service to return. You have two options at that point, either return NULL or throw an exception. Which is better? I could see arguments either way. But in that case, I see caching the NULL state as perfectly valid because you don't want to authorize against that service multiple times in a single request.

[kenjis]

I don't think it is good practice to add code in Services other than creation of instances.
Services responsibility is to return the service instance.
It is simple that Services returns only instances.

Why should it be complicated to return values of various types?
If you want to represent null, you can just create and return a NullObject.

Why does not service('authentication') have getloggedInUser() method?
Why does the Services need to check the login state?
It seems to me that the Services is doing a lot of extra work.

[paulbalandan]

To me, if a service relies on an external service and that external service is down, our service should not swallow that info by returning null. It should throw and tell the dev "Hey, something's wrong with your connection to this endpoint." Returning null for both (1) service is not found or (2) external endpoint for a valid service goes haywire is quite confusing. We would be needing a complex if to determine what this service's null return mean: did the service fails or is the service inexistent.

[paulbalandan]

In my understanding, this PR stems from the use case of having the user service returning null. I mean why change the logic here in BaseService instead of that user service? I agree with @kenjis that by doing so we add extra unnecessary work. At least let's abide by the single reponsibility pattern of SOLID.

[kenjis]

No member actively supports this change.
So this breaking change is unacceptable.
Close this PR.