[4.0] Add deprecate messages for container access in the Factory

[laoneo]

As discussed in #16918 the container functionality in the Factory exists only for the transition period, this pr adds the missing deprecated tags.

I'm splitting #16918 into different pr's to be easier to review.

[wilsonge]

So before we merge this we need to make a decision about what the "one true" global in the system is going to be. In Symfony it's the Kernel where you call Kernel::getContainer which is our equivalent of the application. In Joomla do we want to go down the path of the application being globally available in a similar way using factory or whatever, or the container.

Thoughts from you @mbabker or @nibra ?

[nibra]

I personally like the approach to have (a proxy to) the container in the application object. That way, there's architectural no global, just a main object. Since the application is started with something like

$app->execute($request);

the application can easily provide itself to other parts, so the container (content) can be accessed.

[laoneo]

Do we even need a global? The goal of our DI approach should be to eliminate it. On a long term stuff should be injected. Why do you want to have a global?

[mbabker]

So here's the thing. If we don't have something like Symfony's Kernel, which is in essence doing a lot of the architectural bootstrapping that we do throughout our includes/app.php file (and associated includes), we are stuck with having things in global scope (even if not explicitly declared using the global keyword, if you do a var_dump($GLOBALS); you'll see some stuff leaking forward from our bootstrap process). The Kernel in essence acts as the foundation of the request cycle and manages all core dependencies, including booting the DI container. We aren't doing anything like that at the moment, with our code structure our root CMSApplication object is a request/response handler (or, for those familiar with the code structure of the frameworks, the "deepest" root object we have in our application compares to illuminate/http or symfony/http-foundation, we don't have something like illuminate/foundation or symfony/http-kernel).

[brianteeman]

as this is only a deprecation notice for something to be removed in 5 is there any reason not to merge it?

[brianteeman]

Repeating my comment from last year
as this is only a deprecation notice for something to be removed in 5 is there any reason not to merge it?

[mbabker]

is there any reason not to merge it?

Yeah. The current message is unclear and self contradicting. The annotation on Factory::getContainer() says it's deprecated, to be removed in 5.0, but the message says "well we don't know when it'll be removed".

In truth, the architectural issues pointed out in this thread need to be addressed so there is a proper plan surrounding this deprecation. Otherwise right now it is adding a deprecation without anything actionable for when the deprecated element should be removed or how its removal doesn't catastrophically break the CMS.

[laoneo]

@mbabker any suggestion for a better text?

there is a proper plan surrounding this deprecation
The problem is not this deprecation. The problem is that all these services stuff got introduced without a proper plan to use it in core, especially in the MVC layer. Right now it looks like that there is a one to one replacement of static function calls to Factory::getContainer()->get('service').

[mbabker]

The problem is not this deprecation. The problem is that all these services stuff got introduced without a proper plan to use it in core

And therein lies the root issue. Here's why I don't see a deprecation of Factory::getContainer() ever sticking.

Generally, in a PHP application that is built around some kind of core container (joomla/di, symfony/dependency-injection, illuminate/container), you have a layer at the top of your application that handles booting things up and assembling your container generally called a Kernel (symfony/http-kernel and illuminate/foundation are the packages for those corresponding ecosystems, Joomla doesn't have anything on par with that concept but I've introduced something that acts like that at a high level into the issue tracker and stats server apps, relevant links are for the www/index.php and src/JTracker/Kernel.php files). That layer is generally responsible for booting everything, two of the biggest things being the equivalents to the CMS' components (Symfony Bundle) or plugins (event listeners). As we all know, Joomla does not have that type of bootup stage; rather, it dynamically boots services when something in the system decides it is necessary, which is why you have to manually import plugin groups or boot components (concept added in 4.0) to get things to work. So for that reason alone, Factory::getContainer() can't go away unless you're saying that extensions should not interface with the core service container (for any valid reason; retrieving a service, extending a service, adding new services, etc.), and if that's really the case then the entire DI layer should just be backed out and the use of Joomla\CMS\Factory and Foo::getInstance() just etched in stone as a permanent architecture feature.

As a side effect of that architectural concern, a lot of code (primarily in static helpers because you don't have the luxury of being in a class instance and you risk potentially leaking implementation details if you make all dependencies of a static something that has to be injected, #23779 highlights why that is an architectural concern) is calling Factory::getContainer()->get() in place of a Foo::getInstance() or Factory::getFoo() call. It's a step in the right direction as it's generally easier to mock building services in a container than it is trying to deal with non-mockable static methods returning things in a non-mockable state (a valuable thing in an application that is well tested), but it's still not "pure" as it relates to dependency injection or use (some will say misuse) of service locators.

There is still a lot of work that needs to be done to make dependency injection the preferred method of operation in the CMS over using a static factory and service location. Some intimate details include:

• Joomla\CMS\Form\FormField expects a Joomla\CMS\Form\Form instance in its constructor (an optional dependency but still one that exists); a form instance is not a global service so this can't reliably be shoved in the DI container permanently. Next, the form field (and rule) classes are direct instantiated in Joomla\CMS\Form\FormHelper::loadType() without any constructor dependencies passed in, meaning right now it's impossible to create a DI oriented form field or rule. So, in order to add prototypes for these class types to the containers (non-shared services because each field at least needs to be a unique instance for the <field> element it represents from the XML schema), some work needs to go into how these classes are instantiated (this is outside my other past commentary on having a form registry to get away from things like shoving namespaces into the XML schema).
• The main interface with ACL is through Joomla\CMS\Access\Access. The main interface for the filesystem is through Joomla\CMS\Filesystem classes. There is a whole lotta static in there.
• Everything with options arrays as constructor arguments (which includes the MVC layer). This realistically makes having those classes be services within a DI container pretty darn painful (for example, Joomla\CMS\Installer\InstallerAdapter uses this array to set class member variables, Joomla\CMS\MVC\Controller\BaseController uses this to configure various internals about a controller instance). To prototype these services (again, meaning non-shared services in the container; just for reference a shared service in a container is generally considered singleton) you would have to move these arrays out of the constructors and handle those in some other way, otherwise at best right now you can create factories to manage creating instances of those classes.

[mbabker]

For now, what I would suggest doing is this:

• Use a @note annotation on the container stuff in Joomla\CMS\Factory to describe how the container is only intended to be statically accessed in service locator code (anything whose responsibility is to create a class)
• Ensure core is following this practice and things aren't just doing a swap of Factory::getDbo() with Factory::getContainer()->get(DatabaseDriver::class) for the sake of that swap
• If/When the core architecture can be updated in a way where this factory service is no longer necessary, THEN issue the deprecation. Otherwise, right now adding a deprecation is basically saying "just don't use this at all but core needs to use it to do all this stuff so ignore us and our use of this deprecated code". I don't know the answer to how to reach a state to be able to sanely deprecate this, I just know adding it right now sends a mixed message.

[laoneo]

@mbabker to speed up things, feel free to open a pr and I will close this one then.

[mbabker]

/**
 * Get a container object
 *
 * Returns the global service container object, only creating it if it doesn't already exist.
 *
 * This method is only suggested for use in code whose responsibility is to create new services
 * and needs to be able to resolve the dependencies, and should therefore only be used when the
 * container is not accessible by other means.  Valid uses of this method include:
 *
 * - A static `getInstance()` method calling a factory service from the container, see `Joomla\CMS\Toolbar\Toolbar::getInstance()` as an example
 * - An application front controller loading and executing the Joomla application class, see the `cli/joomla.php` file as an example
 * - Retrieving optional constructor dependencies when not injected into a class during a transitional
 * period to retain backward compatibility, in this case a deprecation notice should also be emitted to
 * notify developers of changes needed in their code
 *
 * This method is not suggested for use as a one-for-one replacement of static calls, such as
 * replacing calls to `Factory::getDbo()` with calls to `Factory::getContainer()->get('db')`, code
 * should be refactored to support dependency injection instead of making this change.
 *
 * @return  Container
 *
 * @since   4.0
 */
public static function getContainer(): Container;

That should be enough. Usually I'm not a fan of this verbose of doc blocks in the code, but since a lot of people like to argue nobody reads the documentation anyway this is the one time I'll bend on that opinion.

As for the last example of a valid use, I'd say the Joomla\CMS\Editor\Editor constructor does it right but hasn't deprecated not passing the dispatcher into the constructor, if that deprecation is added then the reference like I did for the other two examples can be added to that point.

[laoneo]

Adapted the message with the suggestion from Michael.

[mbabker]

This works until there's a definitive long term plan in place (FWIW I fully support deprecating Factory::getContainer() ASAP but doing it now is premature for the reasons I pointed out before). 🚢 it!

[wilsonge]

🚢 'd Thanks for figuring this out guys :)