Fix parseXMLTemplateFile invoke in TemplateModel.php (Create child template button is not displayed)

[sergeytolkachyov]

Fix parseXMLTemplateFile with $template->name param. Change to $template->element due to $template->name might be a language constant

[chmst]

Thank you for our PR. Could you please add some more information?

Testing instructions?

If this can be tested successfully, your second PR for J5 #41314 is not necessary as this one will be upmerged automatically.

[richard67]

A useful title instead of „4.4-dev“ would also be good.

[sergeytolkachyov]

Thank you for our PR. Could you please add some more information?
Testing instructions?

I just review this file and found that the second method parameter are using for file path construction. But $template->name might be a language constant and there will be an error.

[brianteeman]

This looks like it might be correct but you must provide a way for someone to test this.

ie If you do this then it fails but if you apply this change then it no longer fails

[sergeytolkachyov]

Testing instructions

1. Create a dummy template with 2 files: templateDetails.xml and index.php. You can copy a Cassiopeia files and then remove all unnecessary.
2. Set the <name> on something similar to language constant: TPL_TEST_NAME.
3. Install your awesome template.
4. We see, that the template folder is tpl_test_name
   
5. We see that the template's element attribute is the same as the name, but in different cases.
   
6. In the getTemplate() method, a query to the database, from where the element and name are taken
   
   But both are in different cases.
7. In TemplateHelper::parseXMLTemplateFile() method we have a wrong uppercased path to templateDetails.xml. This is on template details page.
   
8. Template is inheritable
   
   But we cannot find a "Create child template" button
   
9. Apply changes from this PR and change a name to element - the "Create child template" is exists now
   

And now we can a create a child template.

'The template folder is not writable. Some features may not work' appears because I didn't add the media folder for the template

[laoneo]

Ping @dgrammatiko

[dgrammatiko]

I just review this file and found that the second method parameter are using for file path construction. But $template->name might be a language constant and there will be an error.

The templateDetails.xml is special in this regard. The name is the same as the element and the translated string is constructed out of it. I cannot point you to some code where this is obvious because I'm on my phone. Also changing it to element is fine so 🤷‍♂️

[sergeytolkachyov]

The templateDetails.xml is special in this regard. The name is the same as the element and the translated string is constructed out of it. I cannot point you to some code where this is obvious because I'm on my phone. Also changing it to element is fine so 🤷‍♂️

Yes, I have already discovered this in the process of researching the code for writing testing instructions. Also, in the process of searching, I found out that this affects the functionality of the create child template button

[Septdir]

A simple example. YOOtheme Template (YOOtheme Pro). Template itself is already designed for child themes and not using them will not cause problem.

However, the developer made 2 mistakes in the xml file that prevent the creation of a child template, even if you add inheritable

1. <name>YOOtheme</name> Due to the fact that name is used and not element, the helper cannot find and parse the manifest. - (This was the reason for this PR)
2. The developer added <element>yootheme</element> to the manifest, which causes the parent template to be replaced when creating a child theme. But this is a topic for a separate PR

I don't have time for PR to the core right now. So I asked Sergey to make it. But I didn't give him all the information, for which I apologize.

[dgrammatiko]

Ok, couple of notes here why the $template->element wasn't used in these cases in the first place:

• my mistake!
• the manifests (for most of the extensions) are missing the <element> tag. @Hackwar is trying to bring some consistency/common sense to the platform with: [6.1] Unifying InstallAdapter::getElement() #40608 but it's not yet merged!
• the existing conversion, afaik, was that the name WAS EXPECTED to be the template folder: https://github.com/joomla/joomla-cms/blob/fafb2756f7c755b6e352dfda518d57901b661a08/administrator/templates/atum/templateDetails.xml#L3C2-L3C19,

  joomla-cms/templates/cassiopeia/templateDetails.xml

  Line 3 in fafb275

  <name>cassiopeia</name>

  but that assumes that the dev is not using the element tag (common practice)...

Anyways, the PR is fine

[laoneo]

What happens when a template doesn't have the element tag?

[dgrammatiko]

What happens when a template doesn't have the element tag?

The name should be the same as the folder of the template (kinda the default option). You can check all the Joomla default templates 2.5/3.x/4.x

FWIW the PR I mentioned above from Hannes should be done against 4.4, merged and all the docs for the manifests should point out that the element tag is required. Consistency could help iron out weird cryptic things that nobody actually knows in all their details...

[laoneo]

To be on the safe side, I would go with the element tag and when it doesn't exist, do a fallback to the name tag. We can also add a deprecate message when an element tag doesn't exist. How it is now, it looks like a rather unexpected (but valid) code change where we need a transition time.

[dgrammatiko]

@laoneo some time ago I fixed the modules that were on the new format (namespaces) #33182 but when people started massively rewriting the rest to the new format they used a hack instead of the element tag. The hack is not documented so devs should copy code, verbatim, that they don't understand and they can't find any docs for it. Not great.

Back to this PR, The test should be easy create a template with a name=Testable and an element=test (the template should be installed in the test folder) and check that the template manager still works as expected.

[laoneo]

For the test it is ok, but to keep backwards compatibility it looks for me that the code should use the element tag and then fallback to the name tag. To help extension devs to do the transition to the element tag, a deprecate message should be thrown till we do the full move.

[dgrammatiko]

@laoneo I agree. BTW that's what I did with #33182 and that's what Hannes is doing with #40608 so the transition should be very smooth

[sergeytolkachyov]

For the test it is ok, but to keep backwards compatibility it looks for me that the code should use the element tag and then fallback to the name tag. To help extension devs to do the transition to the element tag, a deprecate message should be thrown till we do the full move.

What should be added in this case? As far as I understand, at this point in the execution of the application, we are talking only about specifying the correct path to the file, since a few lines before that there is a request to the database, from where we get both element and name. Talking about implementing the element tag for templates (and other types of extensions), which I completely agree with, makes sense at other times of application execution. And within the framework of this particular PR, it seems to me that it makes sense to give the application the opportunity to simply get the correct path to the file.

[laoneo]

Sorry for the noise, just realized that the record is coming from the database and there is the element attribute always set even when it doesn't exist in the manifest file. Then I'm happy with the changes here. But I would like to get some tests before I merge it.

[dgrammatiko]

I have tested this item ✅ successfully on 316ef2c

@Septdir could you also test this?

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/41313.}

[Septdir]

I have tested this item ✅ successfully on 316ef2c

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/41313.}

[alikon]

RTC

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/41313.}

[laoneo]

Thanks!