User tests: Successful: Unsuccessful:
OK So I had a bit of time on a train this morning and the stuff in #38926 prompted me to have a think about what would actually be involved in removing the View package's CMSObject dependencies and also moving it towards a better semblance of DI whilst we were at it for a v5.0 package by removing the config object in favour of child classes directly setting things in their own constructors - none of these properties are actually things that the controller really cares about. Finally I've removed the concept of helper classes as I can't see any use of these in core for like 10 years or more.
This is purely PoC for discussion. I'm know the branch tests will fail because I've not made the relevant changes to the MVCFactory to build the view with the new constructor. This is purely about opening a conversation with the aim of requirements gathering for what we do and don't need in the other PR to get us to this as a proposed end state. Some of this stuff will end up being backported into the 4.x series to give us b/c, docs will be needed in the manual. Again this is a PoC for discussion not something intended to be merged today or tomorrow.
I've been reasonably aggressive with my constructor refactoring here. I'm sure I'm going to get shouted down a bit by @nikosdion to be more practical and that's fine. I completely accept there will need to be revisions based on this (please don't kill me too much :D )
Status | New | ⇒ | Pending |
Category | ⇒ | Libraries |
First thanks for the work.
One problem is that this have to be in a b/c way for j3/4 extensions, which doesn't look like to be now.
I will start by saying that we cannot change the constructor in 5.0 since the production leadership has promised b/c with Joomla 4.
You can change the constructor easily without any b/c break, something like this https://3v4l.org/H5ck7
<?php
class Document {
}
class a {
function __construct(array $config) {
}
}
class b extends a {
protected Document $document;
function __construct($document) {
if (is_array($document)) {
// deprecation warning
$this->document = new Document; // get from factory
// do config stuff
return;
}
$this->document = $document;
}
}
$x = new b(new Document);
var_dump($x);
Looking at the path, we have way too many pathes we are looking at, removing most of the would make sense and maybe replace them with a function call where component creators can define the correct path them self. Including a setPath function, this would solve the use case of backend and frontend using the same view with different layout path.
The Application should not be needed in the view, for my understanding we should have as less super classes as possible in an class. Which means if something is needed it should be injected by the controller that creates the view.
Changing the document within the view sounds wrong shouldn't this be decided by the controller and the controller should injected the needed document?
Looking at the path, we have way too many pathes we are looking at, removing most of the would make sense and maybe replace them with a function call where component creators can define the correct path them self.
I agree. That's basically what I said. Have a default implementation which uses the application and the option implemented in a Trait. If the developer wishes to do something else, they can substitute it with something different.
Including a setPath function, this would solve the use case of backend and frontend using the same view with different layout path
Yes. It also solves the problem of a 3PD wanting to use a core Joomla backend view in the frontend. For example, I want a user picker in the frontend in my helpdesk software. Joomla does not provide one. I have a frontend View in my component which goes through the backend view template for com_user's modal user selection page and which extends the backend com_user's HtmlView. I solve the view template path issue with a judicious use of path management, exactly as you described!
The Application should not be needed in the view, for my understanding we should have as less super classes as possible in an class.
You are wrong.
In the frontend we need at least three things which come from the application:
$app->getParams()
$app->input->getInt('Item', 0);
$app->getMenu()->getActive()
In many cases we need access to the Session and Global Configuration parameters (like the site's name). These also come from the application.
The alternative to that is to push everything the application can provide into the View. I am not sure it makes sense to do it like that and I am not absolutely certain it wouldn't cause any b/c breaks due to naming clashes.
Changing the document within the view sounds wrong shouldn't this be decided by the controller and the controller should injected the needed document?
No, the controller can not dictate the document. The document is determined by the application based on the format
URL parameter which comes from route parsing.
By the time you are rendering the component the Application object has already created a document. You cannot change it, at least not easily and not without breaking third party plugins. Believe me, I have done that and lived to tell you how much I regret having done that.
I will start by saying that we cannot change the constructor in 5.0 since the production leadership has promised b/c with Joomla 4. Refactoring the constructor breaks compatibility with J4 and would require massive changes; let's not start this discussion again. I will just say that we've promised that when releasing major version X we are promising that components written for the API of X-1 would continue working without any significant modification. Changing constructors does not help with that.
OK Fair enough. 5am wakeups make you forget about some of the newer stuff - especially now I'm not in the meetings to keep track. But even if I kept the config array in the signature - effectively with the factory to boot the view the config array is practically speaking pretty useless it's pretty hard to inject for a specific view now and I really don't see which of these properties should be injected from the controller anyhow.
It has also been made very clear that Joomla is not following the DI method of dependencies in the constructor. Instead, it uses interfaces, traits and DI on initialised objects — exactly because this sidesteps the b/c issues changing the constructors would create. So, you'd need a DocumentAware interface, a DocumentAwareTrait to implement the setDocument / getDocument methods and change the MVCFactory to push the document to the view.
I will still fight this for the rest of my life. But fine. I can add a setter/getter/trait whatever anyhow.
A CLI application could instantiate a SiteApplication to render an HTML page, convert it to PDF (e.g. through a headless Chrome browser) and send it by email. Does that sound like invoice creation to you? Because it sounds like invoice creation to me!
?
Moreover, a view is far more likely to need access to the application object so it can get site configuration information (e.g. site name, the site URL — especially when we have it always populate $live_site regardless of what's in configuration.php), the current menu item in the frontend and so on. All my HtmlView classes start with $app = Factory::getApplication() and it irks the living crap out of me.
So yeah i had considered this. But i think without https://github.com/joomla/joomla-cms/pull/38544/files aren't a lot of these still broken anyhow - because we still haven't properly fixed building URLs within the views (even after that PR i'm still not so sure - I assumed that was part of the justification behind the half implemented baseURL property in abstract view without having gone through git history https://github.com/joomla/joomla-cms/blob/4.2-dev/libraries/src/MVC/View/HtmlView.php#L165).
I also see various places in the models where we grabbed the application object - one was to grab the template object - but I half considered that as "should be injectable" in it's own right anyhow as a dedicated object. And then the only place I saw us actually getting properties out the application was here (need to do a deeper dive to confirm though) https://github.com/joomla/joomla-cms/pull/38931/files#diff-610ce01e216b006e1d1e4bc6f38eca81debd39639f1ea66322fe37ee1c132ea8R505-R519 and then began to wonder if it was just overkill.
You can ABSOLUTELY NOT remove the template path support and fall back to the deprecated JPATH_COMPONENT constant! First of all, the constant is deprecated. Second, there are valid cases where I might have two separate views which need to use common view template. For example, when I was implementing MFA with LoginGuard I was reusing the same view templates in the front- and backend. Finally, this is currently required for implementing HMVC. Joomla can do HMVC (fun fact: it could do that since at least 3.0!) BUT by default it fails because it only looks for view templates in JPATH_COMPONENT. I would actually refactor out the fallback to JPATH_COMPONENT with a call to a method, let's say getComponentBasePath(string $option, string $appType = 'site') which can be a trait and used everywhere we currently use JPATH_COMPONENT.
So to be clear JPATH_COMPONENT
is already the default. I'm purely just removing the argument from the config. If people inject it in the constructor through setPath it will still work pretty much the same as it did before. Happy to explore this further though. One of the things that would be good to pull in from @phproberto 's JLayout code is the view priority stuff (https://github.com/joomla/joomla-cms/blob/4.2-dev/libraries/src/Layout/FileLayout.php#L519-L564)
I disagree with giving the ListView properties default values in the property definition. I feel this is something to do in the constructor, possibly trying to use convention-over-configuration. For example, looking for and using something like COM_optionName_VIEW_viewName_TITLE for the toolbarTitle and icon-optionName for the icon if they are unset. This will remove some boilerplate from the view classes. I'd actually prefer if we used convention-over-configuration for the EmptyState page as well. Writing Joomla components should have a flow, not feel like undergoing a Soviet Union border crossing interview.
So some of this is already set in the constructor - most these places I changed the default from null to empty string so typehints worked but I could still check for empty in the constructor to set defaults (and therefore still allowing dev's to use their own constructors to overload).
Labels |
Added:
?
PR-5.0-dev
|
Category | Libraries | ⇒ | Libraries Unit Tests |
Ok so I got this branch working last night with a few tweaks to get things working (I know I’ve not fixed the constructor to be b/c yet but I wanted to see what worked and what didn’t). There is an extra adjustment to not have the category feed class extend from the html view class which is formerly did - as far as I could tell to be able to reuse the escape method once :)
As far as I can tell this largely works in core (didn’t try any extensions yet) with one exception. The backend use of JLayouts. We pass $this into the layout which has always been a mistake (I’ve personally tried to fix this as part of previous GSOC’s but it’s nearly impossible without killing b/c hard)
https://github.com/joomla/joomla-cms/blob/4.2-dev/layouts/joomla/edit/params.php#L31 We use the get method to grab these dynamic class properties in each view https://github.com/joomla/joomla-cms/blob/4.2-dev/administrator/components/com_content/tmpl/article/edit.php#L30%E2%80%A6L35
Aside from this total cluster **** as far as I could tell from a quick click around backend/frontend/feeds seemed to work as expected.
I will say this again. Having the View constructor expect a document object instead of an array is a hard b/c break which cannot and must not happen in Joomla 5.0.
Not only does it break extensions written with the Joomla 3 API in mind (which production leadership promised not to remove in 5.x) but also extensions like mine written with the Joomla 4 API in mind. See https://github.com/akeeba/release-system/blob/development/component/backend/src/Mixin/ReusableModels.php#L89 My perfectly working Joomla 4 native MVC code will fail once this is merged and I will not be able to have the same package work in both 4.x and 5.x, contrary to the promises of the production leadership.
This is the kind of change which makes the production leadership look like compulsive liars and erodes trust in the project.
I would very much prefer it if we had a versioned core MVC API or designate specific major versions as “hard break”, as long as they are at least one major version after the last “hard break” (at the time of this writing this would mean the next “hard break” could be 6.0, not 5.0, as 4.0 was itself a pretty hard b/c break).
Again, this can be fixed by leaving the constructor as-is and using interfaces and traits. In fact, Joomla 5 could deprecate the constructor argument for AbstractView and provide a magic __get and __set method to proxy access to the no longer existing $document property to getDocument / setDocument implemented by a trait. Then I would be happy with this solution.
PS: Regarding the use of get() in layouts, I would propose a Trait (e.g. LegacyCMSObjectCompatibility
) which implements get(), set() and getProperties() the same way CMSObject does but also logging a deprecated notice when it is accessed. This would help developers work through deprecations during Joomla 5.x's lifetime.
I will say this again. Having the View constructor expect a document object instead of an array is a hard b/c break which cannot and must not happen in Joomla 5.0.
I will sort it. I just wanted to see if the CMSObject change affected anything for your PR last night. I understand it's something that needs to be addressed
PS: Regarding the use of get() in layouts, I would propose a Trait (e.g. LegacyCMSObjectCompatibility) which implements get(), set() and getProperties() the same way CMSObject does but also logging a deprecated notice when it is accessed. This would help developers work through deprecations during Joomla 5.x's lifetime.
So I can't do that in view because we have our extra get method inside the class already for the model proxy (that couldn't be removed obviously). So I can just add an extra check for class properties. But given those class properties inside tmpl are already dynamic - it's not really that straightforward as that really should be fixed as part of 5.0 too. Not sure for views specifically what getProperties would be used for - I could at least understand why set would be used by 3PD's even if core doesn't.
So I can't do that in view because we have our extra get method inside the class already for the model proxy (that couldn't be removed obviously)
Yes, you can. You can import a trait's method under a different name and use it in your overridden method in the concrete class instead of calling the no longer existing parent method. Maybe some code would help:
class AbstractView // extends, implements...
{
use LegacyCMSObjectCOmpatibilityTrait {
LegacyCMSObjectCOmpatibilityTrait::get as legacyCMSObjectGet;
}
// ...
public function get($property, $default = null)
{
// current implementation
// Degrade to CMSObject::get
// (replaces the line `return parent::get($property, $default);`
return $this->legacyCMSObjectGet($property, $default);
}
}
But given those class properties inside tmpl are already dynamic - it's not really that straightforward as that really should be fixed as part of 5.0 too.
Core view templates and layouts MUST be fixed. However, we have promised full b/c for third party code, therefore we still need get
to act as a proxy for public properties. Yeah, I know. I am not a fun either. But such is the nature of b/c and mass distributed software.
Sorry forgot you could pull in traits with an alias. thankyou. will sort some updates including constructor this evening.
Status | Pending | ⇒ | Fixed in Code Base |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2022-10-11 17:06:06 |
Closed_By | ⇒ | wilsonge | |
Labels |
Added:
?
Removed: ? |
Wait, you merged this PR into 5.0-dev even though it breaks the backwards compatibility promise given by the production leadership just a month and a half ago?!
@HLeithner @bembelimen please take a look here. This does the exact opposite for b/c than what you guys promised in https://www.joomla.org/announcements/release-news/5868-joomla-5-panta-rhei-the-follow-up.html
I quote the very first bullet item from the top of your article:
Joomla 5 will not include breaking changes for templates and third-party extensions.
Yet, this PR immediately breaks b/c for everyone instantiating a View directly, something still supported in Joomla 4.0 and used by extensions using Joomla 3-style MVC.
So… are we blatantly admitting here that the word of the leadership isn't worth the bytes it's written in? WTF?!
Fity fk f**k. didn't mean to push to the joomla 5.0 branch....... reverted those commits and remaking this PR to continue the discussion.
Okey-dokey. Please be careful, you almost gave me a heart attack and I am now at an age where heart failure is a real risk, mate!
Not half the heart attack when I came here to comment that I'd pushed the constructor revert and then saw the word merged at the top :D
I will start by saying that we cannot change the constructor in 5.0 since the production leadership has promised b/c with Joomla 4. Refactoring the constructor breaks compatibility with J4 and would require massive changes; let's not start this discussion again. I will just say that we've promised that when releasing major version X we are promising that components written for the API of X-1 would continue working without any significant modification. Changing constructors does not help with that.
It has also been made very clear that Joomla is not following the DI method of dependencies in the constructor. Instead, it uses interfaces, traits and DI on initialised objects — exactly because this sidesteps the b/c issues changing the constructors would create.
So, you'd need a DocumentAware interface, a DocumentAwareTrait to implement the setDocument / getDocument methods and change the MVCFactory to push the document to the view.Actually, on second thought, we should just be pushing the application into the view.
The document is intrinsically linked to the application; you cannot render a document disconnected from the application. If someone needs to render a different document than the current application's document they can always create a new document instance with all its dependencies, then ask it to render itself and inspect the response object. Sounds insane? Not at all! A CLI application could instantiate a SiteApplication to render an HTML page, convert it to PDF (e.g. through a headless Chrome browser) and send it by email. Does that sound like invoice creation to you? Because it sounds like invoice creation to me!?
Moreover, a view is far more likely to need access to the application object so it can get site configuration information (e.g. site name, the site URL — especially when we have it always populate $live_site regardless of what's in
configuration.php
), the current menu item in the frontend and so on. All my HtmlView classes start with$app = Factory::getApplication()
and it irks the living crap out of me.I agree with the dispatchEvent refactoring. In fact, once we move into concrete events this should be made into a trait which can be used by views, models, controllers and tables.
You can't really remove the helper support until 6.0, see first paragraph.
You can ABSOLUTELY NOT remove the template path support and fall back to the deprecated
JPATH_COMPONENT
constant! First of all, the constant is deprecated. Second, there are valid cases where I might have two separate views which need to use common view template. For example, when I was implementing MFA with LoginGuard I was reusing the same view templates in the front- and backend. Finally, this is currently required for implementing HMVC. Joomla can do HMVC (fun fact: it could do that since at least 3.0!) BUT by default it fails because it only looks for view templates inJPATH_COMPONENT
. I would actually refactor out the fallback toJPATH_COMPONENT
with a call to a method, let's saygetComponentBasePath(string $option, string $appType = 'site')
which can be a trait and used everywhere we currently use JPATH_COMPONENT.I disagree with giving the ListView properties default values in the property definition. I feel this is something to do in the constructor, possibly trying to use convention-over-configuration. For example, looking for and using something like
COM_optionName_VIEW_viewName_TITLE
for the toolbarTitle andicon-optionName
for the icon if they are unset. This will remove some boilerplate from the view classes. I'd actually prefer if we used convention-over-configuration for the EmptyState page as well. Writing Joomla components should have a flow, not feel like undergoing a Soviet Union border crossing interview.Finally, I am unsure if core and 3PD code uses CMSObject features like get, set and getProperties — or the deprecated error handler code. But we'll take that discussion to #38926?