Consider using more Singleton/Bind at Service Container to handle classes instances

[henriqueramos]

Back on pkp/pkp-lib#7125, I had a problem with IsoCodesFactory showing countries names list on Portuguese version, even the OJS installation had en_US and fr_CA locales as only languages. Thus my local test scenarios aren't being able to pass.

This was due to the way IsoCodesFactory implement his translation driver, getting the current language value directly from the LANGUAGE environment variable, and not from the LC_ALL, as we did on PKPLocale::initialize.

I've fix this issue, by creating a Singleton for the IsoCodesFactory, setting the locale driver same as the PKP locale, and calling this instance across the system.

i.e

PKPContainer::registerBaseBindings

$this->singleton(IsoCodesFactory::class, function () {
    $driver = new GettextExtensionDriver();
    $driver->setLocale(PKPLocale::getLocale());
    return new IsoCodesFactory(null, $driver);
});

$isoCodes = app(IsoCodesFactory::class);

dd((array) $isoCodes->getCountries());

But we should rely more on Singletons and Binds at the Service Container, empowering us to control our classes instances better.

[NateWr]

I agree. It took me a long time to get my head around the Service Container, but once I did I realized that it is going to play a huge role going forward in helping us manage dependencies across all three applications. It also offers us a great way to expose some of the core pieces of the software to extension by plugins.

We should aim to integrate many of our core systems (eg - PKPLocale, Application, TemplateManager, Router, Dispatcher) into the service container, as well as all of the existing service classes that do not fit the Repo/EntityDAO structure (FileService, PKPSchemaService, StatsService, StatsEditorialService). The latter conversion is filed at https://github.com/pkp/pkp-lib/issues/7131.

@asmecher I know there's already a lot to consider at the moment, but if you get a chance to explore the Service Container system -- and especially the combination of its dependency injection and binding features -- I think there's a lot of opportunity here. Off the top of my head, it would be an appropriate place to provide support for things like remote file support, third-party mail services, caching mechanisms, etc. The nice thing about the container approach is that a plugin can inject one class instead of another, so overriding behaviour at a low-level is often as simple as extending a class, overriding methods, and binding that class to calls for another.

[asmecher]

@NateWr, yep, Henrique and I were going over this on Monday, which is what led to this filing. I'm still in the getting-my-head-around it phase but am very interested in pushing our use further.

[NateWr]

So far, I've been putting our core dependencies into an AppServiceProvider. You can see here how I've used this to override PKPRequest with Request.

pkp-lib: https://github.com/pkp/pkp-lib/blob/main/classes/core/AppServiceProvider.inc.php#L45-L47 ojs: https://github.com/pkp/ojs/blob/main/classes/core/AppServiceProvider.inc.php#L30

[jonasraoni]

Talking about the container, we can already get rid of the Pimple dependency =] As a side note, I'm using the container to serve the locale for the #6328:

public function register(): void
{
    $this->app->singleton(LocaleInterface::class, function () {
        return $this->app->make(Locale::class);
    });
    // Replaces the default Laravel translator
    $this->app->alias(LocaleInterface::class, 'translator');
    $this->app->alias(LocaleInterface::class, Translator::class);
}

To keep the ability of using the Locale statically (e.g. Locale::getLocale()), I've created a Facade for it:

class Locale extends \Illuminate\Support\Facades\Facade
{
    protected static function getFacadeAccessor(): string
    {
        return LocaleInterface::class;
    }
}

About testing, the Facade provides mocking methods and it's even possible to replace the underlying Locale instance on the fly with Locale::swap(new MockedLocale()).

[NateWr]

That's cool, @jonasraoni! When extending Illuminate\Support\Facades\Facade and using getFacadeAccessor, is your code editor able to pick up the available methods for autocomplete? Like if you type Locale::get does it show you that getLocale() is a method on the class?

I think that was one of the hurdles I ran into when making the Repo facade, but maybe I was doing something wrong.

[jonasraoni]

Yeah! That's the stinky part of the Facade, which I was going to discuss after opening a PR. As it just has a __callStatic, other limitations include: accessing properties, constants, passing it as an argument, etc.

FYI Laravel is using a docblock to help the code editor display the available methods.

---

I think the Facade is handy, but I would personally opt-out from it and follow a more standard/typed approach, so I think it's good opportunity to discuss a pattern here :)

I think the most straightforward way is to have accessors, like what you did in the PKP\facades\Repo. For the locale, I could add a Locale::get() to avoid using the untyped app(Locale::class).

[NateWr]

That all sounds good. If we are going to go this route, I wonder if it makes sense to step back and think about when we want to use facades and when we should rely on dependency injection. In general, I think dependency injection is preferred.

But when it comes to building extensibility into the system, facades are really useful. For example, I'd be tempted to thing that a Locale class shouldn't be need outside of a few core classes like the request and the application. But I suspect that there will be cases where a plugin or event of some kind wants access to it. Is it better to have a wrapping class, like Application, that provides access to these? Or to use a facade of some kind to get a singleton?

[jonasraoni]

Hmm, I support using shortcuts (in my case, it will be Locale::getInstance()) for now, and "wire" the classes with dependency injection as we can.

About the Laravel's Facade, which I've used mostly to keep things similar, I think it's better to discard 🤔

• The only use case I see is to enforce all code base is using the active instance (e.g. if N classes receive a Request instance by DI, then we replace the request in the container, they will keep using the old one).
• I prefer to use such dynamic features of the language for dynamic tasks (e.g. navigating through a JSON/XML).

[NateWr]

The only downside to using something like Locale::getInstance() is that the class can not be overwritten. Making use of Laravel's service containers we can do something like App::make(Locale::class) and plugins can inject their own extended class at run-time...

[jonasraoni]

Hmm, I think this way is ok:

public static function getInstance(): LocaleInterface
{
    return App::make(LocaleInterface::class);
}

The class is just serving as a shortcut to get a type-hinted instance, and a plugin might still replace the LocaleInterface in the container by an extended version.

[henriqueramos]

@asmecher As requested by you on #7125, I've separated the singleton for the IsoCodesFactory.

Could you please do the code review on this subject? Thank you!

7382

pkp/ojs#3205

[NateWr]

I'm curious: when should we use an alias with a binding like this? An alias lets us bind a singleton to a name, so that instead of doing this:

$countries = app(IsoCodesFactory::class);

We can use an alias like:

$countries = app('countries');

Is this a good idea or a bad idea? I'm not sure. Pros: reduce the chance that someone will accidentally use $countries = new IsoCodesFactory(). Cons: have to document the alias and make sure developers know about it.

I'm thinking for some of our global singletons (PKPLocale, Config) it might make sense to implement an alias. But I'm not sure about more rarely used singletons like the country list.

(This is a general question, not a comment on your PR, @henriqueramos.)

[jonasraoni]

I think it's better to register using type::class, simple aliases are more likely to generate naming conflicts 🤔

About accessing the container directly... Given that app(...) will require adding @var type hints, I created a shortcut, but well, in the future this construction will probably be replaced by the dependency injection.

[henriqueramos]

@asmecher I've migrate the extra codes from #7125 to #7282.

7382

pkp/ojs#3205

[asmecher]

Merged the PRs above, @henriqueramos! Does this need to be ported to OMP or OPS?

[henriqueramos]

Merged the PRs above, @henriqueramos! Does this need to be ported to OMP or OPS?

Only at OMP @asmecher.

pkp/omp#1016