? Documentation Required NPM Resource Changed ? Pending

User tests: Successful: Unsuccessful:

avatar dgrammatiko
dgrammatiko
22 Oct 2021

Pull Request for Issue # .

Summary of Changes

This PR

  • moves Cassiopeia and Atum to the new mode (eg supporting child templates)
  • moves the assets for the system templates to the media folder
  • patches tinyMCE to use the correct fallback editor.css

Testing Instructions

DO NOT USE PATCHETESTER FOR THIS ONE. Download the package instead

  • If you're using Git:
    It needs npm I and also a change in the db table #_template_styles: the column inheritable should become 1 for all the styles of the 2 templates

  • If you're installing the installable package
    No extra steps

  • Check that the Joomla Install doesn't have any visual regression

  • Check that the back end template is not missing any assets and doesn't have any visual regression (skip the extension templates, there's a PR #35998 for that)

  • CHANGE THE DEFAULT EDITOR TO NONE!!! (tinyMCE has its own PR #36011)

  • Check that the front end template is not missing any assets and doesn't have any visual regression

  • Check the Error pages

  • Check that if you delete the file media/templates/cassiopeia/css/editor.css and try to edit an article (assuming that tinyMCE is the default editor) the media/system/css/editor.css is in the list of loaded CSS files

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

There are no visual differences, the changes are in the XML definition of the templates and some static files moved to the media folder

Documentation Changes Required

Still needed:

  • The com_templates is not supporting yet the new mode PR: #35998
  • There is another PR for tinyMCE and the editor.css #36000
avatar dgrammatiko dgrammatiko - open - 22 Oct 2021
avatar dgrammatiko dgrammatiko - change - 22 Oct 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 22 Oct 2021
Category Administration Templates (admin) NPM Change Repository
avatar richard67
richard67 - comment - 22 Oct 2021

@richard67 I need your expertise here for the sql changes and also how we are gonna handle the moving of the existing static folders to the media folder (I'm thinking of 2 functions: one runs preflight copying the css/js/images/scss to media and one post flight removing all these folders from the template/../...)

@dgrammatiko Which SQL changes? I can't see any change in the PR yet for new installations, so I have no idea what to do on updates. And which folders moving? If the update packages contain what we ship with the core at the new place, that will be there after unzip, so we just have to delete the stuff at the old place post-flight with the regular files and folder deletion, or am I missing something?

Update: Or does custom stuff like user.css has to be moved, too?

avatar dgrammatiko
dgrammatiko - comment - 22 Oct 2021

I can't see any change in the PR yet for new installations,

Will add them in a bit

or am I missing something?

Theoretically, users could have their own css/js/images in the template/{atum | cassiopeia}/{css/scss/js/images} thus we need to copy whatever exists there to media preflight and then remove those folders post flight. Or am I stretching this too far?

avatar richard67
richard67 - comment - 22 Oct 2021

Or am I stretching this too far?

@dgrammatiko I have to think about it a bit, but I think you are right. Regarding bilateral detail clarifications we can use Glip, if necessary, in order not to blow up number of comments here.

avatar richard67
richard67 - comment - 22 Oct 2021

@dgrammatiko Just a question for my understanding, sorry if that was already clarified elsewhere and I missed it: With templates which support child templates, will EVERY css and js for a template be below the media folder, also custom stuff added by the user, so below the regular template folder there won't be any (s)css and js anymore?

avatar dgrammatiko
dgrammatiko - comment - 22 Oct 2021

so below the regular template folder there won't be any (s)css and js anymore?

Yup, the folder media/templates/site/cassiopeia/css has all the super powers the templates/cassiopeia/css had (eg placing files there will override the css file, assuming the the API was used for inserting the tag in the first place). The same goes for js and images...

avatar richard67
richard67 - comment - 22 Oct 2021

@dgrammatiko Another thing is that if users did it right, they did not make any customizations (besides a user.css) in the template but made a copy of the template, especially in case of the frontend (Cassiopeia). I don't think we safely can update also such template copies to the new style, so maybe it should be documented somewhere or told in the release announcements something like "If you have made a copy of the Cassiopeia or the Atum template, ....".

avatar dgrammatiko
dgrammatiko - comment - 22 Oct 2021

I don't think we safely can update also such template copies to the new style

Oh no, template copies won't follow this pattern, this will be strictly only for the Cassiopeia and Atum. Their fork, if exists will still work (we don't touch anything in there) but will not converted to the new mode

avatar Stuartemk
Stuartemk - comment - 22 Oct 2021

Hi everyone, @dgrammatiko With this new feature, it would be possible for the superuser to make a template for each device, for example for mobile, tablet, or desktop and thus avoid overloading everything in in a single css? And if it is possible to detect the type of device also to avoid loading several css and only serve the css corresponding to the requesting device? I thank in advance much the answer.

avatar richard67
richard67 - comment - 22 Oct 2021

@dgrammatiko GitHub shows some of the files as a deleted file and a new one instead of having been moved, e.g. the "_bootstrap.scss" file. The reason might be that you have moved the file and then made changes on the content of that file with the same commit. In such a case this happens. It can be avoided by making 2 separate commit, one for moving the file (e.g. with the "git mv" command or on GitHub's UI here, that works nice, too) and then the other commit for the changes inside.

It is not a big problem here, I think, because the history in Git is not completely lost, but I'm not sure if Git blame will work here for these files.

If @bembelimen is ok with it, leave it as it is and use my hint only for future PR's.

avatar dgrammatiko
dgrammatiko - comment - 22 Oct 2021

it would be possible for the superuser to make a template for each device, for example for mobile, tablet, or desktop and thus avoid overloading everything in in a single css?

This is already possible right now but if you use Bootstrap there's no way to have any gains as the minimum CSS served is already predefined. Will it be easier is a more interesting question and I think it will be (although I haven't explored this area of serving per device, so take this with a grain of salt)

avatar dgrammatiko dgrammatiko - change - 22 Oct 2021
Labels Added: NPM Resource Changed ?
avatar richard67
richard67 - comment - 22 Oct 2021

@dgrammatiko The build in npm is failing, see https://ci.joomla.org/joomla/joomla-cms/48041/1/7 :

Error: Can't find stylesheet to import.
@import "../../../../../../../../media/vendor/fontawesome-free/scss/fontawesome";
media/templates/administrator/atum/scss/vendor/fontawesome-free/fontawesome.scss 6:9  @import
installation/template/scss/template.scss 8:9                                          @import
installation/template/scss/template-rtl.scss 1:9                                      root stylesheet
avatar richard67
richard67 - comment - 22 Oct 2021

@dgrammatiko Regarding the update SQL I have a question. Users might have created new template styles for atum or cassiopeia. Shall we update the inheritable flag to 1 for all of them? If yes, then the update would be easy.

If you want I can make a PR for you, but I think you can do that yourself, too. Just add a new "4.1.0-2021-10-22.sql" for each database type with following one statement:

UPDATE `#__template_styles` SET `inheritable` = 1 WHERE `template` = 'atum' AND `client_id` = 1 OR `template` = 'cassiopeia' AND `client_id` = 0;

In the PostgreSQL file just replace the MySQL names quotes by ":

UPDATE "#__template_styles" SET "inheritable" = 1 WHERE "template" = 'atum' AND "client_id" = 1 OR "template" = 'cassiopeia' AND "client_id" = 0;
avatar richard67
richard67 - comment - 22 Oct 2021

@dgrammatiko For the files and folders deletion and copying present custom stuff I will check later. Normally we don't bother authors of PR's for 4.x with the files and folders deletion and do that just before the release. But in case of this PR it makes sense that we do it here together with the custom stuff copy thing.

avatar joomla-cms-bot joomla-cms-bot - change - 22 Oct 2021
Category Administration Templates (admin) NPM Change Repository SQL Administration com_admin Postgresql Templates (admin) NPM Change Repository
avatar dgrammatiko
dgrammatiko - comment - 22 Oct 2021

@richard67 thanks, hopefully I didn't mess things 108250f

Yeah, the folders for this particular case need some custom handling as they might contain user created files and we don't want to delete these

avatar richard67
richard67 - comment - 22 Oct 2021

@dgrammatiko SQL stuff looks ok to me. NPM is happy, too, up to now.

avatar richard67
richard67 - comment - 22 Oct 2021

Yeah, the folders for this particular case need some custom handling as they might contain user created files and we don't want to delete these

@dgrammatiko Could you list which folders we should keep in case of present custom stuff and which we could and should delete because there should not be any user stuff?

avatar dgrammatiko
dgrammatiko - comment - 22 Oct 2021

So, as a case study let's take Atum (cassiopeia will be similar). The final structure of the template folder will be
Screenshot 2021-10-22 at 14 53 49

And the media folder should have all the static folders (js,css,scss,images):
Screenshot 2021-10-22 at 14 54 56

But we cannot simply delete the existing folders in the administrator/templates/atum as these might have user files, so what I was thinking as a safe upgrade path would be something like:

  • in the preflight we copy all these directories from administrator/templates/atum/{css,scss,js,images} to media/templates/administrator/atum/{css,scss,js,images}
  • Let the update overwrite as it should all the existing files in that folder media/templates/administrator/atum/{css,scss,js,images} with the ones from 4.1
  • In the post flight remove the dirs administrator/templates/atum/{css,scss,js,images}

The process should retain any user created static overrides (css,js,images)

avatar richard67
richard67 - comment - 22 Oct 2021

@dgrammatiko Alternatively we don't to anything pre-flight, and post-flight before we delete files and folders we go through these specific folders and copy files which exist in the old but not in the new folder.

Both will make problems if there are files which exist in the old folder but not in the new folder and which are not custom stuff. That would happen e.g. when later after this PR has been merged changes will be done in (s)css or js for these 2 templates which remove some css or js or image files, so you have them in the old but not in the new folder when updating from 4.0.x to 4.1.0 which will include your PR.

I think we should use a hard-coded list of the core old stuff as it is now and which we use as exclusion list for that copying so we only copy stuff which really has been added by the user and not old obsolete core stuff.

Update: Forget the problem. That will be handled with the regular files and folders deletion.

avatar dgrammatiko
dgrammatiko - comment - 22 Oct 2021

Alternatively we don't to anything pre-flight, and post-flight before we delete files and folders we go through these specific folders and copy files which exist in the old but not in the new folder.

That would work as well

Both will make problems if there are files which exist in the old folder but not in the new folder and which are not custom stuff.

If the files are redundant and somehow still exist in the new path obviously is not perfect but as these are static files and mostly few kb that won't create any issues (imho). Keeping a registry with the files is a maintainers nightmare thus my naive approach copy/update/delete. Anyways, I'm ok with any solution as long as users won't start blaming me that I deleted their precious files...

avatar richard67
richard67 - comment - 22 Oct 2021

@dgrammatiko Forget about the problem, I have just updated my previous comment.

avatar richard67
richard67 - comment - 22 Oct 2021

@dgrammatiko Ideally we would put the copying of custom js and css and image files to here between the files removal and the folders removal: https://github.com/joomla/joomla-cms/blob/4.1-dev/administrator/components/com_admin/script.php#L7395

The cool thing if we do that there is that we will have removed all core files at the old place at this point so what is left in the relevant old folders will be custom stuff, and the folder deletion later will delete that at the old place after we have copied it because that how "Folder::delete" works, if deletes all sub-folders and files.

avatar richard67
richard67 - comment - 22 Oct 2021

@dgrammatiko Here is a list of all folders and sub-folders which would be deleted. Do you think we should keep any custom file in all of these? Or are there certain sub-folders where we can exclude this, e.g. "vendor"?

administrator/templates/atum/css
administrator/templates/atum/css/system
administrator/templates/atum/css/system/searchtools
administrator/templates/atum/css/vendor
administrator/templates/atum/css/vendor/awesomplete
administrator/templates/atum/css/vendor/choicesjs
administrator/templates/atum/css/vendor/fontawesome-free
administrator/templates/atum/css/vendor/joomla-custom-elements
administrator/templates/atum/css/vendor/minicolors
administrator/templates/atum/images
administrator/templates/atum/images/logos
administrator/templates/atum/scss
administrator/templates/atum/scss/blocks
administrator/templates/atum/scss/pages
administrator/templates/atum/scss/system
administrator/templates/atum/scss/system/searchtools
administrator/templates/atum/scss/vendor
administrator/templates/atum/scss/vendor/awesomplete
administrator/templates/atum/scss/vendor/bootstrap
administrator/templates/atum/scss/vendor/choicesjs
administrator/templates/atum/scss/vendor/fontawesome-free
administrator/templates/atum/scss/vendor/joomla-custom-elements
administrator/templates/atum/scss/vendor/minicolors
templates/cassiopeia/css
templates/cassiopeia/css/global
templates/cassiopeia/css/system
templates/cassiopeia/css/system/searchtools
templates/cassiopeia/css/vendor
templates/cassiopeia/css/vendor/choicesjs
templates/cassiopeia/css/vendor/joomla-custom-elements
templates/cassiopeia/images
templates/cassiopeia/js
templates/cassiopeia/scss
templates/cassiopeia/scss/blocks
templates/cassiopeia/scss/global
templates/cassiopeia/scss/system
templates/cassiopeia/scss/system/searchtools
templates/cassiopeia/scss/tools
templates/cassiopeia/scss/tools/functions
templates/cassiopeia/scss/tools/variables
templates/cassiopeia/scss/vendor
templates/cassiopeia/scss/vendor/bootstrap
templates/cassiopeia/scss/vendor/choicesjs
templates/cassiopeia/scss/vendor/joomla-custom-elements
templates/cassiopeia/scss/vendor/metismenu

I think we could at least exclude that someone has added custom stuff to one of the present vendor folders, like "(s)css/vendor/joomla-custom-elements".

avatar richard67
richard67 - comment - 22 Oct 2021

@dgrammatiko And what shall we do if someone has created a custom sub-folder, e.g. "templates/cassiopeia/css/foobar"? I guess we have to copy that, too.

avatar dgrammatiko
dgrammatiko - comment - 22 Oct 2021

And what shall we do if someone has created a custom sub-folder, e.g. "templates/cassiopeia/css/foobar"? I guess we have to copy that, too.

Yes, it's not only files but could be subfolders, thus I thought it would be easier to copy things as they are from the template/... to media/... and then do the update (the process will replace all the core files) so everything would be as if they were updating any patch version.

avatar richard67
richard67 - comment - 22 Oct 2021

thus I thought it would be easier to copy things as they are from the template/... to media/... and then do the update (the process will replace all the core files)

@dgrammatiko Not sure about that, because of ongoing parallel development of 4.0.x and 4.1 it might be that we copy files which became obsolete during 4.0.x development and so would be deleted on update, but the files and folder deletion would delete them at the old but not at the new place. We would have to change the way how we generate these lists to handle that right. But I am still thinking about it.

If you want you can already have a PR from me with the files and folders deletion (but without the copying custom stuff). I have that ready here.

avatar dgrammatiko
dgrammatiko - comment - 22 Oct 2021

But I am still thinking about it.

I think we have some time here as this PR is depending on another PR which I have not even started yet 😀

avatar richard67
richard67 - comment - 22 Oct 2021

@dgrammatiko I've made a PR for you with the deleted files and folders update.

Other question: Cassiopeia has a "js" folder, but Atum doesn't. Shall we copy the "js" folder for Atum, too, if a user has added that folder?

avatar brianteeman
brianteeman - comment - 22 Oct 2021

Would it be easier to develop all the child template stuff in a branch?

avatar dgrammatiko
dgrammatiko - comment - 22 Oct 2021

Would it be easier to develop all the child template stuff in a branch?

Sure, the decision is to patch the com_templates with the missing functionality for child templates, thus the changes won't be so many. The other PR that merges the views and introducing new workflows was postponed for a later release, it's not abandoned.

Shall we copy the "js" folder for Atum, too, if a user has added that folder?

Hmm, good question, I guess we have to cover this case as well

avatar richard67
richard67 - comment - 22 Oct 2021

@dgrammatiko I've made a PR for you for the custom files thing.

avatar dgrammatiko
dgrammatiko - comment - 22 Oct 2021

@richard67 that's awesome, thanks.
Now I should patch the com_templates the coming days so we can have some spare time for the docs/etc.

avatar richard67
richard67 - comment - 23 Oct 2021
avatar richard67
richard67 - comment - 23 Oct 2021

@dgrammatiko I'm not sure what "File:move" does when some parent folder doesn't exist, e.g. when we have to move a file "templates/cassiopeia/css/foo/bar/gaga.css" so folders "foo" and "foo/bar" would have to be created at the target path. I will check and if necessary provide a fix.

Update: I've updated my PR for you to handle this, too.

avatar richard67
richard67 - comment - 23 Oct 2021

@dgrammatiko I still don't get why it needs to move the preview images. Can they really have been modified by a user for a core template and do we want to keep these modifications? If this is not the case there is no need to move them because they will be unzipped from the update package and be deleted with the file deletion, and so it does not need the special handling for them which I have added with my PR.

avatar dgrammatiko
dgrammatiko - comment - 23 Oct 2021

I still don't get why it needs to move the preview images.

You're right, we don't need some extra functionality here, the existing script.php could handle these files fine. Slow start...

avatar richard67
richard67 - comment - 23 Oct 2021
avatar richard67
richard67 - comment - 23 Oct 2021

@dgrammatiko For testing instructions I suggest to test both, a new installation made with the branch of this PR, and an update of a 4.1-dev installation using the update package which has been built by drone for that PR. For the latter we could even say one should add some custom js or css folders and files and so we can test that they are moved to the right place on update. Could you update testing instructions in that way? Thanks in advance.

avatar dgrammatiko
dgrammatiko - comment - 24 Oct 2021

@richard67 I'm waiting for #35780 to solve atum and cassiopeia logo image urls (I was about to use HTMLHelper but if the other PR is merged I prefer to use the JLayout which is orders of magnitude more friendly for front enders). Also, this PR should not be merged before the com_template changes are in place (the other PR)

avatar HLeithner
HLeithner - comment - 4 Nov 2021

@dgrammatiko may be a bit off-topic but where would you put the x.webmanifest file?

I didn't reviewed the complete code so please forgive me, did you changed the favicon detection too?

avatar dgrammatiko
dgrammatiko - comment - 4 Nov 2021

@HLeithner so, x.webmanifest is a file served directly from Apache/NginX/Etc so by definition for Joomla is treated as a static file so it should live in the media folder.

avatar dgrammatiko
dgrammatiko - comment - 4 Nov 2021

I didn't reviewed the complete code so please forgive me, did you changed the favicon detection too?

If I didn't do something stupid the favicons should already support child templates:

$noFavicon = true;
$searchFor = 'image/vnd.microsoft.icon';
// @codingStandardsIgnoreStart
array_map(function($value) use(&$noFavicon, $searchFor) {
if (isset($value['attribs']['type']) && $value['attribs']['type'] === $searchFor)
{
$noFavicon = false;
}
}, array_values((array)$this->_doc->_links));
// @codingStandardsIgnoreEnd
if ($noFavicon)
{
$client = $app->isClient('administrator') === true ? 'administrator/' : 'site/';
$template = $app->getTemplate(true);
// Try to find a favicon by checking the template and root folder
$icon = '/favicon.ico';
$foldersToCheck = [
JPATH_BASE,
JPATH_ROOT . '/media/templates/' . $client . $template->template,
JPATH_BASE . '/templates/' . $template->template,
];
foreach ($foldersToCheck as $base => $dir)
{
if ($template->parent !== ''
&& $base === 1
&& !is_file(JPATH_ROOT . '/media/templates/' . $client . $template->template . $icon))
{
$dir = JPATH_ROOT . '/media/templates/' . $client . $template->parent;
}
if (is_file($dir . $icon))
{
$urlBase = in_array($base, [0, 2]) ? Uri::base(true) : Uri::root(true);
$base = in_array($base, [0, 2]) ? JPATH_BASE : JPATH_ROOT;
$path = str_replace($base, '', $dir);
$path = str_replace('\\', '/', $path);
$this->_doc->addFavicon($urlBase . $path . $icon);
break;
}
}
}

avatar HLeithner
HLeithner - comment - 4 Nov 2021

I didn't reviewed the complete code so please forgive me, did you changed the favicon detection too?

If I didn't do something stupid the favicons should already support child templates:

$noFavicon = true;
$searchFor = 'image/vnd.microsoft.icon';
// @codingStandardsIgnoreStart
array_map(function($value) use(&$noFavicon, $searchFor) {
if (isset($value['attribs']['type']) && $value['attribs']['type'] === $searchFor)
{
$noFavicon = false;
}
}, array_values((array)$this->_doc->_links));
// @codingStandardsIgnoreEnd
if ($noFavicon)
{
$client = $app->isClient('administrator') === true ? 'administrator/' : 'site/';
$template = $app->getTemplate(true);
// Try to find a favicon by checking the template and root folder
$icon = '/favicon.ico';
$foldersToCheck = [
JPATH_BASE,
JPATH_ROOT . '/media/templates/' . $client . $template->template,
JPATH_BASE . '/templates/' . $template->template,
];
foreach ($foldersToCheck as $base => $dir)
{
if ($template->parent !== ''
&& $base === 1
&& !is_file(JPATH_ROOT . '/media/templates/' . $client . $template->template . $icon))
{
$dir = JPATH_ROOT . '/media/templates/' . $client . $template->parent;
}
if (is_file($dir . $icon))
{
$urlBase = in_array($base, [0, 2]) ? Uri::base(true) : Uri::root(true);
$base = in_array($base, [0, 2]) ? JPATH_BASE : JPATH_ROOT;
$path = str_replace($base, '', $dir);
$path = str_replace('\\', '/', $path);
$this->_doc->addFavicon($urlBase . $path . $icon);
break;
}
}
}

If I'm not wrong then this doesn't search in the media folder:

			$foldersToCheck = [
				JPATH_BASE,
				JPATH_ROOT . '/media/templates/' . $client . $template->template,
				JPATH_BASE . '/templates/' . $template->template,
			];
avatar HLeithner
HLeithner - comment - 4 Nov 2021

@HLeithner so, x.webmanifest is a file served directly from Apache/NginX/Etc so by definition for Joomla is treated as a static file so it should live in the media folder.

of course this should be in the media folder ;-) but can it be overwritten by a child template?

avatar dgrammatiko
dgrammatiko - comment - 4 Nov 2021

If I'm not wrong then this doesn't search in the media folder:

Hmm, the second array entry: JPATH_ROOT . '/media/templates/' . $client . $template->template, is the media folder path for the current template, so it should be covered

of course this should be in the media folder ;-) but can it be overwritten by a child template?

That's a good question. The HTMLHelper AFAIK only supports scripts, stylesheets and images. That said if you use the script method (I haven't tested this, so it might not be working) with the option to echo only the relative path and then add it to the links array it should be fine (assuming that the file is saved under the js folder). If that (hack) doesn't work I guess we need to introduce another more generic function to get the relative functionality for other files (eg .webmanifest, .json, etc). The automatic resolution happens automatically only for the folders js, css, images IIRC

avatar HLeithner
HLeithner - comment - 4 Nov 2021

If I'm not wrong then this doesn't search in the media folder:

Hmm, the second array entry: JPATH_ROOT . '/media/templates/' . $client . $template->template, is the media folder path for the current template, so it should be covered

haha next time I should better look at the code I post...

of course this should be in the media folder ;-) but can it be overwritten by a child template?

That's a good question. The HTMLHelper AFAIK only supports scripts, stylesheets and images. That said if you use the script method (I haven't tested this, so it might not be working) with the option to echo only the relative path and then add it to the links array it should be fine (assuming that the file is saved under the js folder). If that (hack) doesn't work I guess we need to introduce another more generic function to get the relative functionality for other files (eg .webmanifest, .json, etc). The automatic resolution happens automatically only for the folders js, css, images IIRC

the other question is, is this a webasset?
Edit: and should be covered by the webassetmanager?

avatar dgrammatiko
dgrammatiko - comment - 4 Nov 2021

the other question is, is this a webasset?

It's a static asset so should be covered from weassets (IMHO)

Edit: and should be covered by the webassetmanager?

Yes but there's something deeper here, most apache configurations don't have the mime type enabled by default, so there are changes required in the server level (mime type and expire headers). Also one of the reasons we probably want this done through the webassets is the cache invalidation that the system provides (the ?12341325)

avatar HLeithner
HLeithner - comment - 4 Nov 2021

Edit: and should be covered by the webassetmanager?

Yes but there's something deeper here, most apache configurations don't have the mime type enabled by default, so there are changes required in the server level (mime type and expire headers). Also one of the reasons we probably want this done through the webassets is the cache invalidation that the system provides (the ?12341325)

Ok I think that's another PR, I'm asking my self why they created a new file extension... it's a json file.
Think about this I checked the W3C Editor Draft dated on 6.10.2021 which says

NOTE: File extension: .webmanifest or .json?
The IANA registered file extension for the manifest is .webmanifest. Some web servers recognize this extension and transfer the file using the standardized application manifest media type (application/manifest+json). Developers can also choose a different extension (e.g. .json) or none at all (e.g. /api/GetManifest), but are encouraged to transfer the manifest using the application/manifest+json MIME type, although any JSON MIME type is ok.

Which clearly says we could use .json files which would "allow us" to save it to the .js folder and have less problems with webservers.

Anyway the document is still in a draft state so we can can talk about this when it's finished. In the mean time the js folder should fit.

avatar dgrammatiko
dgrammatiko - comment - 4 Nov 2021

Ok I think that's another PR, I'm asking my self why they created a new file extension... it's a json file.

FWIW I always use .json for the manifest, eg https://github.com/dgrammatiko/sloth/blob/step-6-inherritable/media_src/site.json or the live part of the same file: https://sloth.dgrammatiko.dev/media/templates/site/sloth/site.json

avatar dgrammatiko dgrammatiko - change - 9 Nov 2021
The description was changed
avatar dgrammatiko dgrammatiko - edited - 9 Nov 2021
avatar chmst
chmst - comment - 9 Nov 2021

Updated a 4.1 installation with the update package.
In both, atum and cassiopiea the preview.png and thumbnail.png are missing

avatar dgrammatiko
dgrammatiko - comment - 9 Nov 2021

In both, atum and cassiopiea the preview.png and thumbnail.png are missing

This is expected, the com_templates needs some changes to handle the new mode templates. There's a PR #35879 that fixes the thumbs, introduces the process for creating child templates but missing the file manager part (I'm currently working on it)

avatar laoneo
laoneo - comment - 9 Nov 2021

inheritable was already set after I have installed the pr code. But got the following warning:

Warning
Could not find the file 'editor.css' in the template or templates/system folder. No styles are available.

The rest looks ok as I'v installed blog sample data and browsed around in the back- and frontend.

avatar dgrammatiko
dgrammatiko - comment - 9 Nov 2021

Warning
Could not find the file 'editor.css' in the template or templates/system folder. No styles are available.

What was your default editor?

Edit: Ignore my previous question. The error comes from some wrong conditional:

if ($content_css_custom)
{
// If URL, just pass it to $content_css
if (strpos($content_css_custom, 'http') !== false)
{
$content_css = $content_css_custom;
}
// If it is not a URL, assume it is a file name in the current template folder
else
{
$content_css = Uri::root(true) . '/templates/' . $template . '/css/' . $content_css_custom;
// Issue warning notice if the file is not found (but pass name to $content_css anyway to avoid TinyMCE error
if (!file_exists($templates_path . '/' . $template . '/css/' . $content_css_custom))
{
$msg = sprintf(Text::_('PLG_TINY_ERR_CUSTOMCSSFILENOTPRESENT'), $content_css_custom);
Log::add($msg, Log::WARNING, 'jerror');
}
}
}
else
{
// Process when use_content_css is Yes and no custom file given
if ($use_content_css)
{
// First check templates folder for default template
// if no editor.css file in templates folder, check system template folder
if (!file_exists($templates_path . '/' . $template . '/css/editor.css'))
{
// If no editor.css file in system folder, show alert
if (!file_exists($templates_path . '/system/css/editor.css'))
{
Log::add(Text::_('PLG_TINY_ERR_EDITORCSSFILENOTPRESENT'), Log::WARNING, 'jerror');
}
else
{
$content_css = Uri::root(true) . '/templates/system/css/editor.css';
}
}
else
{
$content_css = Uri::root(true) . '/templates/' . $template . '/css/editor.css';
}
}
}

avatar dgrammatiko
dgrammatiko - comment - 9 Nov 2021

@laoneo I reverted the tinyMCE part (will do it in another PR, tinyMCE needs a bit more work), so your test should be fine now

avatar dgrammatiko dgrammatiko - change - 9 Nov 2021
The description was changed
avatar dgrammatiko dgrammatiko - edited - 9 Nov 2021
avatar laoneo
laoneo - comment - 9 Nov 2021

Should inheritable be 1 after installation?

avatar dgrammatiko
dgrammatiko - comment - 9 Nov 2021

Should inheritable be 1 after installation?

Yes

avatar laoneo
laoneo - comment - 9 Nov 2021

Still getting the same error, did two times also npm ci.

avatar dgrammatiko
dgrammatiko - comment - 9 Nov 2021

Still getting the same error, did two times also npm ci.

Is this for Autm?

avatar laoneo
laoneo - comment - 9 Nov 2021

No front end.

avatar dgrammatiko
dgrammatiko - comment - 9 Nov 2021

No front end.

Weird, is your db table #__template_styles like:
Screenshot 2021-11-09 at 18 38 18

avatar dgrammatiko
dgrammatiko - comment - 9 Nov 2021

@laoneo can you also apply #36000 ?

avatar dgrammatiko dgrammatiko - change - 9 Nov 2021
The description was changed
avatar dgrammatiko dgrammatiko - edited - 9 Nov 2021
avatar laoneo
laoneo - comment - 10 Nov 2021

With #36000 it works, no error and TinyMCE shows up properly on the front end.

avatar Quy
Quy - comment - 22 Nov 2021

Testing locally with the installable package. The site is in this directory, however, the font is referenced in the root.

Joomla_4.1.0-dev+pr.35874-Development-Full_Package

35874

avatar dgrammatiko
dgrammatiko - comment - 22 Nov 2021

@Quy is this the front end or admin template?

avatar Quy
Quy - comment - 22 Nov 2021

Admin template. No errors on frontend.

avatar dgrammatiko
dgrammatiko - comment - 22 Nov 2021

Thanks, I’ll check it in a bit, also the branch here is out of sync, I will ping you once fixed

@Quy should be fine now

avatar joomla-cms-bot joomla-cms-bot - change - 22 Nov 2021
Category Administration Templates (admin) NPM Change Repository SQL com_admin Postgresql Administration com_admin SQL Postgresql com_templates Language & Strings Templates (admin) NPM Change JavaScript Repository
avatar dgrammatiko dgrammatiko - change - 22 Nov 2021
Labels Added: Language Change
avatar joomla-cms-bot joomla-cms-bot - change - 22 Nov 2021
Category Administration Templates (admin) NPM Change Repository SQL com_admin Postgresql com_templates Language & Strings JavaScript Administration com_admin SQL Postgresql Templates (admin) NPM Change Repository
avatar dgrammatiko dgrammatiko - change - 22 Nov 2021
Labels Removed: Language Change
avatar dgrammatiko dgrammatiko - change - 22 Nov 2021
Title
4.1 Child templates 1/2
4.1 Child templates 1/3
avatar dgrammatiko dgrammatiko - edited - 22 Nov 2021
avatar Quy
Quy - comment - 22 Nov 2021

From the installation page.

GET http://localhost/Joomla_4.1.0-dev+pr.35874-Development-Full_Package/build/media_source/vendor/fontawesome-free/webfonts/fa-solid-900.woff2
[HTTP/1.1 404 Not Found 1ms]

downloadable font: download failed (font-family: "Font Awesome 5 Free" style:normal weight:900 stretch:100 src index:1): status=2147746065 source: http://localhost/Joomla_4.1.0-dev+pr.35874-Development-Full_Package/build/media_source/vendor/fontawesome-free/webfonts/fa-solid-900.woff2
GET http://localhost/Joomla_4.1.0-dev+pr.35874-Development-Full_Package/build/media_source/vendor/fontawesome-free/webfonts/fa-solid-900.woff
[HTTP/1.1 404 Not Found 1ms]

downloadable font: download failed (font-family: "Font Awesome 5 Free" style:normal weight:900 stretch:100 src index:2): status=2147746065 source: http://localhost/Joomla_4.1.0-dev+pr.35874-Development-Full_Package/build/media_source/vendor/fontawesome-free/webfonts/fa-solid-900.woff
GET http://localhost/Joomla_4.1.0-dev+pr.35874-Development-Full_Package/build/media_source/vendor/fontawesome-free/webfonts/fa-solid-900.ttf
[HTTP/1.1 404 Not Found 1ms]

downloadable font: download failed (font-family: "Font Awesome 5 Free" style:normal weight:900 stretch:100 src index:3): status=2147746065 source: http://localhost/Joomla_4.1.0-dev+pr.35874-Development-Full_Package/build/media_source/vendor/fontawesome-free/webfonts/fa-solid-900.ttf
avatar dgrammatiko
dgrammatiko - comment - 22 Nov 2021

Oh, that’s npm related. I will check although I didn’t get any such message couple of hours ago

avatar Quy
Quy - comment - 24 Nov 2021

In \installation\template\css\template.css, it references the build directory.

@font-face {
  font-family: "Font Awesome 5 Free";
  font-style: normal;
  font-weight: 900;
  font-display: block;
  src: url("../../../build/media_source/vendor/fontawesome-free/webfonts/fa-solid-900.eot");
  src: url("../../../build/media_source/vendor/fontawesome-free/webfonts/fa-solid-900.eot?#iefix") format("embedded-opentype"), url("../../../build/media_source/vendor/fontawesome-free/webfonts/fa-solid-900.woff2") format("woff2"), url("../../../build/media_source/vendor/fontawesome-free/webfonts/fa-solid-900.woff") format("woff"), url("../../../build/media_source/vendor/fontawesome-free/webfonts/fa-solid-900.ttf") format("truetype"), url("../../../build/media_source/vendor/fontawesome-free/webfonts/fa-solid-900.svg#fontawesome") format("svg");
}
avatar dgrammatiko
dgrammatiko - comment - 24 Nov 2021

In \installation\template\css\template.css, it references the build directory.

Ohh that's wrong, I will fix it

@Quy should be fixed with 8e884a8 Thanks for spotting it

avatar Krshivam25 Krshivam25 - test_item - 25 Nov 2021 - Tested unsuccessfully
avatar Krshivam25
Krshivam25 - comment - 25 Nov 2021

I have tested this item 🔴 unsuccessfully on fb1a059


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35874.

avatar dgrammatiko
dgrammatiko - comment - 25 Nov 2021

@Krshivam25 what is broken?

avatar richard67
richard67 - comment - 25 Nov 2021

@Krshivam25 When having tested without success, please report back what has not worked for you. Just marking a test as unsuccessful without any additional information is as helpful for us as a PITA.

avatar dgrammatiko dgrammatiko - change - 25 Nov 2021
The description was changed
avatar dgrammatiko dgrammatiko - edited - 25 Nov 2021
avatar Krshivam25
Krshivam25 - comment - 25 Nov 2021

@Krshivam25 what is broken?

I am unable find it
error
After applying patch I am unable to move or refresh
screen-capture (16)

avatar dgrammatiko
dgrammatiko - comment - 25 Nov 2021

@Krshivam25 please download the installable from https://ci.joomla.org/artifacts/joomla/joomla-cms/4.1-dev/35874/downloads/48659/
I have no clue why your npm failed but it's probably irrelevant to the changes in this PR

Oh you CANNOT test this PR with patch tester

avatar dgrammatiko dgrammatiko - change - 25 Nov 2021
The description was changed
avatar dgrammatiko dgrammatiko - edited - 25 Nov 2021
avatar Quy
Quy - comment - 26 Nov 2021

On the frontend, file not found. It should be in the Joomla_4.1.0-dev+pr.35874-Development-Full_Package folder.
http://localhost/media/vendor/joomla-custom-elements/css/joomla-alert.css

avatar dgrammatiko
dgrammatiko - comment - 26 Nov 2021

@Quy good catch!

avatar brianteeman
brianteeman - comment - 27 Nov 2021

Checked out this branch, went to sysstem->database
clicked on update structure.

Two errors

  1. class: path not found
Call stack
#	Function	Location
1	()	JROOT\administrator\components\com_admin\script.php:8508
2	JoomlaInstallerScript->moveRemainingTemplateFiles()	JROOT\administrator\components\com_admin\script.php:7662
3	JoomlaInstallerScript->deleteUnexistingFiles()	JROOT\administrator\components\com_installer\src\Model\DatabaseModel.php:323
4	Joomla\Component\Installer\Administrator\Model\DatabaseModel->fix()	JROOT\administrator\components\com_installer\src\Controller\DatabaseController.php:57
5	Joomla\Component\Installer\Administrator\Controller\DatabaseController->fix()	JROOT\libraries\src\MVC\Controller\BaseController.php:730
6	Joomla\CMS\MVC\Controller\BaseController->execute()	JROOT\libraries\src\Dispatcher\ComponentDispatcher.php:146
7	Joomla\CMS\Dispatcher\ComponentDispatcher->dispatch()	JROOT\libraries\src\Component\ComponentHelper.php:389
8	Joomla\CMS\Component\ComponentHelper::renderComponent()	JROOT\libraries\src\Application\AdministratorApplication.php:143
9	Joomla\CMS\Application\AdministratorApplication->dispatch()	JROOT\libraries\src\Application\AdministratorApplication.php:186
10	Joomla\CMS\Application\AdministratorApplication->doExecute()	JROOT\libraries\src\Application\CMSApplication.php:278
11	Joomla\CMS\Application\CMSApplication->execute()	JROOT\administrator\includes\app.php:63
12	require_once()	JROOT\administrator\index.php:32
  1. The template
    None of the css etc for the template was loaded - just not present at all in the html
    image
avatar brianteeman
brianteeman - comment - 27 Nov 2021

It might be unrelated - the first error prevented the sql from running hence the second error

avatar dgrammatiko
dgrammatiko - comment - 27 Nov 2021

Checked out this branch,

You need to apply the db changes as well (change all the rows of the #__template_styles table so that inheritable is 1)
Screenshot 2021-11-27 at 10 46 11

avatar brianteeman
brianteeman - comment - 27 Nov 2021

@dgrammatiko thats what the database update structure should have done

avatar brianteeman
brianteeman - comment - 27 Nov 2021

ok - can confirm that the path not found error is also not related to this PR

avatar dgrammatiko
dgrammatiko - comment - 27 Nov 2021

I can also confirm that path is missing. Adding use Joomla\Filesystem\Path; solved it for me. Probably a PR on its own?

avatar brianteeman
brianteeman - comment - 27 Nov 2021

Correction

The problem is in this PR with
protected function moveRemainingTemplateFiles()

It should NOT run when you are doing a database update structure

avatar richard67
richard67 - comment - 27 Nov 2021

@brianteeman You should meanwhile know that the "Update Structure" button in the database view only updates structure, i.e. runs DDL (data definition language) statements and not updates content, i.e. not runs DML (data manipulation) statements. The update SQL scripts here do not contain any DDL, only DML ("UPDATE" statements). So these statements are not executed when using the button. The update SQL will be run when using the update package created by drone for this PR.

avatar brianteeman
brianteeman - comment - 27 Nov 2021

Either way this should not have happened and the problem code is in this pr

avatar dgrammatiko
dgrammatiko - comment - 27 Nov 2021

FWIW I added the missing import. For the logic in the script.php I think @richard67 is right, the update happens only when you use joomlaupdate or installing directly 4.1

avatar richard67
richard67 - comment - 27 Nov 2021

@dgrammatiko When installing 4.1 directly, no update happens. Or did you mean install 4.1 without your PR and then update to the package with this PR? This will work.

avatar brianteeman
brianteeman - comment - 27 Nov 2021

Expected

image

Actual

image

avatar dgrammatiko
dgrammatiko - comment - 27 Nov 2021

I meant that installing 4.1 the db has the correct data (so no update is needed). The whole issue with the db is for those upgrading to 4.1 as they have to alter one of the template columns

Expected

This is the com_templates extension which needs the PR 2/3 to handle properly the templates in the new mode. Also, there's a PR for tinyMCE to handle correctly the editor.css and the HTML templates

avatar brianteeman
brianteeman - comment - 27 Nov 2021

This is the com_templates extension which needs the PR 2/3 to handle properly the templates in the new mode. Also, there's a PR for tinyMCE to handle correctly the editor.css and the HTML templates

I am just testing according to your instructions

Check that the back end template is not missing any assets and doesn't have any visual regression

avatar richard67
richard67 - comment - 27 Nov 2021

I meant that installing 4.1 the db has the correct data (so no update is needed). The whole issue with the db is for those upgrading to 4.1 as they have to alter one of the template columns

@dgrammatiko Yes, that’s right. And by my review the changes in base.sql and the new update SQL scripts are correct and consistent.

avatar dgrammatiko dgrammatiko - change - 27 Nov 2021
The description was changed
avatar dgrammatiko dgrammatiko - edited - 27 Nov 2021
avatar dgrammatiko
dgrammatiko - comment - 27 Nov 2021

I am just testing according to your instructions

Oops my bad. Updated the description to:

  • Check that the back end template is not missing any assets and doesn't have any visual regression (skip the extension templates, there's a PR #35998 for that)
avatar richard67
richard67 - comment - 27 Nov 2021

@brianteeman So using the "Update Structure" button was not the right way to apply the update SQL for this PR, but it is good that you've tried it because it has shown the missing use statement.

The fact that the buttons runs also some stuff from script.php is an ugly hack which is a long standing issue, I think I remember that we even have an issue created by you here somewhere that this button does more than what it says. It is not related to this PR and should be solved somehow, but it's not an easy thing and I have no idea yet what's a good way to fix it.

avatar dgrammatiko
dgrammatiko - comment - 27 Nov 2021

@richard67 unrelated to this PR but I think for sake of sanity debugging anything on the script.php the long arrays need to be moved at the end of the file (eg with a fn getFiles, getFolders, etc). The file is a huge pain to even scroll to read the code... I always was in favour of separating the data from the actual code but as we all remember that didn't fly

avatar richard67
richard67 - comment - 27 Nov 2021

@richard67 unrelated to this PR but I think for sake of sanity debugging anything on the script.php the long arrays need to be moved at the end of the file (eg with a fn getFiles, getFolders, etc). The file is a huge pain to even scroll to read the code... I always was in favour of separating the data from the actual code but as we all remember that didn't fly

@dgrammatiko I agree and was working on that, too.

avatar dgrammatiko dgrammatiko - change - 27 Nov 2021
Title
4.1 Child templates 1/3
4.1 Child templates 1/2
avatar dgrammatiko dgrammatiko - edited - 27 Nov 2021
avatar richard67
richard67 - comment - 28 Nov 2021

@dgrammatiko I've just made an update from a 4.1-dev to the package of this PR using the custom URL provided by drone https://ci.joomla.org/artifacts/joomla/joomla-cms/4.1-dev/35874/downloads/48742/pr_list.xml, and it fails. I get the following error in my PHP error log:

PHP Fatal error:  Uncaught Error: Class 'Joomla\\CMS\\Filesystem\\Path' not found in /joomla-cms-4.1-dev/administrator/components/com_admin/script.php:8509
Stack trace:
#0 /joomla-cms-4.1-dev/administrator/components/com_admin/script.php(7663): JoomlaInstallerScript->moveRemainingTemplateFiles()
#1 /joomla-cms-4.1-dev/administrator/components/com_joomlaupdate/finalisation.php(76): JoomlaInstallerScript->deleteUnexistingFiles()
#2 /joomla-cms-4.1-dev/administrator/components/com_joomlaupdate/extract.php(2035): finalizeUpdate()
#3 {main}
  thrown in /joomla-cms-4.1-dev/administrator/components/com_admin/script.php on line 8509,
referer: https://www.joomla-41-dev.vmubu01.vmnet2.local/administrator/index.php?option=com_joomlaupdate&task=update.install&799181f416cdd0b202b9097d0fd2d515=1

I have no explanation for that because in the update package "Joomla_4.1.0-dev+pr.35874-Development-Full_Package.zip" the script.php file contains the necessary "use" statement which you had added with commit dc65503 , and the "libraries/src/Filesystem/Path.php" of course exists, too.

Update: Could be a problem on my environment only.

Update of update: No, it's not the permission problem I had on my environment.

avatar dgrammatiko
dgrammatiko - comment - 28 Nov 2021

So, it turns out that the use is worthless in the finalization step (we don't have JLOADER at that point). We need to mock the Path here:

similar to the File, Folder that are already there. Let me know if you're happy doing that or I'll add it myself

avatar richard67
richard67 - comment - 28 Nov 2021

So, it turns out that the use is worthless in the finalization step (we don't have JLOADER at that point). We need to mock the Path here:


similar to the File, Folder that are already there.

@dgrammatiko You are right .. how could I forget about that? I should have known.

Let me know if you're happy doing that or I'll add it myself

I can do that, but that would create a dependency of this PR here to my PR. So maybe it's better if you or me do it with this PR here. If you want I can make a PR for you, but if you know how to do it, do it yourself, I am also happy with that.

avatar richard67
richard67 - comment - 28 Nov 2021

@dgrammatiko Do we really need to use path clean here? In the files and folders removal as well as in the "fixFilenameCasing" we don't use it. The difference here now is that with your new method we deal with files and folders which we don't know in advance, so they might have weird names or be symbolic links to forbidden places, so it could make sense. But I'm not sure about that.

avatar dgrammatiko
dgrammatiko - comment - 28 Nov 2021

Do we really need to use path clean here?

I'm not sure if it's needed. Also it might be easier if we add a protected fn cleanPath with the code from

public static function clean($path, $ds = DIRECTORY_SEPARATOR)
{
if (!\is_string($path) && !empty($path))
{
throw new \UnexpectedValueException(
sprintf(
'%s() - $path is not a string',
__METHOD__
)
);
}
$path = trim($path);
if (empty($path))
{
$path = JPATH_ROOT;
}
// Remove double slashes and backslashes and convert all slashes and backslashes to DIRECTORY_SEPARATOR
// If dealing with a UNC path don't forget to prepend the path with a backslash.
elseif (($ds === '\\') && substr($path, 0, 2) === '\\\\')
{
$path = "\\" . preg_replace('#[/\\\\]+#', $ds, $path);
}
else
{
$path = preg_replace('#[/\\\\]+#', $ds, $path);
}
return $path;
instead of adding the class path in the finalization. I mean if we need this functionality we have to repeat the code either in the finalization or in the script. If it's in the script could be smaller:

	pravate function cleanPath($path)
	{
		$path = trim($path);

		// Remove double slashes and backslashes and convert all slashes and backslashes to DIRECTORY_SEPARATOR
		// If dealing with a UNC path don't forget to prepend the path with a backslash.
		if ((DIRECTORY_SEPARATOR === '\\') && substr($path, 0, 2) === '\\\\')
		{
			$path = "\\" . preg_replace('#[/\\\\]+#', DIRECTORY_SEPARATOR, $path);
		}
		else
		{
			$path = preg_replace('#[/\\\\]+#', DIRECTORY_SEPARATOR, $path);
		}

		return $path;
	}
avatar richard67
richard67 - comment - 28 Nov 2021

@dgrammatiko Regarding simplify: It doesn't even need the trim. I remember now that we had to use the path clean here in order to get the same directory separators in the $oldPath and the $oldFile to make the substr($oldFile, strlen($oldPath) thing work.

Possibly we could use realpath from PHP instead to achieve the same?

avatar richard67
richard67 - comment - 28 Nov 2021

@dgrammatiko I think that's it: Use realpath instead of Path::clean at these 2 places and remove the use Joomla\CMS\Filesystem\Path;.

avatar dgrammatiko
dgrammatiko - comment - 28 Nov 2021

@richard67 let me know if you're ok with ebff3bb

avatar richard67
richard67 - comment - 28 Nov 2021

@dgrammatiko Looks good. Will test and report back.

avatar richard67
richard67 - comment - 28 Nov 2021

@dgrammatiko Now we run into the next one:

PHP Fatal error:  Uncaught Error: Call to undefined method Joomla\\CMS\\Filesystem\\Folder::files() in /home/richard/lamp/public_html/joomla-cms-4.1-dev/administrator/components/com_admin/script.php:8512

So either we add that to the Folder mock class in finalisation.php, or we use glob or readdir here.

avatar dgrammatiko
dgrammatiko - comment - 28 Nov 2021

If glob is safe in this context I would personally go with that

avatar richard67
richard67 - comment - 28 Nov 2021

If we solve this, the next one will be the Folder:create.

avatar richard67
richard67 - comment - 28 Nov 2021

@dgrammatiko I think "glob" won't be recursive into the subfolders,

avatar richard67
richard67 - comment - 28 Nov 2021

@dgrammatiko Could we use "RecursiveDirectoryIterator" instead of "glob"?

avatar dgrammatiko
dgrammatiko - comment - 28 Nov 2021

So checking how things were done in the extrct.php I used mkdir:

private function createDirectory(): void
{
// Do we need to create a directory?
if (empty($this->fileHeader->realFile))
{
$this->fileHeader->realFile = $this->fileHeader->file;
}
$lastSlash = strrpos($this->fileHeader->realFile, '/');
$dirName = substr($this->fileHeader->realFile, 0, $lastSlash);
$perms = 0755;
$ignore = $this->isIgnoredDirectory($dirName);
if (@is_dir($dirName))
{
return;
}
if ((@mkdir($dirName, $perms, true) === false) && (!$ignore))
{
$this->setError(sprintf('Could not create %s folder', $dirName));
}
}

Also RecursiveDirectoryIterator replaced the Folder::files().

16a3e5f 28 Nov 2021 avatar dgrammatiko Meh
avatar richard67
richard67 - comment - 28 Nov 2021

@dgrammatiko I have made some debug output into script.php and tested the update. The iterator doesn't only contain files but also folders, including "." and "..". We have to filter those out so we have only files at the end.

avatar richard67
richard67 - comment - 28 Nov 2021

@dgrammatiko Am working on a fix.

avatar dgrammatiko
dgrammatiko - comment - 28 Nov 2021

Am working on a fix.

Check 1d2d60a

avatar richard67
richard67 - comment - 28 Nov 2021

@dgrammatiko Possibly it might not work, see https://stackoverflow.com/questions/6337810/why-does-isdot-fail-on-me-php ... but I haven't tested yet, and I have to interrupt soon for an hour or two to do something else.

avatar richard67
richard67 - comment - 28 Nov 2021

@dgrammatiko Maybe you just can use flag FilesystemIterator::SKIP_DOTS when creating the directory iterator?

avatar dgrammatiko
dgrammatiko - comment - 28 Nov 2021

@richard67 I switched the code to use the flag. Docs say that dots are ignored by default, but yeah PHP...

avatar richard67
richard67 - comment - 28 Nov 2021

@dgrammatiko My debug output shows there goes something wrong. I have the output just after the $newFile = $newPath . substr($oldFile, strlen($oldPath)); line.

DEBUG: oldFile="/home/richard/lamp/public_html/joomla-cms-4.1-dev/templates/cassiopeia/css/user.css",
       newFile="/home/richard/lamp/public_html/joomla-cms-4.1-dev/media/templates/site/cassiopeia/css"
DEBUG: oldFile="/home/richard/lamp/public_html/joomla-cms-4.1-dev/templates/cassiopeia/images/MyImages/2021-11-27_colored.png",
       newFile="/home/richard/lamp/public_html/joomla-cms-4.1-dev/media/templates/site/cassiopeia/images"
DEBUG: oldFile="/home/richard/lamp/public_html/joomla-cms-4.1-dev/templates/cassiopeia/js/gaga.js",
       newFile="/home/richard/lamp/public_html/joomla-cms-4.1-dev/media/templates/site/cassiopeia/js"
DEBUG: oldFile="/home/richard/lamp/public_html/joomla-cms-4.1-dev/templates/cassiopeia/scss/global/gaga.scss",
       newFile="/home/richard/lamp/public_html/joomla-cms-4.1-dev/media/templates/site/cassiopeia/scss"

Old file is right, new file is missing the part after the new parent folder.

Could be my mistake since I had proposed that code as far as I remember.

Update: No, it was the $oldPath = $oldFile->getPathname(); which you have added. It needs to remove that.

avatar richard67
richard67 - comment - 28 Nov 2021

@dgrammatiko First the good news: Moving the custom files looks ok now. Here my debug output:

DEBUG: oldFile="/home/richard/lamp/public_html/joomla-cms-4.1-dev/templates/cassiopeia/css/user.css",
       newFile="/home/richard/lamp/public_html/joomla-cms-4.1-dev/media/templates/site/cassiopeia/css/user.css"

DEBUG: oldFile="/home/richard/lamp/public_html/joomla-cms-4.1-dev/templates/cassiopeia/images/MyImages/2021-11-27_colored.png",
       newFile="/home/richard/lamp/public_html/joomla-cms-4.1-dev/media/templates/site/cassiopeia/images/MyImages/2021-11-27_colored.png"

DEBUG: oldFile="/home/richard/lamp/public_html/joomla-cms-4.1-dev/templates/cassiopeia/images/MyImages/MyOtherImages/2021-11-27_monochrome.png",
       newFile="/home/richard/lamp/public_html/joomla-cms-4.1-dev/media/templates/site/cassiopeia/images/MyImages/MyOtherImages/2021-11-27_monochrome.png"

DEBUG: oldFile="/home/richard/lamp/public_html/joomla-cms-4.1-dev/templates/cassiopeia/js/gaga.js",
       newFile="/home/richard/lamp/public_html/joomla-cms-4.1-dev/media/templates/site/cassiopeia/js/gaga.js"

DEBUG: oldFile="/home/richard/lamp/public_html/joomla-cms-4.1-dev/templates/cassiopeia/scss/global/gaga.scss",
       newFile="/home/richard/lamp/public_html/joomla-cms-4.1-dev/media/templates/site/cassiopeia/scss/global/gaga.scss"

And all files are present at the new location.

But after the update my backend looks like this:

2021-11-28_pr-35874_1

Network analysis in browser's developer tools shows status 200 everywhere, all green. No errors in browser console.

avatar richard67
richard67 - comment - 28 Nov 2021

And after this update npm ci fails when compiling css:

Error: Can't find stylesheet to import.
  ╷
2 │ @import "../../../administrator/templates/atum/scss/variables";
  │         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵
  installation/template/scss/template.scss 2:9      @import
  installation/template/scss/template-rtl.scss 1:9  root stylesheet

Update: This is expected behaviour since the update package doesn't update the installation folder. So pls. ignore this comment.

avatar richard67
richard67 - comment - 28 Nov 2021

@dgrammatiko I think the CSS and probably also JS issues after an update could come from assets not being searched at the right place for some reason. Maybe with previous tests by other people this was not observed because applying the patch did not remove the old assets while the update package correctly removes the assets from the old place, and maybe with new installation all is fine?.

avatar dgrammatiko
dgrammatiko - comment - 28 Nov 2021

@richard67 is this #35920 related (should be the problem with missing media files in the update)

avatar richard67
richard67 - comment - 28 Nov 2021

@richard67 is this #35920 related (should be the problem with missing media files in the update)

No, the media files are there in the update package built by drone because that's always the full update package while the issue exists only for the "small" update package not used by the live update.

Maybe the issue is that other successful testers used Windows and my server is on Linux?

avatar dgrammatiko
dgrammatiko - comment - 28 Nov 2021

@richard67 can you check that in the #__templates_styles all the rows for cassiopeia and atum have the column parent set to 1

avatar richard67
richard67 - comment - 28 Nov 2021

@dgrammatiko Ouch .. I should have known. The update did not run your update SQL script because the version in the file name is too old. Rename the scripts from "4.1.0-2021-10-22.sql" to "4.1.0-2021-11-28.sql" and it will work.

avatar richard67
richard67 - comment - 28 Nov 2021

@dgrammatiko ... you know, in the 4.1-dev branch we meanwhile have update SQL with a newer date from the task scheduler, so the database schema version is already greater than the one of your update SQL so it does not run on update.

avatar richard67
richard67 - comment - 28 Nov 2021

@dgrammatiko Yeah !!! I think that was it. Now I gotta make some 1 hour break due to dinner and then I will test this one and the other PR.

avatar dgrammatiko
dgrammatiko - comment - 28 Nov 2021

you know, in the 4.1-dev branch we meanwhile have update SQL with a newer date from the task scheduler, so the database schema version is already greater than the one of your update SQL so it does not run on update.

Now that you wrote it I realised that this was the problem. Finally this should be safe now

avatar richard67
richard67 - comment - 28 Nov 2021

I am very optimistic now. CU later.

avatar richard67
richard67 - comment - 28 Nov 2021

@dgrammatiko Loading the local Roboto fonts, i.e. fonts scheme "Roboto (local)" doesn't work. Fonts from web do work.

The network analysis in browser's developer tools shows an error loading for a resource which seems to be a version modifier without a file name, "?b3fc84e13caa5c536799085ad5741261".

avatar richard67
richard67 - comment - 28 Nov 2021

@dgrammatiko The reason for the local roboto font not working is that you need to fix the path to the css file here: https://github.com/dgrammatiko/joomla-cms/blob/4.1-dev-child-templates/templates/cassiopeia/templateDetails.xml#L97

avatar richard67
richard67 - comment - 28 Nov 2021

P.S.: We might need to update that path in the database in the template style settings, too. I will prepare an add-on for your update SQL scripts for that with a PR to your branch together with the fix for the XML.

avatar richard67
richard67 - comment - 28 Nov 2021

@dgrammatiko See dgrammatiko#8 for the update SQL.

avatar dgrammatiko
dgrammatiko - comment - 28 Nov 2021

@richard67 thank you for the PR. Now the roboto font also should be fixed

avatar richard67
richard67 - comment - 28 Nov 2021

I have tested this item successfully on 16a3e5f


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35874.

avatar richard67 richard67 - test_item - 28 Nov 2021 - Tested successfully
avatar richard67
richard67 - comment - 28 Nov 2021

That was a long struggle but it was worth it.

avatar richard67 richard67 - alter_testresult - 28 Nov 2021 - richard67: Tested successfully
avatar chmst chmst - test_item - 28 Nov 2021 - Tested successfully
avatar chmst
chmst - comment - 28 Nov 2021

I have tested this item successfully on 16a3e5f

Tested on a 4.0 installation where an own template (derived from cassiopeia). After PR thePR template is unchanged, files of cassiopeia and atum are moved as described.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35874.
avatar richard67 richard67 - change - 28 Nov 2021
Status Pending Ready to Commit
avatar richard67
richard67 - comment - 28 Nov 2021

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35874.

avatar brianteeman
brianteeman - comment - 28 Nov 2021

There is still time (GMT) for this to be merged on @dgrammatiko birthday if you're quick

avatar richard67
richard67 - comment - 28 Nov 2021

@dgrammatiko Happy (belated) birthday.

avatar richard67
richard67 - comment - 28 Nov 2021

So the name of the update SQL scripts is very suitable, it seems, if that’s the birthday of the author :-)

avatar Quy
Quy - comment - 29 Nov 2021

It is not possible to create overrides.

Failed to create override.

avatar dgrammatiko
dgrammatiko - comment - 29 Nov 2021

@Quy can you write the steps you followed to get this error?

avatar Quy
Quy - comment - 29 Nov 2021

Go to System > Site Templates
Click Cassiopeia Details and Files
Click Create Overrides tab
Click mod_articles_archive

avatar Quy
Quy - comment - 29 Nov 2021

Some directories/files are missing.

4.1.0
editor

4.1.0+35874
editor-35874

avatar dgrammatiko
dgrammatiko - comment - 29 Nov 2021

This is expected. This is the first of 2 PRs. To get the whole picture try to apply also the second pr

avatar richard67
richard67 - comment - 29 Nov 2021

I had no problems creating an override, but it was for the language switcher module.

avatar Quy
Quy - comment - 29 Nov 2021

Applied the other PR and still cannot create overrides. Testing locally with WampServer on Windows 10.

avatar Quy
Quy - comment - 29 Nov 2021

No errors in Console and PHP error log.

avatar richard67
richard67 - comment - 29 Nov 2021

@Quy How did you apply the patches?

If you are using patchtester, maybe that has a problem?

I had updated a clean 4.1-dev to the branch of this PR using the custom URL created by drone, and the same I did with the other PR (and then without this one here) on a clean 4.1-dev. After that I had tested both together by creating a new branch based on 4.1-dev, merging both branches of both PRs into that one and made a new installation using that branch (with composer install and nmp ci of course).

avatar Quy
Quy - comment - 29 Nov 2021

Using the prebuilt package and Joomla update to manually install the other PR.

avatar richard67
richard67 - comment - 29 Nov 2021

Hmm, maybe an problem on Windows? I've tested with a Linux server.

avatar dgrammatiko
dgrammatiko - comment - 29 Nov 2021

@Quy it seems to work fine here
Screenshot 2021-11-29 at 18 33 02

Also, the changes in this PR have nothing to do with the code in com_content that handles the creation of the overrides and on top of that the layout overrides didn't had any changes here (I mean the HTML directory wasn't moved). Can you try the same in a vanilla 4.1 installation? Maybe there's something else affecting permissions or there's a bug that you just discovered. My point is that this PR should not affect in any shape or form the functionality for creating layout overrides...

avatar Quy
Quy - comment - 29 Nov 2021

Tested fine with the 4.1.0 nightly build from today. https://developer.joomla.org/nightly-builds.html

avatar dgrammatiko
dgrammatiko - comment - 29 Nov 2021

@Quy are there any JS errors? Can you share a video of the process, I'm really curious why this is failing (the code changes here are unrelated to the process)

avatar Quy
Quy - comment - 29 Nov 2021

No JS/PHP errors.

create-overrides.mp4
avatar dgrammatiko
dgrammatiko - comment - 29 Nov 2021

@Quy applying only this PR you shouldn't have any preview images in the templates list. I'm guessing there's something wrong with the packages (?). Once again this PR is NOT touching at all the com_templates which is the extension that is misbehaving here...

avatar Quy
Quy - comment - 29 Nov 2021

This installation includes the other PR. I get the same error with this PR by itself. I will try to troubleshoot later and report any findings.

avatar Quy
Quy - comment - 29 Nov 2021

The $override parameter to createOverride is empty in this PR but not in the 4.1 nightly build.

avatar dgrammatiko
dgrammatiko - comment - 29 Nov 2021

@Quy Can you clear your browser's cache reload the page and send me the list of js files loaded?

avatar Quy
Quy - comment - 29 Nov 2021

Hopefully this is it of the Create Overrides page.

35874-js

avatar dgrammatiko
dgrammatiko - comment - 29 Nov 2021

@Quy try this:

  • open the browser inspector
  • right click on the module override link and inpect
  • check/copy the href

Screenshot 2021-11-29 at 21 00 31

But ONCE AGAIN: this is unrelated to what this PR is doing!!!. This PR just moves some static files, nothing to do with Overrides!!!

avatar Quy
Quy - comment - 29 Nov 2021

http://localhost/Joomla_4.1.0-dev+pr.35874-Development-Full_Package/administrator/index.php?option=com_templates&view=template&task=template.overrides&folder=Qzpcd2FtcDY0XHd3d1xKb29tbGFfNC4xLjAtZGV2K3ByLjM1ODc0LURldmVsb3BtZW50LUZ1bGxfUGFja2FnZVxtb2R1bGVzXG1vZF9hcnRpY2xlc19hcmNoaXZl&id=218&file=aG9tZQ&33c1bf54cc8ea6301b9a91a7b4e85521=1

avatar Quy
Quy - comment - 29 Nov 2021

But ONCE AGAIN: this is unrelated to what this PR is doing!!!. This PR just moves some static files, nothing to do with Overrides!!!

I don't want to waste any more of your time. Let's get this in and worry about it later if it persists. Thank you!

avatar dgrammatiko
dgrammatiko - comment - 29 Nov 2021

Screenshot 2021-11-29 at 21 15 31

So, the part folder=... is the base64 encoded part of the override that you asked the component to create
You said that the $override (i guess in the controller) is empty. But if that is failing is due to this:

And this part wasn't touched by this PR thus my surprise here. This shouldn't fail with this PR...

avatar Quy
Quy - comment - 29 Nov 2021

The + is the folder Joomla_4.1.0-dev+pr.35874-Development-Full_Package caused the issue and unrelated to this PR. Sorry for the false alarm.

avatar dgrammatiko
dgrammatiko - comment - 29 Nov 2021

Wow, that's actually a very good catch!!
@HLeithner I think you need to rename the packages, it seems the + will be problematic on Windows for some cases.

avatar bembelimen bembelimen - change - 30 Nov 2021
Labels Added: ? Documentation Required
avatar bembelimen bembelimen - change - 30 Nov 2021
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2021-11-30 02:34:52
Closed_By bembelimen
avatar bembelimen bembelimen - close - 30 Nov 2021
avatar bembelimen bembelimen - merge - 30 Nov 2021
avatar bembelimen
bembelimen - comment - 30 Nov 2021

Thx, awesome job @dgrammatiko and thanks for all the feedbacks

avatar dgrammatiko
dgrammatiko - comment - 30 Nov 2021

Thanks, @bembelimen also thanks to everyone contributing and testing here, especially @richard67 for the update patches!!

avatar HLeithner
HLeithner - comment - 30 Nov 2021

Wow, that's actually a very good catch!! @HLeithner I think you need to rename the packages, it seems the + will be problematic on Windows for some cases.

know your environment ;-)

Add a Comment

Login with GitHub to post a comment