[#39327] - Ease the use of multiple models in one view
- Closed
- 13 Dec 2022
- Medium
- Build: 4.3-dev
- # 39327
- Diff
- framontb:4.3-dev-BaseController_setViewModels
User tests: Successful: Unsuccessful:
Summary of Changes
Changes to ease the use of multiple models in one view for custom (or core) components in Joomla 4.
By moving just three lines of code from BaseController->display()
to a new method BaseController->setViewModels(object $view),
you can easily assign the models you want to any of your views.
Everything works exactly the same. The method is called at the same point the lines were before.
But the diference is:
- now you can overwrite the new method in the
DisplayControllerof your custom component like this:
public function display($cachable = false, $urlparams = [])
{
return parent::display();
}
public function setViewModels(object $view)
{
parent::setViewModels($view);
$viewName = $view->getName();
// Push the Second model into the Foos view
if ($viewName == strtolower("Foos") && ($model = $this->getModel('Second')) ){
$view->setModel($model);
}
// Push the Third model into the Foos view
if ($viewName == strtolower("Foos") && ($model = $this->getModel('Third')) ){
$view->setModel($model);
}
}Here you see that you don't need to touch your DisplayController->display() method to assign the models.
Nor call to Factories. All you need is already in the DisplayController with minimum code.
You only overwrite the DisplayController->setViewModels(), call the parent (that assigns the default model as usual),
and assigns a couple of new models to the "Foos" view.
Now you only have to call it properly and easily in your view, like this:
public function display($tpl = null)
{
$this->msg = $this->get('Msg');
$this->msg2 = $this->get('Msg', 'Second');
$this->msg3 = $this->get('Msg', 'Third');
return parent::display($tpl);
}Here you see how to call the methods of the Second and Third Model.
Just like it is described here: Using multiple models in an MVC component
Only now, you don't need to care about custom display() methods in your views.
The default display() works perfectly fine.
Testing Instructions
I developed this little Component com_foos to test the changes.
You only need to apply the changes to your Joomla 4 installation and install the component as usual.
Actual result BEFORE applying this Pull Request
Break MVC model to call several models for one view, using factories called in helpers,
or need to rewrite your own DisplayController->display().
See How can I use two models in my custom component view in Joomla4?
Expected result AFTER applying this Pull Request
If you pick in the Backend > Component > COM_FOOS menu link, you will see:
Hello Foo from the model: DEFAULT
Hello FOOS from the ADMINISTRATOR model: SECOND
Hello FOOS from the ADMINISTRATOR model: THIRD
Each one of this messages, coming from a different model attached to the Foos View.
If you create a menu link in the FronEnd of the testing site, for the com_foos component as usual, you will see:
Hello FOO from the SITE model: DEFAULT
Hello FOO from the SITE model: SECOND
Hello FOOS from the ADMINISTRATOR model: THIRD
Each one of this messages, coming from a different model attached to the Foo View.
Note that the THIRD model is a backend model. You can assign it exactly the same way.
Link to documentations
Please select:
-
Documentation link for docs.joomla.org: Not specified yet
-
No documentation changes for docs.joomla.org needed
-
Pull Request link for manual.joomla.org:
-
No documentation changes for manual.joomla.org needed
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries |
| Labels |
Added:
PR-4.3-dev
|
||
A new developer manual has been set up. It will help developers to write extensions and update their extensions to J4. https://github.com/joomla/Manual
2 things
- I'm not so happy with the function name, because it doesn't "set" the model, it "add" a model, also the plural is a bit confusing especially as you don't add the $model to add in the function signature.
- the ViewInterface doesn't guarantee a
setModelfunction... @laoneo any objections on this? checking method exists? or required abstractView, or hope the best?
2 things
1. I'm not so happy with the function name, because it doesn't "set" the model, it "add" a model, also the plural is a bit confusing especially as you don't add the $model to add in the function signature. 2. the ViewInterface doesn't guarantee a `setModel` function... @laoneo any objections on this? checking method exists? or required abstractView, or hope the best?
- I guess it would end up in a
prepareor so function. - Probably only a check if a method
setModelexists and then set it. Otherwise it can be counted as BC break if all of the sudden anAbstractViewis required.
Probably only a check if a method setModel exists and then set it. Otherwise it can be counted as BC break if all of the sudden an AbstractView is required.
Just create a new interface. ViewModelInterface that extends from ViewInterface and contains the setModel() function. Change abstract view to implement it. Full b/c and typehinting will work too.
What I did:
I changed the name of the function from setViewModels() to the more clear prepareViewModel() as @HLeithner suggested.
Regarding the typehint, three options were suggested (at least):
- Typehint with
ViewInterfaceand check the existence of thesetModel()method (since the interface does not contain it). - No typehint the param, but declare it in the DocBlock (implementing as few changes as possible for the sake of BC)
- Just create a new interface
ViewModelInterfaceand change abstract view to implement it.
I opted for the second option because it seemed the simplest and least disruptive.
Hope everybody agree !
Just create a new interface. ViewModelInterface that extends from ViewInterface and contains the
setModel()function. Change abstract view to implement it. Full b/c and typehinting will work too.
That would be going backwards. It's long time to stop passing entire models into the views. Besides that, @laoneo's concern is that 3PDs could be doing something weird like creating a view with the required methods but without extending AbstractView. Introducing a new interface wouldn't help with this. In hindsight, this could have been avoided if proper type checks were added when ViewInterface was introduced..
If it was my call, I'd just close this. Anything that propagates model injection is a step backwards. For those who really need this, there's createView() method where dependencies are being injected that can be overridden. Yes, it will be weird having the main model injected afterwards but all of this should really be considered legacy code anyways.
That would be going backwards. It's long time to stop passing entire models into the views.
[...]
If it was my call, I'd just close this. Anything that propagates model injection is a step backwards.
Sorry about the question but, is it not inherent to Joomla MVC pattern to instantiate Model and View in the Controller and inject the Model in the View?
(at least, it seems is how it's working now - I don't know if there is a pattern's mayor change going on here).
Yes, unfortunately, that's how it still works. But this concept dates back to the early days of 1.5 and it's one of the many things in the MVC stack that need to go away. That can't happen if people keep introducing new features that depend on this ancient code. Although it also depends on the powers that be. Joomla had taken a step once in the right direction by introducing the "new MVC" with single task controllers. It was deprecated, decoupled and shamelessly renamed to "legacy MVC" despite the actual legacy MVC layer still being in the CMS.
That would be going backwards. It's long time to stop passing entire models into the views. Besides that, @laoneo's concern is that 3PDs could be doing something weird like creating a view with the required methods but without extending AbstractView. Introducing a new interface wouldn't help with this. In hindsight, this could have been avoided if proper type checks were added when ViewInterface was introduced.
Can you explain a bit more why? Like I see this as a relatively standard implementation of MVVM and I don't think that's considered an anti-pattern or anything like that. It would be nicer to have better separation between that View class and the tmpl folder (which is partly why we considered moving core to JLayouts in 4.0 - but ended up ditching it as a too large b/c break for templates).
Is your expectation that the controller should retrieve data from the model and inject it into the view? Or something else? Even if you consider a replacement the reality is the transition for this is going to be long and helping people migrate isn't necessarily a bad thing anyhow - if you wanted to inject data from the controller you'd still need a hook in a similar place to facilitate that (albeit with a different method signature).
I admit I'm a bit lost here. Shouldn't someone be posting about what is the new pattern @SharkyKZ is talking about? Which core components follow this new pattern? Where is it implemented? (if it is) Or if there is any documentation about it?
When or how the old MVC patter will be deprecated?
I'm not really up to date on the latest news.
Is your expectation that the controller should retrieve data from the model and inject it into the view?
Pretty much, yes. I realize there are differing opinions about this. This is just my side. From what I can gather, the concept of injecting models into the view comes from GUI applications where updating the model updates the view. This does not seem to apply to web applications. Meanwhile, having the controller retrieve the data has benefits like allowing caching the data and not just the entire view output.
That aside, the controller currently is still too light. It does not even manipulate the model, instead the model directly reads request data. Another thing that needs to go away. There are even some instances of models setting session data and performing redirects..
It would be nicer to have better separation between that View class and the tmpl folder (which is partly why we considered moving core to JLayouts in 4.0 - but ended up ditching it as a too large b/c break for templates).
This is sensible. As long as the entire view is not injected into the layout as is currently done in so many places. Also consider changing the view to return the rendered string instead of outputting it. Ultimately, the rendering of the entire document should be delegated to the component. But this is for a very distant future..
if you wanted to inject data from the controller you'd still need a hook in a similar place to facilitate that
Rename the method to something more generic that doesn't referencemodeland call it a day?
Where is it implemented? (if it is)
This is not used in CMS. But you can always implement it in your own components. The framework's view package has also been refactored to follow this.
My point of view about this PR:
AFAIK the changes proposed by @SharkyKZ are not implemented yet, and are heavy changes that will be hard to implement. It requires implement the pattern in Core Base classes and refactor core components to be compliant. The proposed changes sounds very good, but in my (poor knowledge) view, such a thing would require a new major Joomla version to be in place and probably involvement of some Teams to assess (SA&S ?). I know there is a Joomla 5 development in progress. May be the proposal should go there?
Regarding this (humble) PR, I think it's a very little change, that would ease custom components developers life. It respects the underlying pattern in place in Joomla 4 currently. Being the alternative: use of helpers with factories that don't follow a pattern at all (for good or bad).
Unless there is a good detailed plan in place to make a huge change to Joomla 4, I don't see why feature development should stop.
Anyway it's not my decision (I don't know whose), so I will eagerly wait for news to come.
Please let me know !
anything new about this?
What should I do with this PR?
No need to move this to 5.0 as there's virtually no code change and potential method name conflicts are not covered by B/C policy. I'd still rename this to something that doesn't reference model.
From code perspective I do agree, but you change the logic of a function which is used in almost every component. Just a simple example. When templates or extensions do override the BaseController class in JLoader and an extension is using that new function, then it will crash. I know this is an edge case, but seen this already in the past.
It's not an edge case. It's a core hack and thus completely irrelevant.
It's not a core hack, it is something our API supports.
Then it's time to archive the repository because no further changes can be made. I swear, maintainers like to come and pretend they're doing something useful by posting extremely stupid, irrelevant time wasting comments. Just for the sake of doing it.
When I started with this PR I thought (erroneously) that it would be easy to publish, but somehow it seems a polemic one.
I don't understand very well the reason to rebase it to version 5.0 (I'm not an expert), but nevertheless I will do it.
Bad news is I didn't expect to change version, and I updated (merged) 4.3 changes , so I can't rebase (it happened to me before
I will close this PR and open a new one... I hope don't bother too much (again) !
@framontb No, you are not bothering us. You are proposing a good change, and we are very thankful for that. It's just like it is when a software is not made by a single person but by many, especially in an open source community. You have different people with different opinions. And with a mass distributed software you have to consider backwards compatibility aspects not only for the core but also for 3rd party stuff, and maybe sometimes our b/c policy is not as clear as it should be. I hope you are not disappointed or confused by all that, and I want to thank you a lot for your patience with us.
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2022-12-13 17:27:45 |
| Closed_By | ⇒ | framontb |
Documentation for manual.joomla.org is needed if this is accepted