JoomGalleryfriends / JG4-dev

Development repository for JoomGallery v4.x
GNU General Public License v3.0
10 stars 6 forks source link

Fix delete empty category #166

Closed MrMusic closed 7 months ago

MrMusic commented 8 months ago

Possible Fix for issue https://github.com/JoomGalleryfriends/JG4-dev/issues/162

Before this fix: Deleting a empty category does not delete the folder in the filesystem.

After this fix: The folder in the filesystem shoud be deleted.

rowi68 commented 8 months ago

I have tested this item ✅ successfully. If deleting empty category, folder in the filesystem is also deleted...

AlexanderSupp commented 8 months ago

See my test results in Joomla 5.0 documented in issue https://github.com/JoomGalleryfriends/JG4-dev/issues/162 When this issue here is solved, is this also in Joomla 5.0 solved?

MrMusic commented 8 months ago

When this issue here is solved, is this also in Joomla 5.0 solved?

Unfortunately, it looks to me as if Joomla 5 is adding further problems. Example: Deleting categories under Joomla 5: In line 1103 of FileManager.php there is the following line: if(\is_object($cat) && $cat instanceof \Joomla\CMS\Object\CMSObject && isset($cat->path)). The problem here is $cat instanceof \Joomla\CMS\Object\CMSObject. In Joomla 4.4 this was still valid but in Joomla 5.0 it is not anymore. I don't know why this is so and how to fix the problem. Simply removing $cat instanceof \Joomla\CMS\Object\CMSObject at this point would only be a temporary workaround. This code is used in many other places. We'll have to wait what @Elfangor93 means about.

AlexanderSupp commented 8 months ago

We'll have to wait what @Elfangor93 means about.

Thank you. Hopefully Joomla 5 won't be a major construction site.

Elfangor93 commented 8 months ago

Yes, I know. CMSObject is removed with Joomla 5. I can explain you that on the next meeting...

AlexanderSupp commented 8 months ago

Testing with Joomla 4.3.4, PHP 8.1.13, mysql 8.0.31. I can confirm the result from @rowi68. PR works successfully and is ready to merge into main.

Elfangor93 commented 8 months ago

If I am informed correctly CMSObject is replaced with Registry. Maybe you could exchange the is instance of...

MrMusic commented 8 months ago

If I am informed correctly CMSObject is replaced with Registry. Maybe you could exchange the is instance of...

Simply replacing instanceof \Joomla\CMS\Object\CMSObject with instanceof \Joomla\Registry\Registry unfortunately does not fix the error.

MrMusic commented 8 months ago

Changes:

The following should now work in J4 and J5:

AlexanderSupp commented 8 months ago

Uninstalled JG4 and installed this PR into Joomla 5.0 Screenshot 2023-11-14 183938

I built up all data from scratch. I try to reproduce the issue #162 errors are gone. Everything fine. No problem with CMSObject. I try to delete a category with content. Response correct: Category-Folder with name 'wanderboys' could not be deleted because there are still files and subfolders in it. I delete all images and repeat the catalogue delete. Everything fine.

It seems that this PR is ready to merge into main. However, I continue my tests with this 5.0 installation.

rowi68 commented 8 months ago

I can confirm result from @AlexanderSupp. Successfully tested with J4 and J5 :+1:

Elfangor93 commented 7 months ago

'instanceof' removed

I am not satisfied with this quick fix. It does not solve the problem but suppresses the negative effects. I will start working on a PR removing CMSObject from our extension anyway soon. With this the error will be properly fixed.

AlexanderSupp commented 7 months ago

Is it possible to merge this PR now into main? And then close this PR, and we wait of a new PR. There are too many open PR, with label "needs testing".