? ? Pending

User tests: Successful: Unsuccessful:

avatar Bakual
Bakual
26 Apr 2020

Pull Request for Issue #28796 and #26259.

Summary of Changes

  • Adding logic to the template copy method to take care of language files in global language folder and media assets in /media
  • Adjusted the templates manifest files: removing the language prefix, removing language folder from Cassiopeia, adding media folder to Atum
  • Fixing the file filter to match all .ini files (it has to be regex)

Testing Instructions

Test by copying a template in frontend and backend.

Expected result

Works and all related files are copied and properly renamed where applicable.

Actual result

  • Cassiopeia fails with an error
  • Atum is copied but not its assets in /media/templates/atum/js

Documentation Changes Required

None

avatar Bakual Bakual - open - 26 Apr 2020
avatar Bakual Bakual - change - 26 Apr 2020
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 26 Apr 2020
Category Administration com_templates Templates (admin) Front End Templates (site)
avatar coolcat-creations
coolcat-creations - comment - 26 Apr 2020

I have tested this item successfully on 0e764d2

Thank you, tested successfully - copied template also works in frontend


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

avatar coolcat-creations coolcat-creations - test_item - 26 Apr 2020 - Tested successfully
avatar Bakual Bakual - change - 26 Apr 2020
Labels Added: ?
avatar ChristineWk
ChristineWk - comment - 26 Apr 2020

I have tested this item successfully on 6503481


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

avatar ChristineWk ChristineWk - test_item - 26 Apr 2020 - Tested successfully
avatar richard67
richard67 - comment - 26 Apr 2020

@coolcat-creations Could you redo your test after the latest changes? Thanks in advance.

avatar Quy Quy - change - 26 Apr 2020
Status Pending Ready to Commit
avatar Quy
Quy - comment - 26 Apr 2020

RTC

Last change is a typo. No need to retest.


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

avatar richard67
richard67 - comment - 26 Apr 2020

OK, fine.

avatar richard67 richard67 - alter_testresult - 26 Apr 2020 - coolcat-creations: Tested successfully
avatar richard67 richard67 - close - 26 Apr 2020
avatar richard67 richard67 - merge - 26 Apr 2020
avatar richard67 richard67 - change - 26 Apr 2020
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2020-04-26 18:21:04
Closed_By richard67
Labels Added: ?
avatar richard67
richard67 - comment - 26 Apr 2020

Thanks to all!

avatar infograf768
infograf768 - comment - 26 Apr 2020

Not on desktop right now to be able to test. I think I know the reply but better be sure before it is merged.
A simple question : does the pr copy ini files from all installed languages or only from the languages stated in the manifest?
Does it copy these files in a specific language folder in the new template itself?

avatar richard67
richard67 - comment - 26 Apr 2020

@infograf768 Sorry, seems I was too fast.

avatar richard67
richard67 - comment - 26 Apr 2020

A simple question : does the pr copy ini files from all installed languages or only from the languages stated in the manifest?

Copying the language files from the global languages folders seems to be limited to the languages stated in the manifest file, as far as I can read in the code.

Does it copy these files in a specific language folder in the new template itself?

I don't know, maybe this is covered by copying any of the sub folders? In this case it would copy all languages files in the sub folder, not only those stated in the manifest file.

@Bakual Can you check and if necessary correct and complete my answers?

avatar Bakual
Bakual - comment - 26 Apr 2020

does the pr copy ini files from all installed languages or only from the languages stated in the manifest?

I deliberately did only take care of the language files shipped with the template (or core, for the core templates). Which means the ones specified in the manifest.

If we want to take care of possible translations coming from language packs, then it gets a bit more complex as we will have to split the language file path and replace the language prefix(es), while looking up prefixed and non-prefixed language files.
If we want to do that, we have to do in a separate PR as it much more complex than this one here ?

avatar richard67
richard67 - comment - 26 Apr 2020

So I will not be tarred and feathered now because I've merged?

avatar infograf768
infograf768 - comment - 27 Apr 2020

Does it copy these files in a specific language folder in the new template itself?

Reply to myself: Testing here shows it copies the ini files in the global language folder.

I created a site template named "another" containing a language folder to test copy.

BUG: We have an error when copying if the language files of the original template have not been installed in the core folders.


JFile: :copy: Can't find or read file: /Applications/MAMP/htdocs/testinstall/joomla40/language/en-GB/tpl_another.ini

JFile: :copy: Can't find or read file: /Applications/MAMP/htdocs/testinstall/joomla40/language/en-GB/tpl_another.sys.ini

I guess it's just a matter of checking if $src exists before using File::copy($src, $dst);

avatar richard67
richard67 - comment - 27 Apr 2020

@Bakual Will you make a new PR later to fix @infograf768 's findings?

avatar richard67
richard67 - comment - 27 Apr 2020

I created a site template named "another" containing a language folder to test copy.

@infograf768 And has that been copied? Or not?

avatar infograf768
infograf768 - comment - 27 Apr 2020

It has been copied, including its language folder. We just have this Error stated above.

avatar Bakual
Bakual - comment - 27 Apr 2020

We have an error when copying if the language files of the original template have not been installed in the core folders.

Sure, I can add a check with File::exists. But I don't understand how you get there, there should be no way that the manifest has files declared which are not installed except if the user manually deleted them. Or do I miss something?

avatar infograf768
infograf768 - comment - 27 Apr 2020

A template or any extension may have a language folder ( <folder>language</folder> ) but not necessarily the install part in global folders

	<languages folder="language">
		<language tag="en-GB">en-GB/en-GB.whatever.ini</language>
		<language tag="en-GB">en-GB/en-GB.whetever.sys.ini</language>
	</languages>
avatar richard67
richard67 - comment - 27 Apr 2020
	<languages folder="language">
		<language tag="en-GB">en-GB/en-GB.whatever.ini</language>
		<language tag="en-GB">en-GB/en-GB.whetever.sys.ini</language>
	</languages>

Shouldn't it be

	<languages folder="language">
		<language tag="en-GB">en-GB/whatever.ini</language>
		<language tag="en-GB">en-GB/whetever.sys.ini</language>
	</languages>

on J4?

avatar infograf768
infograf768 - comment - 27 Apr 2020

sure. that was not the point

avatar richard67
richard67 - comment - 27 Apr 2020

@Bakual What I could see in the code is that you just check the languages $manifest->languages in the XML, but not the particular files, these you don't check in the XML. You just assume that in the global language folder a file for that language exists. Is that how it is meant in the XML? Or can the $manifest->languages also be set in the XML if the template doesn't have language files in the global folder?

avatar Bakual
Bakual - comment - 27 Apr 2020

A template or any extension may have a language folder ( language ) but not necessarily the install part in global folders

It's either or, not both. The extension either ships their files in its own language folder (preferred way) with the <folder> tag or it installs the files into the global language folder (core way) with the <language> tag.

avatar infograf768
infograf768 - comment - 27 Apr 2020

and by the way, it would not install in global folders with that code as, if we have a
<folder>language</language>
We would need a code like

<languages>
		<language tag="fr-FR">language/fr-FR/whatever.ini</language>
		<language tag="fr-FR">language/fr-FR/whatever.sys.ini</language>
	</languages>

to install in global folders...
which is also possible and can make sense.

avatar Bakual
Bakual - comment - 27 Apr 2020

@richard67 If the manifest contains entries for language files that don't exist, it will throw an error during installation of said template. So that shouldn't happen or the developer is a very bad guy you should stay away from ?

But I will add the checks, just to be sure.

avatar infograf768
infograf768 - comment - 27 Apr 2020

In any case, better be safe than sorry and just add the conditional...

avatar richard67
richard67 - comment - 27 Apr 2020

@Bakual Most developers are bad guys you should stay away from, but that's not always related to code only ;-)

avatar infograf768
infograf768 - comment - 27 Apr 2020

Basically, a template does not have to place its ini files in a folder, but if it does, then installing in global needs the code above...

Using this without a language folder

 <languages folder="language">
		<language tag="en-GB">en-GB/whatever.ini</language>
		<language tag="en-GB">en-GB/whetever.sys.ini</language>
	</languages>

does not install in global folders... ;)

avatar Bakual
Bakual - comment - 27 Apr 2020
avatar infograf768
infograf768 - comment - 27 Apr 2020

Shall not we also correct

	<languages folder="language">
		<language tag="en-GB">en-GB/tpl_atum.ini</language>
		<language tag="en-GB">en-GB/tpl_atum.sys.ini</language>
	</languages>

and same for cassiopea?

avatar Bakual
Bakual - comment - 27 Apr 2020

This was part of this PR and also mentioned in the PR description ?

avatar infograf768
infograf768 - comment - 27 Apr 2020

Sorry but it does not make sense. A template which would copy the core templatedetails.xml will never install files in the global folder with that code.
Please reconsider.

avatar ChristineWk
ChristineWk - comment - 27 Apr 2020

Just a question: In the current 4.0.0-dev (april 25th) it's shown:

<languages folder="language"> <language tag="en-GB">en-GB/en-GB.tpl_cassiopeia.ini</language> <language tag="en-GB">en-GB/en-GB.tpl_cassiopeia.sys.ini</language> </languages>

So, here:
<language tag="en-GB">en-GB/whatever.ini</language> <language tag="en-GB">en-GB/whetever.sys.ini</language>

I'm missing the dot - or not?

avatar richard67
richard67 - comment - 27 Apr 2020

@ChristineWk No, the thing is that in J4 meanwhile the file names don't have a language code prefix, i.e. e.g. "en-GB/en-GB.whatever.sys.ini" (old) becomes "en-GB/whatever.sys.ini". Atum's and Cassiopeia's XML's didn't reflect that on April 25, this PR here corrected that, too.

avatar infograf768
infograf768 - comment - 27 Apr 2020

Concernng my comments above with installation of the ini files, please forget. I had discovered the template and not installed it. In case of discovery the inis have to be in the global folder already before discover.
Was good though as it showed the possible error witch is now corrected in #28823

Add a Comment

Login with GitHub to post a comment