Skip to content

Comments

Update TemplateModel.php#40712

Closed
BrainforgeUK wants to merge 2 commits intojoomla:4.3-devfrom
BrainforgeUK:patch-13
Closed

Update TemplateModel.php#40712
BrainforgeUK wants to merge 2 commits intojoomla:4.3-devfrom
BrainforgeUK:patch-13

Conversation

@BrainforgeUK
Copy link
Contributor

Allows admin to add files to an empty template media folder.

Pull Request for Issue #40666 .

Summary of Changes

Allow admin to add files / folders to empty template media folder.
The admin might have deleted (accidently or intentionally) all template media content.
See admin Templates: Customise page.

Similarly admin can add media to a template which did not have any in the first place.
Should installation of templates without media be disallowed? Not addressed by this PR.

Testing Instructions

Install a template and use the admin Templates: Customise page to delete all the files / folders for that template.
Check that on the adminTemplates: Customise page the media folder is still accessible and more media files / folders can be added.

Similarly add a template without a media section and check that media files / folders can be added by the admin.
tpl_cassiopeia_test1.zip

Actual result BEFORE applying this Pull Request

Cannot add media files / folders to a template when the template media folder is empty.
When the template media folder is empty there is a misleading message about the media template folder not being writtable ... it is writtable but empty!

Expected result AFTER applying this Pull Request

Use the admin Templates: Customise page to delete template media files / folders and when all have been deleted be able to add media files / folders. Similarly add such if the template did not contain any media files in the first place.

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

Allows admin to add files to an empty template media folder.
Corrected comment.
@dgrammatiko
Copy link
Contributor

Once again, if you want to handle this do it in the template adapter not in the template manager!

@alikon
Copy link
Contributor

alikon commented Jun 4, 2023

this pr has been already rejected once see #40667 for reference
and i don't think this is the way to go, anyway... i'll close this pr as for my undestanding
but i can be wrong, so...

feel free to describe in more details what this pr is trying to solve
thank you for your time

@alikon alikon closed this Jun 4, 2023
@BrainforgeUK
Copy link
Contributor Author

Please reopen this PR and read the comments in the code and explain what the problem is with it.
This PR addresses issues in the template management not template installation - although it does raise questions about template installation (and maybe other extension type installations) but making template adapter changes will not solve the issue - see (b) below.

(a) As a compromise I could remove the mkdir and instead display a message such as
The media folder is missing please contact template developer.
But why add this limitation - see (b)?

(b) I could add a change to the template adapter so that templates with missing media content are rejected.
But why? Just seems to me to provide the template developer with an unnecessary constraint.
If they want a template without any media then let them develop one!

Which takes us back to (a) above. The developer creates a template without any media why stop the site admin adding some through the template manager? Should they have a create media button in the template manager?

(c) What purpose does the bizarre unwrittable media folder check / message serve? Any number of media files / folders could be write protected - just leave it for a sensible error message when site admin tries to make changes ... I have not checked what happens if the file or folder is not writtable.


Ok I am willing to make that compromise in (a) but really I don't see the point.
The other changes still ensure that if site admin deletes all the template media they can still add some back in.

I suggest that this PR be reopened asis and either further changes made to address the above issues or once it has been pushed onto core a new issue created to investigate this matter further.

@dgrammatiko
Copy link
Contributor

Or you can provide a proper media section in the xml and not have any of these problems…

@BrainforgeUK
Copy link
Contributor Author

Having a proper media section in the XML does not solve anything!

(d) Site admin can still use the template manager to remove all existing media content, they are then unable to add media content to the template.
(e) The bizarre unwrittable media folder is still there - if necessary need proper error handling if admin tries to change unwrittable media files / folders using the template manager. Be better handled as a separate issue / PR.
(f) Changing the template adapter to force media developers to include a media section still leaves items (e) and (f) unresolved.

@dgrammatiko
Copy link
Contributor

dgrammatiko commented Jun 4, 2023

Site admin can still use the template manager to remove all existing media content, they are then unable to add media content to the template.

How can an admin remove the media folder?
Screenshot 2023-06-04 at 21 32 40

@brianteeman
Copy link
Contributor

if you remove the folders inside the media folder then the media folder itself disappears from the template manager

chrome_2023-06-04_20-38-43.mp4

@dgrammatiko
Copy link
Contributor

dgrammatiko commented Jun 4, 2023

if you remove the folders inside the media folder then the media folder itself disappears from the template manager

That's baggages from the past and the reason I added all these empty folders...

Actually that's a bug, @brianteeman please test #40719

@BrainforgeUK
Copy link
Contributor Author

I think my solution improves on what is there already.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants