Replies: 13 comments 24 replies
-
As a self-identified person who sometimes writes docs and contributes to core but doesn't build extensions properly i have opinions too to make everyone mad :D
I think this is by design? Containers effectively store a singleton and because we can have multiple models loaded on the same page (e.g. content article and archived article module) you have to have the factory in the DIC and the factory will then create it when you need it
OK I'm going to start here by playing a bit of devils advocate. If your booting something through a DI container and you REQUIRE it to available at model boot through setFoodbar - why isn't it a constructor argument (which could then be injected by the DIC) rather than going through the setter? I know @laoneo has started this for a while (e.g. with the dbaware stuff) so it's not strictly a blame on any extension when core started it. But I've said it before and I'll say it again - i'm really unsure about it in these DIC scenarios. I mean the biggest argument against is just the fact that from a b/c perspective it get's really hard the day core ever does a major revamp of it's constructor args for the MVC classes (and maybe that makes it a 5.0 thing - but doesn't mean that setters are a good bodge either to keep b/c at all costs).
OK Makes sense - a protected getter method sounds reasonable (don't really want to allow internal overrides)
I mean we can point to a method within the class I guess - anything with an executable should work right?
Why would you think that. In theory everything in the library folder (therefore this) is subject to the b/c promises no? I guess the bigger problem is all the constant injections through into the factory as we add more and more setters?
given you've just overriden the entire service provider - is it? in theory you've just made it more stable surely?
I don't like it (surprise surprise). If such a class needs heavy initialisation - to me it means that it should be a separate service provider that the mvcfactory provider depends on. Not that all that logic should happen in the mvcfactory (maybe I've just looked at too much java recently too :D )
Bearing in mind 80% of the work (once you remove deprecated code) from the factory is just initializing the mvc classes (bearing in mind it has to deal with whatever namespaces - whereas for any given component you do know the namespace and can hardcode if you wanted) - remaining 20% being the setters. I'll argue that for a custom component with tonnes of setters and a custom factory too - that potentially you should just be using your own factory and service provider and just implement MVCFactoryInterface? Like there's gotta be a line where your supposed to implement the interface and do it yourself instead of core anyhow from a practical perspective. Not 100% on exactly where it is but... |
Beta Was this translation helpful? Give feedback.
-
You didn't get me at all. I am not talking about how you get "a" service from the container, I understand how DI works. I am talking about the fact that the MVCFactory needs to be pushed concrete instances of all services it will subsequently be pushing to the MVC objects. However, this is not always a good idea. For example, I may have a service which needs to do heavy I/O or use up a lot of memory to initialise e.g. because it needs to load and parse a large JWT. Having it initialise every time the component's MVCFactory initialises is killing performance. This happens every time the component is booted which is NOT ONLY when the component is loading.
That was my question too. The answer is that Joomla does not create MVC objects using the DI Container That's exactly why I am saying that Joomla's DIC is actually a service locator, not a DIC. It's only ever used as a service locator. Walks like a duck, talks like a duck, gotta be a duck.
PREACH, BROTHER!
Confession: I wrote the following three times. I was going to say it's not possible then I realised it is, then I realised it's even simpler. Thank you for being my rubber ducky :) We only have one problem: the model has a pesky $config array we can't get rid of for the next 1 or 2 major releases. The way If we could indeed do that we can change the base MVC objects' constructor signature. For example the BaseDatabaseModel would be public function __construct($config = array(), ?MVCFactoryInterface $factory = null, ?DatabaseInterface $db = null, ?DispatcherInterface $dispatcher = null, ?User $currentUser = null, ?CacheControllerFactoryInterface $cacheFactory = null) then a descendant class with an overridden constructor like this public function __construct($config = array(), ?MVCFactoryInterface $factory = null) can still work AS LONG AS the developer calls the parent constructor with NULL arguments for all dependencies AND implements the interfaces (and uses the traits) already in place in 4.2. The MVCFactory would create the model object by doing // Clone because the Factory needs to return a new instance whenever it's called
$model = clone $this->getContainer()->buildCMSObject($className, $config); instead of $model = new $className($config, $this); Dude, this is doable. The code changes can even be implemented as Rector rules.
Yup
The code of the MVCFactory service changed drastically in 4.1 and especially 4.2 since it's being pushed more and more services. Since it cannot be extended from it has to be duplicated. Therefore anyone who did exactly that in 4.1 would have a broken component in 4.2 which, as far as I am concerned, is a b/c break.
Yes, precisely. However, if we construct the MVCFactory object through the DI Container's very handy That said, having the MVCFactory construct objects through the DI Container requires passing the DI Container / Service Locator to the MVCFactory object. Maybe it would make more sense pulling those service when needed, unless they have already been pushed to it. This way we don't need to wait for a new major version to add a service to the MVCFactory, we can maintain b/c between two subsequent major versions AND it's still testable as we can push mocks in Unit Tests (if they we push mocks they will be used instead of pulling these services from the DI Container). It's not clean but it's practical.
No, it's not. Let's say Joomla 4.3 adds a UserFactory service to the MVCFactory. My MVCFactory service code is unaware of that change. When my component tries to run on Joomla 4.3 it will break because the UserFactory service is not set. This means that I need to perpetually check if something changed in the core code and copy it over to my component, all because I wanted to use a custom service and do it the Joomla Way. What I'd do instead is override my component's extension class to have a static member which returns the component's DIC and use it to get the DIC in my model and use the DIC to instantiate my service. Not clean, I am pulling dependencies instead of pushing them BUT my code would work on 4.3 and 5.0 no matter what changes you make to the MVCFactory. Meaning, the only way for me to use Joomla without my component breaking in every minor Joomla version update is to NOT use Joomla's MVC as Joomla intended. This is the maddening part.
It's WAY TOO LATE to change that now. You'd be telling all 3PDs like me who did the right thing and used the Joomla 4 MVC that they need to rewrite their components from scratch to instantiate MVC classes through the DI Container instead of Joomla's MVCFactory. Sorry, I just spent a year of 10 hour days during which I could not implement new features. If I have to do that all over again I might just as well go out of business! Also, if I am to just use the DI Container to instantiate MVC classes, does it really make all that much of a difference if I had a helper called MVCFactory to do that for me? |
Beta Was this translation helpful? Give feedback.
-
First of all I want to clarify, that Joomla 4.0 is the first iteration of using a DI container to build extensions, so I wanted to make it as simple as possible by not replacing one magic system with another one. Sure you write every time similar code in the provider.php file, but it is clear as hell now and the dev knows what he is doing when writing the service provider. It was a huge effort to even come to that point, now it is time for the next step and optimize things. So I'm grateful we are discussing it now.
If this is really needed, as I would rather fix the class which does the heavy instantiation, then we probably need a concept similar to symfonies proxy approach. But I would rather prefer to make constructors as lightweight as possible.
You can use the
Go ahead and create some composition service providers. You are not the first one who brings this on the table. The MVCFactory class itself was built at that time that way, to be as much BC as possible with the legacy approach. So it would be worth a try, how it would look like with a reference to a DI container. What bothers me most on this discussion is that you guys want to shift to constructor injection. Having setters makes adding new service much easier in regards of BC. Good example was the MVCFactory in the controller class which brought up some troubles. Symfony has implemented autowiring for setters, so this is possible as well, when we want to go the way to auto build objects. |
Beta Was this translation helpful? Give feedback.
-
No matter how lightweight they are, at some point we are instantiating too many objects. remember that we are booting each component in the frontend to do routing. Beyond performance measured in milliseconds there's also memory performance. I am worried that moving our components to services would end up bloating the memory usage of the Joomla site.
So, you're saying that we should basically be using decorators for the custom MVCFactory instead? I am mostly concerned because the MVCFactory is constructed twice: once implicitly by the ComponentDispatcherFactory and once explicitly when setting it to the component's extension object in the component's service provider. I guess extending the service should happen right after registering the MVCFactory service provider to avoid a “lame duck” factory?
Fair enough. I will give it a go.
Look, I can see pros and cons in both approaches. I do think that using setters makes it easier for BC. I do not think it's the easier approach for creating objects when we already have a DI Container. I would be okay with it and shut up if the decision was “Joomla is using its DI Container as a glorified Service Locator and nothing more”. If that's the case, sure, setters are the only reasonable way to construct objects and good for managing b/c, so no contest from me. |
Beta Was this translation helpful? Give feedback.
-
@laoneo Unfortunately, extending the service does not work at all. Here's the thing. Our decorated MVCFactory can only operate on objects constructed by the The problem lies with instantiating the Controller and the fact that the Controller object itself is a proxy to the MVCFactory's createModel and createView methods. Our decorated class' When the Controller calls its As a result, our component breaks 🤦🏽♂️ The controller does NOT have a You can see this problem in https://github.com/nikosdion/com_example/commit/06f43fde850b79673500697e41c4945602fa3415 What if I created a custom subclass of |
Beta Was this translation helpful? Give feedback.
-
True, would the mentioned setMVCFactory function solve it? |
Beta Was this translation helpful? Give feedback.
-
@laoneo Yes, it would. I would also propose a base MVCFactoryWrapper class developers can extend from so that:
Would you be open to that idea? Optional, further idea. If we ever write that auto–magic service provider we were talking about today we could prescribe the normative location of an MVCFactory decorator in the component's source tree so that this extended service is registered automatically, without the developer having to write a lot of boilerplate code in |
Beta Was this translation helpful? Give feedback.
-
Sure go ahead. As already said, the current implementation is the simplest possible solution. The architecture allows us now to improve it into a direction which makes the life of us 3rd party devs more convenient. |
Beta Was this translation helpful? Give feedback.
-
This was converted from an issue to a discussion. Should this be moved back to issues? |
Beta Was this translation helpful? Give feedback.
-
OMG, why was this converted into a discussion?! Are you people for real?! I specifically created an issue, not a discussion, for this matter BECAUSE IT IS AN ARCHITECTURAL BUG. @laoneo was adamant that it'd be possible to push custom services to Models, I demonstrated it cannot happen (with code!), I proposed a solution, we discussed it a lot, proposed a different solution, and wrote a PR — and code to use it and prove it does work. In other words, this is an architectural bug, and I opened an architectural PR once it was adequately discussed: #38597 Architectural PRs are not kindergarten level bugs. It's not “two successful tests by any two random people on the Internet (who could also be sock puppet accounts of the PR's author…) and we merge them”. These get reviewed by Joomla's architectural team. There is a long lead time between issue, PR, and merge — usually months, if not years. Closing the issue or downgrading it into a discussion is a disservice to Joomla first and foremost, as all it accomplishes is that the architectural bug will not be solved. Considering that we only have a window of once in 2 years and then it's another 4 years before this code can be widely used you are keeping Joomla back for a decade at a time! Downgrading the issue reporting a big architectural bug to an "Ideas" discussion is also a big “F... you” to the person who discovered it, documented it, and addressed it — that would be me. You are telling me through your actions that Joomla does not care about its architecture and the time of people trying to help with it. Joomla's only differentiator is good architecture; take it away and it's dead. Joomla has a problem recruiting volunteers; tell them through your actions you don't value their time and they won't come. Furthermore, the issue had linear history of what was discussed, it was clear who was replying to whom. This is now a jumbled up mess which makes al of us look like we had some kind of stroke and replying to the ether. Discussing architecture needs a structured, linear conversation. The StackOverflow-style Discussions IS NOT MEANT TO BE USED TO HOLD A CONVERSATION, it is only meant FOR SUPPORT (HELPING PEOPLE). @bembelimen Why are people allowed to screw Joomla up like that and why are their commit rights not immediately revoked? |
Beta Was this translation helpful? Give feedback.
-
Hello @nikosdion , I hope we get it resolved asap. |
Beta Was this translation helpful? Give feedback.
-
@nikosdion , I apologize that this occurred. As @bembelimen said, we are addressing this and swift action is being taken. |
Beta Was this translation helpful? Give feedback.
-
as I have been tagged here, let me say something:
|
Beta Was this translation helpful? Give feedback.
-
EDIT NOVEMBER 20TH, 2022 by the author: Back in August I created an issue, NOT a discussion, and that was on purpose. Joomla promises that we can set up custom services in our MVC objects through the services provider. I discovered this is not true which means that this is an architectural bug. Architectural bugs have a long lead time before a PR lands. Converting them to discussions without asking the people involved only achieves one thing: these issues fall off our radar and will not be solved in the next major release. Therefore we are stuck with an architectural bug introduced in 2017 until at the very least the end of Joomla 5's life which will be October 2027. Congratulations, whoever stupidly pressed the button to convert the issue to a discussion. Do everyone a favour and surrender your commit rights to this repo. You have no idea what you are doing, dammit! End Edit.
While writing developer documentation I ran into a maddening problem which I feel needs to be solved sooner rather than later and possibly before 5.0. It will not only make 3PD's life easier, it will also sell them on the idea of having a DI Container / Service Locator in their components. Right now, most 3PDs perceive it as a liability than an asset and the more I work with it the more I tend to agree with them. Without further ado, let's get to the problem at hand: how in the name of Joomla can I use a custom service in a component's models?
Problem identified
I have a component, my very minimalist com_example.
I want to implement a custom service in my component which can be used by my Models. To demonstrate this I created a very simple
\Acme\Example\Administrator\Service\Foobar\Foobar
service.How do I do that? Hold on to your hats, it's gonna be a wild ride.
Part I: Creating and using a custom service
Code for part I of the wild ride: https://github.com/nikosdion/com_example/commit/573110f7209b53c7e25c784b0db7153b1135f4aa
First of all, I need to register my service to the component's DI Container (which is really a service locator but let's not split hairs, I don't have many to spare anyway). So, for starters, I need to create a service provider
\Acme\Example\Administrator\Provider\Foobar
which implements the\Joomla\DI\ServiceProviderInterface
and modify myservices.php
to register it. Cool.I need to know which objects are aware of the existence of this service so let me also create a
\Acme\Example\Administrator\Service\Foobar\FoobarAware
interface and a corresponding Trait to implement it,\Acme\Example\Administrator\Service\Foobar\FoobarAwareTrait
.My model,
\Acme\Example\Administrator\Model\FoobarModel
, can now implement the aforementioned Interface and use the aforementioned Trait. Very cool. However, since nothing has set my custom service to my Model and my Model does not have access to the component'sservice locatorDI Container my component is broken. 😞Part II: A custom MVCFactory
You can see the code for Part II here: https://github.com/nikosdion/com_example/commit/8acf819931893265a1123155ea2c39a201613258
My Model is created by the MVCFactory object the component knows about. Therefore I need to create a custom MVCFactory which checks if the created Model object implements the
\Acme\Example\Administrator\Service\Foobar\FoobarAware
interface and callsetFoobar
to push the service. For this reason we create\Acme\Example\Administrator\Service\MVCFactory
.I have two problems now. One: I need to tell Joomla to use my custom MVCFactory instead of
\Joomla\CMS\MVC\Factory\MVCFactory
. Two, when the custom MVCFactory is created I need to pass it an instance of my custom service.IDENTIFIED PROBLEM 1: AN MVCFactory CAN ONLY PUSH ALREADY INSTANTIATED SERVICES (YOU CANNOT DO LATE / ON-DEMAND INSTANTIATION).
But how can I tell Joomla to use my custom MVCFactory? I need to substitute the
\Joomla\CMS\Extension\Service\Provider\MVCFactory
service provider. However, trying to extend that class does not work because a. it has a private property for the namespace and b. its register method is registering a closure which is defined inline in said method. Therefore I have to copy the entire code of\Joomla\CMS\Extension\Service\Provider\MVCFactory
into my custom\Acme\Example\Administrator\Provider\MVCFactory
service provider even though I only really needed two changes: 1. using a different class name to instantiate the MVCFactory and 2. pass a custom service to the MVCFactory.Here's the insanity of this approach.
First of all, this code is WET (Write Everything Twice), not DRY (Don't Repeat Yourself) which is doubleplusungood.
Second, the code inside
\Joomla\CMS\Extension\Service\Provider\MVCFactory
is considered “internals” and may change anytime in any minor or patch release without it being considered a b/c break. Therefore I need to check that code in every patch release of Joomla and update my extension which will otherwise might be broken. I went through all that trouble to do things the One True Joomla Way™ and all I got for my trouble is an unstable component which randomly breaks when Joomla is updated.IDENTIFIED PROBLEM 2: HAVING TO COPY THE ENTIRE CODE OF
\Joomla\CMS\Extension\Service\Provider\MVCFactory
IN THIRD PARTY COMPONENTS IS UNMAINTAINABLE.Proposed solution
I don't (currently) have a definitive solution for the first problem. See “open questions” below.
For the second problem I propose refactoring the
\Joomla\CMS\Extension\Service\Provider\MVCFactory
provider.The
namespace
property should be protected, not private.The provider closure should be a protected method instead of an inline closure. Overriding it allows a 3PD to register custom services to the factory object.
The if-block which returns the factory object should be a protected method. A 3PD can override it to return their own, custom MVCFactory object.
Open questions
Regarding the first problem and considering that the component's DI Container is in fact a service locator I'd propose passing it to the MVCFactory object i.e. the
\Joomla\CMS\MVC\Factory\MVCFactory
should implement\Joomla\DI\ContainerAwareInterface
and use the\Joomla\DI\ContainerAwareTrait
.In this case we do not need to pass a custom service to the MVCProvider object. In our example we could instead implement its
getFoobar
method as:This is very important if the custom services implemented by the component require some heavy initialisation, something VERY likely to happen in 3PD code.
Further to that, this makes creating custom MVCFactory instances easier, without having to massively refactor the
\Joomla\CMS\Extension\Service\Provider\MVCFactory
provider. Instead, the if-block which figures out the class name would be first looking for an MVCFactory class in the component's namespace for the application side we are in, falling back to the custom backend MVCFactory for other CMSApplication types (e.g. CLI) and if all else fails fall back to Joomla's default MVCFactory and ApiMVCFactory classes.Such a change would eliminate the boilerplate code a 3PD needs to write in the same way no boilerplate is needed for a custom component Dispatcher.
Tagging @nibra @HLeithner @laoneo and @wilsonge for feedback.
Beta Was this translation helpful? Give feedback.
All reactions