Conversation
|
My recommendation, please keep the database out of it. Just a normal override option would be great. I don't know why Joomla has deep integration with the database for all possible features except the most important Media Manager. Thanks |
Thank @Fedik for this!!
One way or another the db is involved here as it holds the style data
Very short answer: db is the only secure storage it got. Just don't ask me why the template style details (or any such data) needs any elevated security 😂 |
|
Couple 2 note :) First note. In current usage I would call it Second note. |
$templaPath = JPATH_THEMES;
if ($template->parent || !empty($template->inherits))
{
$client = $app->isClient('administrator') === true ? 'administrator' : 'site';
$templaPath = JPATH_ROOT . "/media/templates/$client";
}Basically it's a switch for the new mode, the static files exist in the
Inherits imply |
I see now. $template = $app->getTemplate(true);
$templatePath = JPATH_THEMES;
$client = $app->isClient('administrator') === true ? 'administrator' : 'site';
if (is_dir(JPATH_ROOT . "/media/templates/$client/{$template->template}"))
{
$templaPath = JPATH_ROOT . "/media/templates/$client";
}So if original (parent) template has the styles/scripts in /media/templates, then HTMLHelper will take it
Exactly this part is confusing me 😉 |
This is true we could have eliminated the parent attribute both from the db and the manifest but I'm against it I mean stretching the code you could make any template $logo = '<img src="' . $this->baseurl . '/templates/' . $this->template . '/images/logo.svg" class="logo d-inline-block" alt="' . $sitename . '">';which will fail miserable if you try to make a child from it (the $logo = HTMLHelper::image('logo.svg', '', ['class' => 'logo d-inline-block'], true, 0);in order to not break their children...
Are you referring to the db names or the php ones? |
To both :) $template = $app->getTemplate(true);
var_dump($template->inherits);
// ^^^^^^^^^
// This part, I expect it like: $template inherits from "inherits value".
// But "inherits value" contain "own folder name"Well, nevermind, maybe it just me. |
Maybe I misunderstood you comment in the other PR: |
I think that okay. Sure, it will not work for hard-coded paths, that is not our problem. |
Yeah, I meant like this: <name>foobar</name>
<inherits>cassiopeia</inherits>Sorry for confusing :) UPD: But I forgot that the |
|
@Fedik how about instead of @brianteeman any suggestions here? |
That sounds good. To be sure I right understood: The dummy xml of child template will have something like: <name>foobar</name>
<parent>cassiopeia</parent>
And parent template will have I am understood correctly? :) |
|
Yup, you got it right. The pain is that I have to redo all the parent/child templates for the tests 🥱 |
- `parent` is now named `inheritable` (eg allows children) - `inherits` is now `parent` (eg the parent template’s name )
|
Child themes are a great idea. Usually child themes do not require their own manifest, but the name of the child theme directory indicates the link with the parent. In the case of an example. The name of the child theme must be |
Technically a child template is an actual template but only with one file the manifest (templateDetails.xml). This way there are very few changes that need to be done in the core. Can it be done with a snake case naming? Well it's just code so it can be done but will bee extremely inefficient as you will end up scanning the templates dir and checking each folder. Now the data is coming from the db which one way or another Joomla is using it atm and has exactly 0 performance impact. Also it's not just to get something working in the core you have to think/see what's the code in com_templates. With the proposed approach the changes are minimal. |
I recently did this for the yootheme template, I had to override a couple of classes. One way or another, I would still add a naming rule for the child template. |
That's possible, eg prefix it with the parent name like |
|
thanks for the explanation I would like to make some code suggestion, I hope it's ok for you I have reopened the pr. |
| if ($this->template->parent) | ||
| { | ||
| if (!file_exists(JPATH_THEMES . '/' . $this->template->template . '/index.php')) | ||
| { | ||
| if (!file_exists(JPATH_THEMES . '/' . $this->template->parent . '/index.php')) | ||
| { | ||
| throw new \InvalidArgumentException(Text::sprintf('JERROR_COULD_NOT_FIND_TEMPLATE', $this->template->template)); | ||
| } | ||
| } | ||
| } | ||
| elseif (!file_exists(JPATH_THEMES . '/' . $this->template->template . '/index.php')) |
There was a problem hiding this comment.
| if ($this->template->parent) | |
| { | |
| if (!file_exists(JPATH_THEMES . '/' . $this->template->template . '/index.php')) | |
| { | |
| if (!file_exists(JPATH_THEMES . '/' . $this->template->parent . '/index.php')) | |
| { | |
| throw new \InvalidArgumentException(Text::sprintf('JERROR_COULD_NOT_FIND_TEMPLATE', $this->template->template)); | |
| } | |
| } | |
| } | |
| elseif (!file_exists(JPATH_THEMES . '/' . $this->template->template . '/index.php')) | |
| if ( | |
| !file_exists(JPATH_THEMES . '/' . $this->template->template . '/index.php') | |
| && ( | |
| empty($this->template->parent) | |
| || !file_exists(JPATH_THEMES . '/' . $this->template->parent . '/index.php') | |
| ) | |
| ) |
There was a problem hiding this comment.
@HLeithner the rest of the changes are fine but not this one. You can reduce the inner 2 conditionals to 1 but changing it to 1 (top) conditional is wrong in the sense that the ($this->template->parent) is extremely cheap to resolve but
!file_exists(JPATH_THEMES . '/' . $this->template->template . '/index.php')
&& (
empty($this->template->parent)
|| !file_exists(JPATH_THEMES . '/' . $this->template->parent . '/index.php')
)introduces 2 more file lookups for every execution for no reason.
There was a problem hiding this comment.
introduces 2 more file lookups for every execution for no reason.
actually not true, the second !file_exists will not be called if empty($this->template->parent) is true
but that was not the point you can leave it that way if you want it's only to reduce duplicated code. Will you merge the other suggestions?
There was a problem hiding this comment.
Done, although I have to state that I'm not so sure I want to move this one forward anymore.
There was a problem hiding this comment.
What's the reason? because I said it can not go into 4.1?
There was a problem hiding this comment.
I read them but the explanation but they don't make sense to me, a js that's template independent is something different then a image that's only used in the template. We already split everything about the joomla filesystem at least the templates are "self contained" but that's only my opinion.
Anyway that doesn't hold this feature back.
There was a problem hiding this comment.
The move to media folder seems unnecessary. Can't we just check for assets in child template directory and fallback to parent template directory?
If you want to move assets to media directory for other purposes, that could be done in a separate PR so it doesn't have to wait for this.
There was a problem hiding this comment.
You really don’t get it. It’s you only opportunity to fix this and bring consistency...
There was a problem hiding this comment.
You really don’t get it. It’s you only opportunity to fix this and bring consistency...
Do a PR where you only move the assets to media. I actually think that could still be done for 4.0 as it's not a new feature.
If it can be done independant, lets do it independant and don't mix things.
There was a problem hiding this comment.
Guys do you even realise the amount of work I put here to create all the 4 templates and the other html overrides for the tests?
Asking to break it in 2 is just asking me to repeat the whole work twice. Why? This is a new mode, it doesn't and shouldn't affect any of the current templates
|
|
||
| // Fallback template | ||
| if (!file_exists(JPATH_THEMES . '/' . $template->template . '/index.php')) | ||
| if (!empty($template->parent)) |
There was a problem hiding this comment.
I think you can simplify this too like them above to reduce code duplication
Co-authored-by: Harald Leithner <leithner@itronic.at>
Co-authored-by: Harald Leithner <leithner@itronic.at>
Co-authored-by: Harald Leithner <leithner@itronic.at>
|
I have tested this item ✅ successfully on eec9d9a This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30192. |
|
I really really shouldn't be pressing merge here - but I've seen several template clubs in favour of this so I'm going to break my own rules and hope I don't regret it. thankyou for this really nice feature @dgrammatiko |
| `inheritable` smallint DEFAULT 0 NOT NULL, | ||
| `parent` varchar(50) DEFAULT '', |
There was a problem hiding this comment.
Strange that nobody saw this and nobody tested with PostgreSQL like it should always be done when a PR has database changes. This has broken installation on PostgreSQL. Will prepare a fix.
| @@ -0,0 +1,2 @@ | |||
| ALTER TABLE "#__template_styles" ADD COLUMN "inheritable" smallint NOT NULL DEFAULT 0; | |||
| ALTER TABLE "#__template_styles" ADD COLUMN "parent" varchar(50) DEFAULT ""; | |||
There was a problem hiding this comment.
Next mistake DEFAULT "" should be DEFAULT ''
There was a problem hiding this comment.
please submit a fix i'll test it
This fixes update from 3.10 to 4 failing with SQL error due to not existing database columns.
Pull Request is the foundation for #30149
Summary of Changes
Introduces Inheritable templates solving numerous problem with the current state of templates. Read more on the linked issue above
This PR is complete in the sense that contains all the changes (even the upgrade path from J3) to introduce the new mode but without introducing the GUI and neither is moving any of the templates to the new mode. These can be done in following PR's
Testing Instructions
Do not try to test this with patch tester, it won't work.
Install the PR's installable, you can d/l it here (at the bottom of this page)
If you want to use git command lines or the desktop app make sure that you'll do a fresh install or apply the updates (sql files)
Make sure you have the browser's console open so you can observe any missing assets.
If this went smooth, then it's a good sign as the installation is a seperate application with it's own template and although this PR is not touching the installation template everything works as before. B/C check 1
Front End step 1
Go to the front end and vist the following urls:
Everything should be fine, no console logs or missinng icons, etc. B/C check 2
Front End step 2
Download this cassiopeia_overrides_v2.zip and extract the contents and copy them in the folder
templates/cassiopeia/html. Revisit the pages of the previous step. You should see in the pages some plain text (that will also break a bit the design)Legacy Component view override. You have to look forComponent,Module,PluginandJLayout. If so then the cascading of the templating is working correctly, in the previous step there was no overrides in this one the overrides were treated as top priority. Also cassiopeia has some overrides for the static assets so that was already tested (if you want you might open a normal 4.0 version in another winndow and check the number of assets per page between the 2). B/C check 3Front End step 3
Install the first new mode template (basically cassiopeia with another name: siteparent) from here siteparent_v2.zip
Go to the admin and make the siteparent template default and also assign all the menus to this template!!! Go to the front end and repeat the Front End step 1 and confirm that everything is as before. check 4
Front End step 4
Go with your filemanager to
templates/siteparentand copy the contents os the folderxhtmlto the folderhtml. Redo the ritual of Front End step 1 and confirm that all the partsComponent,Module,PluginandJLayoutare there. check 5Front End step 5
Although the child templates should be created inside joomla's env for testing purposes we will do it using the installer, so install this: sitechild_v2.zip
Go to the admin and make the sitechild template default and also assign all the menus to this template!!! Go to the front end and repeat the Front End step 1 and confirm that everything is as before. check 6
Front End step 6
Go with your filemanager to
templates/sitechildand rename the folderxhtmltohtml. Redo the ritual of Front End step 1 and confirm that all the partsComponent,Module,PluginandJLayoutare there. check 7Back End step 1
Go to the addinistrator end and vist the following urls:
Everything should be fine, no console logs or missinng icons, etc. B/C check 8
Back End step 2
Download this atum_overrides_v2.zip and extract the contents and copy them in the folder
administrator/templates/atum/html. Revisit the pages of the previous step. You should see in the pages some plain text (that will also break a bit the design)Legacy Component view override. You have to look forComponent,Module,PluginandJLayout. If so then the cascading of the templating is working correctly, in the previous step there was no overrides in this one the overrides were treated as top priority. Also cassiopeia has some overrides for the static assets so that was already tested (if you want you might open a normal 4.0 version in another window and check the number of assets per page between the 2). B/C check 9Back End step 3
Install the new mode template (basically atum with another name: adminparent) from here
adminparent_v2.zip
Go to the administrator template styles and make the adminparent template the default one. Repeat the Back End step 1 and confirm that everything is as before. check 10
Back End step 4
Go with your filemanager to
administrator/templates/adminparentand copy the contents os the folderxhtmlto the folderhtml. Redo the dance of the Back End step 1 and confirm that all the partsComponent,Module,PluginandJLayoutare there. check 11Back End step 5
Althouth the child templates should be created inside joomla's env for testing purposes we will do it using the installer, so install this:
adminchild_v2.zip
Go to the admin and make the adminchild template the default. Repeat the Back End step 1 and confirm that everything is as before. check 12
Back End step 6
Go with your filemanager to
administrator/templates/adminchildand rename the folderxhtmltohtml. Redo the ritual of the Back End step 1 and confirm that all the partsComponent,Module,PluginandJLayoutare there. check 13Final checks
Check your db and the table template_styles, you should have
0and inherits ``1and inherits ``0and inheritssiteparentandsiteadminrespectfullyFinally someone needs to confirm that the update sql files are all that is needed here
And the last one
You have probably saw some missing strings when you were trying to edit any of the template styles. This is expected as I forgot to include any languages in the parent template, but this is quite easy to check. You are already in the adminchild template so all you have to do is create a folder
language/en-GBin the adminparent template and then copy the tmpl_atum*.ini to this folder and rename them to tmpl_adminparent.ini and tmpl_adminparent.sys.ini. Check again the strings are back. Then make the adminparent the default template and check for any changes (readable text should remain readable). You can repeat this for the front end templates (although the function is common for both).Documentation Changes Required
For the devs the new parts are 2:
<inheritable>1</inheritable >to enable a template in the new modemedia/( administrator || site )/templateName. Folders like css/js/images inside the above path will act exactly the same as they did in the oldtemplates/name/( css || js || images )