We need the "Documentation Required" label here.

Added label as requested

This hard couples the entire application stack to the CMS application class chain

Why? It cleans up the mess with different applications. At the end it all should work with the CMSApplicationInterface which cli does implement as well. There is now one way to have applications set up in the front controllers.

You are creating dependencies galore to JFactory::getApplication(), which is inherently coupling everything to the CMSApplicationInterface. For no other reason than to fetch the service container.

I added JFactory::getContainer() with a very specific purpose. It is there to bridge the gap from the existing 3.x mentality of no services or injection at all to building a framework that supports DI, with a goal of deprecating it once we have bridged enough of core to use a proper DI/service injection workflow (potentially during 4.x and removal at 5.0). There are API functions we have now that act as service locators and must be able to find the container to find their services during that transition point, that is why the container needs to exist in the factory as a separate entity from the application.

To me this PR is a step backwards. Instead of us having hard dependencies on JFactory, we now have hard dependencies on a CMSApplicationInterface implementation, still retrieved out of JFactory. This isn't improving anything.

You and me know that it will be very hard to get away from JFactory::getContainer later on. We want to get rid of JFactory at all, introducing more functions on it is for me a step backwards.

And you think it'll be easier to get rid of JFactory::getApplication() calls later on?

And you think it'll be easier to get rid of JFactory::getApplication() calls later on?

An app can be injected, JFactory::getContainer not. This is the only reason. I would even introduce a new function like getServiceLocator on the app which clearly states that this is a service locator. For me it is totally wrong to work on a DI container by accessing it globally in a static way.

Look how many test cases you had to add mocking the application into. You are creating new hard couplings that did not exist previously. That is a major problem. That demonstrates exactly why this PR should not be accepted in this state.

An app can be injected, JFactory::getContainer not.

JFactory::$container, the same way JFactory::$application lets you inject anything for the application dependency.

For me it is totally wrong to work on a DI container by accessing it globally in a static way.

JFactory::getContainer() is the bridging layer. It's no different than the class mapping stuff. It allows us to retain backward compatibility while migrating the code forward to a new philosophy and structure.

You can call it JFactory::getApplication()->getTheContainerSinceIDoNotDoDependencyInjectionCorrectly() for all I care. It's still giving you the DI container. It's still not the right way to do things. Your way is creating a second level dependency (you have to have both an application and a container now), my way had one dependency and only one (the container).

Look how many test cases you had to add mocking the application into.

That's not an issue of this pr, but reveals a general problem. I had to add this code because of stuff like https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/CMS/Pathway/Pathway.php#L70.

JFactory::getContainer() is the bridging layer

For me it is absolutely not clear where the path should go to then. How should a component access the services then in the future? Like it is now, it is clear, it should go trough the app (like getDocument, getLanguage or whatever else). The plan is to inject the app or even the container.

The plan is to inject the app or even the container.

No. No. No. No. No.

You should not inject the application or container to get services. You inject the services you need. If you are injecting the application or container to use them as a service locator in a class chain where you have full dependency injection available to you, you are doing it wrong. Period.

JFactory::getContainer() ONLY exists to bridge the gap in our various service locating factory methods (some are static getInstance(), others are in a OOP API but don't have the right dependencies injected right now). JFactory::getContainer() or JFactory::getApplication()->getContainer() do not exist to liberally use our container as a service locator for global services. Except for where we have to use it for service locating functionality, it should not be abused as a service locator. Otherwise we've really only replaced one service locator (JFactory) with another (the DI container) and we've solved nothing of our underlying architecture issues.

Like it is now, it is clear, it should go trough the app (like getDocument, getLanguage or whatever else). The plan is to inject the app or even the container.

For things that are part of the application (document, language, and session), it is perfectly fine to have the application injected for that. As long as you're also using the application for other things. If you're just using the document or language in a class, you should be injecting those classes directly. The global services that are present in JFactory should also eventually be present in the container. So it shouldn't matter if your constructor typehints JDocument and you get the right instance immediately or CMSApplicationInterface and you have to call $app->getDocument() to get the document, both should be the same, and the former should be used as THAT is proper dependency injection; the latter is service location and creates extra dependencies that must be mocked to get the desired behavior.

No. No. No. No. No.

You got me wrong. For sure you should inject the services itself in a perfect world. But for now we inject the application into the dispatcher. That's what I meant. It is absolutely clear that injecting the app is not ideal. But at least we work with an object and not a static JFactory call.

do not exist to liberally use our container as a service locator for global services

First you want to use the container as global service locator and now not. Sorry but now I lost you.
I just don't see where it should go to how it is now. Adding more dependencies to JFactory is for me the wrong approach.

Adding more dependencies to JFactory is for me the wrong approach.

It's a transient dependency, the same as the classmapping stuff. It exists to create a compatibility layer. Take JDocument::getInstance() as an example, to transition that to using the document factory without creating a hard B/C break, the getInstance() method needs to be able to get to the container to get the factory it needs. So in that aspect, it needs to be able to use the container as a service locator, but once that getInstance() method goes away, the only replacement is to use the document factory class (which you either get by having it dependency injected into your class, or since it's a dependency-less class, you can just create a new instance for yourself).

For other places right now where we have a new $class(); type construct, I added support for those locations to use the container as a locator. This allows developers to overload services by declaring them as services in the container, instead of having to manipulate the autoloader and overload core classes. It's less than ideal, but is a step forward in improving the flexibility of the platform. The key point there though is those methods are already acting as a service locator/provider of sorts by creating a new class (service), so adding the option to look for it in the container is an extension of that logic.

What we should not be doing is replacing JFactory::getDbo() with JFactory::getApplication()->getContainer()->get('db');. When we start introducing that style of code, we've screwed up royally.

First you want to use the container as global service locator and now not.

The right way to do things is nothing is aware of the container but the front controller and the container's service providers. We can't get there very quickly. So the compromise I made is making the container available as a service locator through JFactory and ONLY using it as a service locator in parts of the architecture where we are already building or locating services (i.e. many of the getInstance() calls or places where we have new $class(); logic). It is not a replacement for any other JFactory call. And the presence of it in JFactory should only be considered a temporary measure because without it either we create hard system couplings to another object (as this PR has done) or we create hard B/C breaks by doing pure dependency injection and removing support for existing practices without any sort of migration path.

It is absolutely clear that injecting the app is not ideal. But at least we work with an object and not a static JFactory call.

Except what this PR is doing is replacing one static JFactory call with another static JFactory call and requires additional effort to make that path actually work. So for places where we're still making the static method call, there is no benefit whatsoever. In places where the code is already receiving the application through dependency injection or fetching the application from JFactory, this changes nothing.

once that getInstance() method goes away, the only replacement is to use the document factory class

Ok, got that, then it makes sense in some way to have a static available container somewhere.

This allows developers to overload services

How do you ever want to get rid of that service locator functionality? For me this sounds like a permanent solution and not something like a migration layer. It feels like: We have now a container, lets make it the super global thing to work with.

So the compromise I made is making the container available as a service locator through JFactory and ONLY using it as a service locator in parts of the architecture where we are already building or locating services

Then this needs to be added clearly to the doc block of JFactory::getContainer(). I fear that this function will be abused otherwise.

this changes nothing

It changes that in the component stack we can work with the injected application and their services.

How do you ever want to get rid of that service locator functionality? For me this sounds like a permanent solution and not something like a migration layer. It feels like: We have now a container, lets make it the super global thing to work with.

Factories are allowed to be service locators. Some scan the filesystem, some assemble a class name based on a configuration, some retrieve something from the DI container by way of service location. While it's not exactly the most optimal thing, in this scenario it is an acceptable use of the DI container as a service locator. So maybe the migration plan for some of these scenarios is the classes become container aware to continue offering these features, there's plenty of time for discussion and review on that.

FWIW there are extensions in the ecosystem which have overloaded core classes in the past, because that was the only way to add their custom functionality. I don't think class overloading belongs as a supported feature of an API, but I do believe in being able to use the DI container to define a service which replaces a specific implementation with another compatible implementation (this is why you generally typehint APIs around a high level abstract class or an interface). So being able to access the container in these factories helps with that.

It changes that in the component stack we can work with the injected application and their services.

If you're already working with JFactory::getApplication(), there's nothing to change. You can use and abuse the DI container all day long. For the parts of our architecture which are not application aware and do not need the application, this is creating a new dependency. Similar to how in the tests you can mock an application by setting something to JFactory::$application, you can mock the container by setting your mock at JFactory::$container then any call to JFactory::getContainer() works with that mock, the same as calling JFactory::getApplication(). So really, all you are doing is making it harder to write tests by creating more dependencies in classes. At least what's there now can work with a default container that gets created internally if nothing is set, with this change now everything HAS to mock an application, the container, and what you're trying to get from the container (if you're testing on that code path).

HAS to mock an application, the container, and what you're trying to get from the container

If it needs an application then it needs to get mocked. Don't know what is wrong with that. This is not much more different than what we have now.

Actually the tests are working with the default container, which is not mocked. So this leads to a situation that tests are messing around with the default container. What is missing on every test case, where the tested code uses the container, the save and restore of the factory state anyway. What the actual changes revealed are the positions in the code where the states of JFactory are not restored correctly.

Mocking the stuff is fine. Yes, your changes expose the fact that we are not mocking every little thing in every test, that's already a problem in a lot of tests anyway. To me though, you're taking a sledgehammer to a nail; instead of fixing the problem that the container isn't being mocked, you're forcing an application to be mocked which requires a container to be mocked which requires the calls to the container to be mocked versus just mocking the container and its calls.

To me, you are making things more difficult to work with, from a practical code perspective as well as the test infrastructure. I do not agree with this PR philosophically and fundamentally. But as I am not the release lead, I will not make a decision on this PR. All I can do is voice an opinion that aspects of this PR are worsening things more than helping things.

It is more clear now how things do work. Actually the container of the application is only used as service locator and nothing more. How it is now, JFactory::getContainer() is used on too many places already. But the container is not used on the front controllers of the CMS apps, where it should in my eyes.
I'm willing to change my code and say that JFactory::getContainer() can be used in the getInstance functions, where we deprecate them at the same time. Also deprecate JFactory::getContainer() and clearly document it's purposes.

What I would also welcome is that we define how to access the service locator, I think JFactory::getContainer() is the wrong place for it. Because that is not only a temporary solution according to your statements, or?

Also deprecate JFactory::getContainer() and clearly document it's purposes.

Yes, let's do that. It should not be a permanent part of the API.

What I would also welcome is that we define how to access the service locator, I think JFactory::getContainer() is the wrong place for it. Because that is not only a temporary solution according to your statements, or?

It is a temporary solution in that it exists as part of the API to help with the transition from old to new. Same with the classmap.php file being a temporary solution to help transition from globally named classes to namespaced classes. It's not designed to be a permanent part of the API, but relying on it shouldn't be encouraged.

As for dealing with where it's used, some of that is going to be on us to decide, and some of it will be developers who provide system services making decisions (hopefully based on how we do things). As things get split into proper factories, those factories can decide if they need the container and declare themselves as such (i.e. https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/CMS/Toolbar/ContainerAwareToolbarFactory.php which supports service locator functionality but makes the container a required part of the class through the interface/trait, then on the other end of the spectrum https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/CMS/Menu/MenuFactory.php doesn't use the container to create objects because it's not really efficient to do that with an untyped and dynamic options array).

Every other thing becomes a case-by-case basis. Just looking at the first few library classes in this PR...

• Archive: Move to a factory service, make the default factory we use container aware
• Adapter: I actually want to deprecate JAdapter so I think I'd just leave this be
• Authentication: Make the event dispatcher a required argument in 5.0, leave the JFactory call in place during 4.x and emit a deprecation notice (similar to https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/CMS/Toolbar/Toolbar.php#L78)

As I understood it, the service locator stuff should replace the need to override classes. This is also only temporary? What would be then the end product?

As I understood it, the service locator stuff should replace the need to override classes.

Correct. Having the factories for creating stuff lets developers either add custom classes that fit the conventions our default factories use or allows them to inject a custom factory service into the container (as demonstrated in #16259) to introduce custom logic for creating objects. So there really should be no place for class overloading in extensions as we adapt our code to look things up from service factories and the container in general.

This is also only temporary? What would be then the end product?

Service overloading is a pretty common thing, but to use it most efficiently requires that we typehint against interfaces and high level abstract classes and make the code less resilient on a specific class' behaviors when you're dealing with library level code (obviously some parts of the code care about a specific implementation of an object more than others, i.e. in JDocument when you want the stylesheet renderer you probably wouldn't want a module renderer to come back). So as it relates to service overloading, for things that have shifted to using factories in the container, my demo code in that linked PR becomes the preferred way of adding your custom logic on top of the core conventions. A more real life example can be found in the Framework website's service providers where https://github.com/joomla/framework.joomla.org/blob/master/src/Service/DebugBarProvider.php#L59 is decorating the default Twig_Environment class with a decorated variant to pull some debug information out when the debug mode is enabled.

That part I did understand. The question was how does an extension or whatever has the need to overload a service actually doing it? How does it set on which container when JFactory::getContainer is deprecated? Thats where I'm struggling.

With my proposal, the container of the application is the service locator.

JFactory::getApplication()->getContainer() and JFactory::getContainer() should reference the same object and if they aren't then we've screwed up royally (same with JFactory::getApplication()->getLanguage() and JFactory::getLanguage()). So in an absolute worst case scenario, you're fetching it from the application. We don't want people doing that though, in general any JFactory::getApplication()->getContainer() call should be seen as a code smell and indicate code needs to be refactored.

In theory you're going to be overloading services in plugins very early in the request (onAfterInitialise). Plugins are already able to be application aware. It shouldn't take much to also support them being container aware, then they just do $this->getContainer() or $this->container and do their business.

And again, we don't want the container to be abused as a service locator except for a very few special cases. Those cases already exist in the code basically (as far as a concept goes, not saying we've successfully deprecated all getInstance methods where it's practical to do so in favor service factories). So if we are adding more calls to the container that aren't in a getInstance style method, we need to be questioning what we're doing wrong.

Now it starts to make sense how it actually is. But from the current code base it is not obvious for me.

@mbabker can you make a pr with the deprecation notices? So we have it correctly and I can adjust this pr then accordingly.

I reverted the code which removes the JFactory::getContainer code (which I still thin is the wrong approach) and restored the unit tests. I'v also added some deprecate annotations on the getInstance functions which do use a factory already.
@wilsonge and @mbabker can you guys please review it again. Thanks.

which I still thin is the wrong approach

If you can come up with an approach that makes the container accessible to our existing static factory methods and allows them to proxy forward into the preferred APIs without introducing hard B/C breaks, we can re-evaluate. The biggest thing that is driving the need to make the container available through the factory is to be able to transparently proxy existing code to the preferred way of working with the container and object oriented service factories. Otherwise the only other option to satisfy your very open desire to remove every static from the code is to just hard break all the things and that's not going to make a lot of people happy.

Honestly, we screwed up making a getter part of ContainerAwareInterface. The container is a class' implementation detail and we really should remove it from the interface at a minimum (possibly the trait too, or make it a protected method so it can't be abused outside the inherited class chain). We do that, the CMS is left with one true way of accessing the container until the full application stack supports running in an entirely dependency injected way, and that's through our central factory.

This is the part we as the developers building 4.0 have to work to make clear. Yes, we are adding less-than-optimal things to our API. We are doing so for a very explicit reason. Not doing so would either hinder us from doing the things we are doing or cause there to be some very hard B/C breaks at whatever point we decided to just cut the cord on the "legacy" structure. So we have to steer the development community. The folks who grasp these concepts will understand why what we're doing is less than optimal and will probably be in a better spot to just jump straight to the desired end state (everything DI driven, don't touch the container except for registering services, etc.). For the folks who are just copying core because if it's good enough for us it's good enough for them, they are the ones that we have to make sure we are building things in the preferred way and that means limiting Factory::getContainer() calls to the absolute bare minimum with it made clear somewhere why we are making the call.

If you can come up with an approach that makes the container accessible

At least I would make it available in another class like \Joomla\CMS\Container\ContainerFactory which is deprecated and contains the container set up code and getContainer.

That part's just aesthetics. What's the real difference between being in the centralized factory and it being a random class elsewhere? It's still going to be global singleton and static methods.

I would like to get rid of JFactory, so adding more stuff to it would be a wrong signal. Having it in a new class would make it more clear. We can even call it InternalContainerFactory to make it cristal clear that it is only a transition class. But this can be done on a later point if required.
I did your reccomendations, so @wilsonge what do you think, ready to merge or still some concerns?

Closing this one in favor of smaller pr's. Guess we stick with JFactory::getContainer()so there is no need to have an effort to move it into it's own file.