Skip to content

[4.0][RFC] Child templates a-la wp, but better#30149

Closed
dgrammatiko wants to merge 17 commits intojoomla:4.0-devfrom
dgrammatiko:properlayouts
Closed

[4.0][RFC] Child templates a-la wp, but better#30149
dgrammatiko wants to merge 17 commits intojoomla:4.0-devfrom
dgrammatiko:properlayouts

Conversation

@dgrammatiko
Copy link
Contributor

@dgrammatiko dgrammatiko commented Jul 21, 2020

First and foremost what is proposed here is totally B/C and opt-in

The problem

Let's do a simple case study:
Imagine that you're a dev/designer/admin/etc and you decided to use a 3rd PD template. To make this more realistic let's say that you want to use @C-Lodder https://github.com/C-Lodder/joomla4-backend-template
Few days later you decide that some changes in one of the overrides will fit better your use case. You ego ahead and do the changes. Couple days later Joomla says that the template has an update and you go ahead and apply the update.

Ooops your overrides have been overridden

Ok, apart from this case also the core itself provides overrides https://github.com/joomla/joomla-cms/tree/4.0-dev/administrator/templates/atum/html which CANNOT be overridden by the user. So, to cut the story sort here, there are numerous MAJOR problems with the current templates:

  • User is not able to actually override everything (as the project is advertising)
  • There is no way any template to provide safe, self owned overrides and also allow the user to use/create their own ones
  • Templates behave differently than the rest extensions by placing their static assets in subfolders like css/js/images inside them. The rest of the extensions are using the media folder. Now this might sound weird but there are couple of things with this: security and consistency.

The solution

Provide child templates a-la wp.

Summary of Changes

Architectural decisions

The table template_styles gets 2 more fields, named parent and inherit. Both fields default to 0 which translates to the current behaviour. The field parent is a boolean and acts as the enabler for the new functionality and also as a preventer if you try to make a child out of a child template. You can only inherit from a real template. the field parent should be exposed to the manifest as <parent>1</parent> for the templates allowing childs.
The field inherit should not be exposed in the manifest, should be treated as reserved. The value in that column is populated by com_templates whenever a child is created. The value is an integer and eqautes to the template style id that the child used as a parent.

There are changes in the View of the MVC, so that the template will search for the given template then if not found and it's a child in the parent then the system
There are changes in all the SiteApplication, AdministratorApplication that basically do the same thing as the procedure in the MVC
Same goes for the HTMLHelper that the includesFile is patched to cascade in the right folders
And finally the modules Helper to pick the correct override (child/parent)

The templates that implement the new mode should have their static assets in the media folder either in templates/site/template_name or templates/admin/template_name

These changes were all that was needed in the core system for this new mode to be enabled

Testing Instructions

Download the intalling package at the end in the checks area (there is a download button) and install a copy of Joomla.

The installer should work as before.

Install the sampledata

Go to /index.php?option=com_templates&view=templates&client_id=0 and click on the cassiopeia link. There is a new button Create Child click it. You should get a newly child template with the name that you gave in the modal. Click on close to go back and then click on the newly created template. The folder should have only an xml file and 2 images. Great!

Go to the site template styles annd select the newly child template. Make the template default annd then go to the menu assignement and reassing all menus to this template. Check the front end.
Wow a template without any files...

Create some overrides, component, module, layout in the Cassiopeia template
Edit each of the overridden file and add some simple text, so you can understand which file was used for the rendering of the page
Check the front end that although you were editing the parent template the child got them as well. Looking good...

Repeat the creation of the same overrides but this time on the newly created child template. Put some text to idendify the changes.

That's all

What is not there yet

  • The com_templates is just monkey patched atm this can be demoed. It needs proper DRY code
  • The creation of the overrides is not aware of the parent template and always using the com/mod/etc
  • Patch the override notifier so it picks changes for both the parent template and the ext
  • Some conditionals need some clean up for consistency

Performance impact

Should be unnoticeable. The reason is that the core changes are using the template object (derives from the DB) which is cached, eg generated only once per lifecycle, and the additional payload are just few more conditionals. Static assets that have an option relative => true also get a couple more file lookups if the template is a child. All and all performance shouldn't be a negative factor here.

@wilsonge @mbabker @laoneo @ciar4n @C-Lodder can you some feedback here?

Documentation Changes Required

Yes!

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev RFC Request for Comment labels Jul 21, 2020
…outs

* '4.0-dev' of github.com:joomla/joomla-cms: (612 commits)
  [4.0] Smart Search: Fixing ordering, order direction and disabled button (joomla#29474)
  [4.0] Generate routed Modal links for iframes when not on the root (joomla#30007)
  [4.0] Get menu directly in com_tags menu route helper (joomla#30039)
  Remove collapse when resizing from mobile to desktop (joomla#30132)
  [4.0] Wrap component output in `main` element to make Cassiopeia more accessible (joomla#29870)
  [4.0] Webauthn gmp warning (joomla#29731)
  [4.0] Refactor to return early, remove if depths and throw NotAllowed (joomla#29694)
  [4.0] CLI help text (joomla#29811)
  Feature/draggable typo fixes (joomla#29987)
  [4.0] Removing unnecessary workaround in finder indexer (joomla#30037)
  [4.0] Optimizing Smart Search for larger content (joomla#30008)
  [4.0] Fix js ajax for pre update checker (joomla#29980)
  [4.0] Cassiopea: Fixing modals custom-select fields display (joomla#30097)
  [4.0][com_fields] Fix draggable sorting (joomla#30094)
  [4.0] Correct incorrect @return documentation (joomla#30092)
  [4.0] Menu items modal: adding missing filters (joomla#30087)
  short to long php open tags with echo (joomla#30089)
  Use new Toolbar (joomla#30085)
  [4.0] Center status/date created headers (joomla#29249)
  [4.0] Fix Cassiopea searchtools alignment in modals (joomla#30077)
  ...

# Conflicts:
#	administrator/components/com_templates/src/View/Template/HtmlView.php
#	installation/sql/postgresql/base.sql
#	libraries/src/Application/AdministratorApplication.php
#	libraries/src/Application/SiteApplication.php
@PhocaCz
Copy link
Contributor

PhocaCz commented Jul 21, 2020

Hi, can you please add more detailed information on how to test.

I don't understand this:

Download the intalling package at the end in the checks area (there is a download button) and install a copy of Joomla.

I cannot find any "checks area", so testing standard way (J!4 Dev, composer, npm, com_patchtester, ...), getting following error:

patchtester

Thank you, Jan

@dgrammatiko
Copy link
Contributor Author

The checks area refers to the tests at the bottom of the pr in the GitHub. There is a section called download with a link named details, clicking there will take you on a page with an installable package
3BD60268-B9E2-4B93-8EB5-AEC9D6329665

@PhocaCz
Copy link
Contributor

PhocaCz commented Jul 21, 2020

The checks area refers to the tests at the bottom of the pr in the GitHub. There is a section called download with a link named details, clicking there will take you on a page with an installable package

Thank you, now I understand.

@@ -41,11 +41,6 @@ module.exports.compile = (options, path) => {
}
} else {
files = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to remove the cassiopeia scss from the compile scripts or should it just have needed a change in the path

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The templates that implement the new mode should have their static assets in the media folder either in templates/site/template_name or templates/admin/template_name

All the generated static files have been moved to templates/site/cassiopeia. That's one of the changes proposed for this new mode. Therefore the current build tools work as expected as the new mode templates behave as the rest of the Joomla extensions. The security thing that I mentioned in the description is that if all your templates are implementing the new mode you could disable access to templates and any descending folders/files and less exposed files means more secure site...

@PhocaCz
Copy link
Contributor

PhocaCz commented Jul 21, 2020

Testing now,

I get some notices:


Notice
: Undefined property: stdClass::$inherits in
...libraries\src\Document\HtmlDocument.php
on line
812

Notice
: Undefined variable: template in
...libraries\src\Document\HtmlDocument.php
on line
833

Notice
: Trying to get property 'template' of non-object in
...libraries\src\Document\HtmlDocument.php
on line
833

Notice
: Undefined variable: template in
...libraries\src\Document\HtmlDocument.php
on line
834

Notice
: Trying to get property 'template' of non-object in
...libraries\src\Document\HtmlDocument.php
on line
834

Notice
: Undefined variable: template in
...libraries\src\Document\HtmlDocument.php
on line
835

Notice
: Trying to get property 'inherits' of non-object in
...libraries\src\Document\HtmlDocument.php
on line
835

Notice
: Undefined property: stdClass::$parent in
...libraries\src\HTML\HTMLHelper.php
on line
443

Notice
: Undefined property: stdClass::$inherits in
...libraries\src\Helper\ModuleHelper.php
on line
338


Notice
: Undefined property: stdClass::$inherits in
...libraries\src\Helper\ModuleHelper.php
on line
349

Notice: Undefined property: stdClass::$inherits in ...libraries\src\MVC\View\HtmlView.php on line 506

Notice: Undefined property: stdClass::$inherits in ...libraries\src\MVC\View\HtmlView.php on line 513

Not translated string: COM_TEMPLATES_TEMPLATE_FORK

After I created the child, I was not able to close the edit:

close

But child template was successfully created. Both parent and child can have different parameters, so it works OK for me.

I have one question, because I don't work with the following features, I would be interested in the difference between:

  • Copy template
  • Duplicate template style
  • Create Child

Thank you.

@dgrammatiko
Copy link
Contributor Author

I have one question, because I don't work with the following features, I would be interested in the difference between: Copy template/Duplicate template style/Create Child

Copy template copies Everything inside a template folder in another given name folder and applies some changes in the xml so basically it becomes a forked template. About Duplicate I have no clue and haven't read the code to see what it does (I'm guessing it's a copy without the changes in the xml?). The huge difference with the child template is that the new template inherits the executable .php files and also the static assets. This is extremely important for the template authors as their updates will never overwrite any of the files the end user tweaked.

Btw sorry about the notices, I really didn't spent much time on the com_templates component, just enough so someone could demo and get a feeling of the RFC. Also thanks for testing

Co-authored-by: Brian Teeman <brian@teeman.net>
@Fedik
Copy link
Member

Fedik commented Jul 22, 2020

I think this is not a bad idea.

A couple notes from me:

  • I would prefer to extend existing "Template Styles" feature (in similar way): User create a new style (as usual), and optionally can create a file override within this style (technically may be implemented similar as you did, maybe, not sure). But in the way as you made here it is also fine.
  • I would suggest to not to copy the XML, but create a very basic dummy, and update the template model: the model should load the params form from the parent XML when you open the params editing. This will be safe when the parent template will be updated and XML changed.
  • And fix all notices it makes a bad impression ;)

@brianteeman
Copy link
Contributor

From a technical semver perspective I would expect that this could not go into 4.0 as we are passed the point where such a major piece can be added and that it would have to be 4.1

That in itself might require some additional b/c changes to your proposal so there are no breaks with 4.0

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented Jul 22, 2020

@dgrammatiko yes @brianteeman is right, this can't go to 4.0 because Beta = feature freeze and no more b/c break

@joeforjoomla can you point me where is the B/C break here, I'm curious to see what I missed here because I think the first line of the description here calls for an non B/C breaking and opt-in behaviour?
If you can't do that please don't spread false information, it's not helping anyone

@joeforjoomla
Copy link
Contributor

joeforjoomla commented Jul 22, 2020

I can't say that for sure, i just had a quick look at the code and noticed a lot of changes in the template architecture... so i'm afraid that most probably there will be something that will break b/c

@dgrammatiko
Copy link
Contributor Author

so i'm afraid that most probably there will be something that will break b/c

@joeforjoomla sorry but this is unacceptable. Code is not about fear or brave or supernatural. Test it! Test all the possible cases you can think, did it broke? Then either my claim of non B/C breaks is false or there is a bug. But just because you read some lines of code and you think that this might not be ok it is really unprofessional and deeply offending the creator of the PR.

@joeforjoomla
Copy link
Contributor

I don't want to offend anybody... just to point out that merging so much changes at a Beta 3 stage is too risky.

@dgrammatiko
Copy link
Contributor Author

merging so much changes at a Beta 3 stage is too risky

Merging any code that didn't broke any existing unit and codeception tests and also has been extensively tested by users is not risky. Your claims are totally out of this world, please stop it you're harming your reputation with all these comments

@brianteeman
Copy link
Contributor

@wilsonge before anyone spends any more time on this can you confirm or deny that this would be acceptable to be merged in 4.0 even though we are now passed beta 2

@Fedik
Copy link
Member

Fedik commented Jul 22, 2020

I think it is fine for 4.1, no need to hurry, it does not have a B/C problems.

It like this, you have a two pills:
1 💊 make it fast, but dirty
2 💊 make it good and shiny, but slow

@dgrammatiko
Copy link
Contributor Author

It like this, you have a two pills:

There's a third option:

  • break it to 3 PRs : core changes/com_template changes and actual template changes
    You can then commit 1,3 in 4.0 (minor changes in both PR's) and provide the UI for the new mode in 4.1

In such scenario you have the existing templates in the new mode which is something that could not happen in a minor as it's an actual B/C break. Also I don't think I will be interested to work on this for a year or so...

@PhocaCz
Copy link
Contributor

PhocaCz commented Jul 22, 2020

😊 Maybe Joomla! project should have some kind of "Wild Card" status for feature requests which do not break B/C to allow them to be implemented in Beta in case such feature is better to implement in large version than in minor (in this case because of template developers). But I would say, this feature needs to be properly discussed especially among the leading template developers.

@Bakual
Copy link
Contributor

Bakual commented Jul 22, 2020

feature requests which do not break B/C

B/C isn't the issue here. Also it's not about SemVer.
It's about getting a release ready and when you're still adding features into betas you never get that J4 out of the door. Especially if said feature isn't yet polished.

@PhocaCz
Copy link
Contributor

PhocaCz commented Jul 22, 2020

B/C isn't the issue here. Also it's not about SemVer.
It's about getting a release ready and when you're still adding features into betas you never get that J4 out of the door. Especially if said feature isn't yet polished.

Yes, that sounds reasonable.

@brianteeman
Copy link
Contributor

My comment about b/c was to highlight that if this was bumped to 4.1 then there might be a b/c issue with 4.0

@kawshar
Copy link

kawshar commented Jul 22, 2020

I think the child template could be an amazing feature of Joomla 4. And, should be implemented in 4.0 instead of 4.1. If anyone cares from the leadership team should seriously consider giving it a try.

Thanks

@Bakual
Copy link
Contributor

Bakual commented Jul 22, 2020

Guys, you all see this PR is an RFC and far from being ready to be merged.
It's absolutely out of scope for Joomla 4.0 in its current state. So that discussion is pointless.

If someone brings it to a polished state within a few weeks and people are testing it, then that discussion could start. But now it's a waste of time.

Personally I like the suggestion by @dgrammatiko to break it up into multiple PRs.
Eg moving the template assets out of the template folder into media which could be done fast and could well go into 4.0. And that would lay one of the foundations for this proposal.

@alikon
Copy link
Contributor

alikon commented Jul 22, 2020

personally speaking i really don't care about strict rules
if there is a chance to do something better or even to do something to embrace the most user base desiderata
IMHO strict rules are not a dogma.....
f..of... b/c...

@Fedik
Copy link
Member

Fedik commented Jul 23, 2020

Suggestion about UI:

1 Move button "Create Child" to a template style form, and rename to "Override template files" (or something similar)

2 When User click this button the system will create a dummy template with only templateDetails.xml (with <inherits>parent_name</inherits>), similar to what is made in this PR.
After new template are created, the button "Override template files" hides, and appears the button to "edit files", which basically a link to "template edit".
Suggested xml :

<extension type="template_style" client="site">
  <inherits>parent_name</inherits> (or <parent_name>{blabla}</parent_name>) 
</extension>

3 This "new template" should not be visible in "templates list", only via template style. (this need to avoid confusing between all these template things)

What need to keep in mind:

User must be able to copy his/her fresh override to another site without hacking around a database.
So templateDetails.xml should contain <inherits>parent_name</inherits> (or <parent_name>{blabla}</parent_name>) instead of style_id, and "extension discovery" method should "register" this template as "template style" for "parent_name" template, and if "parent_name" not exist then it should crash and burn.

@dgrammatiko
Copy link
Contributor Author

This "new template" should not be visible in "templates list"

That's extremely easy, it's an extra sql join in the templates list view and another one in the styles view and yes I like the idea as presented (also not to bad in terms of the needed code) 👍.

So templateDetails.xml should contain parent_name (or <parent_name>{blabla}</parent_name>) instead of style_id

This a great point, also should be extremely easy to fix

Eg moving the template assets out of the template folder into media which could be done fast and could well go into 4.0

@Bakual we would have half the payload here if couple years ago we had gone by my proposal for the static media: #21294
It seems that most here see my proposal as madness but there is always some logic in them (ok sometimes too many things need to happen before the ideas are more digestible)

@Bakual
Copy link
Contributor

Bakual commented Jul 24, 2020

@dgrammatiko That other PR imho was to big and changed to much things at once. That's never a good idea.
This one here isn't that big, but it may benefit from splitting it up as well.

I think the idea is good. To late for 4.0 but certainly something to look at for 4.1. So if there is something that needs to go into 4.0 to prevent B/C issues in 4.1, we should look if that smaller part could be done.

@SharkyKZ
Copy link
Contributor

If the idea is that users should only modify child templates base_html lookup is not needed.

@dgrammatiko
Copy link
Contributor Author

To late for 4.0

I really don't understand this, why is it too late for 4.0? Are you in the process of releasing 4.0 in the coming week? If not then why is it too late? I didn't ask for people to do any coding here all I'm asking is if there are things that the project wants in a particular way to be implemented/named/etc. I'll do the code, will not take months or weeks so all I'm asking is for some feedback and then once the real PR is landed to be properly tested. I'm not asking for any favours or assigning any of the coders/creating a working group or anything between. The tracker counts over 400 open issues for the 4.0-dev branch https://github.com/joomla/joomla-cms/issues?page=1&q=is%3Aissue+is%3Aopen+%5B4.0%5D but you keep saying it's too late for any meaningful improvement. I think I have to throw the towel here...

@alikon
Copy link
Contributor

alikon commented Jul 24, 2020

why is it too late for 4.0?

i've the same question, plus, in some way this is a bug fix

There is no way any template to provide safe, self owned overrides and also allow the user to use/create their own ones

Co-authored-by: SharkyKZ <sharkykz@gmail.com>
@Bakual
Copy link
Contributor

Bakual commented Jul 24, 2020

I really don't understand this, why is it too late for 4.0?

Afaik there was a decision to set the focus on the release blockers so it can be released soonish. Adding new features will throw that goal back for sure. But ultimately George has to decide, I have no say.
Realistically speaking, it's far to early to discuss if this may or may not go into 4.0. It has to be written first and then tested. And then comes the decision part.

Imho best approach is to split the PR. So parts that are working fine can already be merged and other parts which may be more controversial or need more love (eg UX things) could be delayed to 4.1 without any issues.

@Bakual
Copy link
Contributor

Bakual commented Jul 24, 2020

The tracker counts over 400 open issues for the 4.0-dev branch

Just on a sidenote, only 24 of those are release blockers. If those are fixed, J4 will be released regardless of the 400 others.

@dgrammatiko
Copy link
Contributor Author

#30192 is just the core changes and it's open for tests, so please go and test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester RFC Request for Comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Comments