Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow folders in packages to be uninstalled like zips #8089

Merged
merged 2 commits into from Oct 25, 2015

Conversation

Bakual
Copy link
Contributor

@Bakual Bakual commented Oct 14, 2015

Issue

The package installer allows to include the contained packages either as zip files or as plain folder.
All works fine when the extensions are contained as zips, but when you have them included as unpacked folders it installs fine, but doesn't get uninstalled together with the package.

For regular extensions, that isn't as much as an issue as you can still uninstall the extensions manually. However in the case of language packs, we have a special check there which enforces that language packs can only be uninstalled as package (don't ask me why). And thus when you uninstall a language package which contains admin and site as folders you will not be able to uninstall them.

So the following works both with install and uninstall:

  <files>
    <file type="language" client="site" id="id-ID">site_id-ID.zip</file>
    <file type="language" client="administrator" id="id-ID">admin_id-ID.zip</file>
  </files>

while this only works for install:

  <files>
    <folder type="language" client="site" id="id-ID">site_id-ID</folder>
    <folder type="language" client="administrator" id="id-ID">admin_id-ID</folder>
  </files>

Solution

This PR just adds folder tags to the "filelist" array in the package manifest builder. From there everything works again.

Testing

We found this issue while testing out Crowdin for creating the core language packages. Thus you can try one of the completed packages from there. Eg:
Indonesian: https://crowdin.com/download/project/joomla-cms/id.zip
Bulgarian: https://crowdin.com/download/project/joomla-cms/bg.zip

They should install fine, but when trying to uninstall the package, the actual language packs for site and admins aren't uninstalled.
After the PR is applied, they will be uninstalled together with the package as expected.

Obviously also try with a regular package that this still installs and uninstalls fine. You can use any of the official language packages (using the language manager) or a 3rd party extension package to test that.

foreach ($xml->files->folder as $folder)
{
// NOTE: JInstallerExtension doesn't expect a string.
// DO NOT CAST $file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you what to write DO NOT CAST $folder ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't write that, I just copy-pasted :)

Good catch, I will adjust that ;-)

@infograf768
Copy link
Member

Some comments (unrelated to the possibility or not to include folders instead of .zip for admin and site):

The names of the .zips proposed above (id.zip, bg.zip) do not follow the format used to pick them in the http://update.joomla.org/language/translationlist_3.xml for Extensions=>Updates proposed as well as install from Extensions=>Manage=>Install Languages.
(I wonder what would happen if en-US and en-GB both get a en.zip)

The package name should be of the type:
xx-XX_joomla_lang_full_3.4.4v2.zip
The pkg_xx-XX.xml above have a wrong version as well as other xmls:
we use for example: 3.4.4.1, 3.4.4.2 etc. for users to know which version is installed on their site.
It is specially important in the pkg xml for updates to function properly.

Concerning:

we have a special check there which enforces that language packs can only be uninstalled as package (don't ask me why).>

The manifest is a package and as such, has to be uninstalled as a package. If a part of the package (site or admin) is uninstalled we get errors.
Also as many plugins (admin) need their ini files in frontend, we anyway should never install a pack without at least some admin inis, see:
https://docs.joomla.org/J3.x:Making_a_Language_Pack_for_Joomla#The_Site-only_pack

A third party extension lang zip is not usually a package type manifest but a file type manifest. See
https://docs.joomla.org/J2.5:Making_non-core_language_packs
https://docs.joomla.org/Creating_language_packs_for_extensions_in_Joomla_2.5

Hope it helps

@Bakual
Copy link
Contributor Author

Bakual commented Oct 14, 2015

It's out of the scope of this PR as this is only about a bug in the uninstaller. The language packs generated by Crowdin are only how I detected it and the simplest way to test it. I could instead have written a custom package for a regular extension instead, but why do when I already have an example at hand 😄

The names of the .zips proposed above (id.zip, bg.zip) do not follow the format used to pick them in the http://update.joomla.org/language/translationlist_3.xml for Extensions=>Updates proposed as well as install from Extensions=>Manage=>Install Languages.

They are not meant to be picked up by that. The download location for the files currently still is JC, and it will be an own server in the future (the same as for Joomla packages).

(I wonder what would happen if en-US and en-GB both get a en.zip)
You can have a look at https://crowdin.com/page/api/language-codes and see that they have en-US and en-GB there. That's what the names will be for the generated zips in those cases.

The package name should be of the type:
xx-XX_joomla_lang_full_3.4.4v2.zip
The pkg_xx-XX.xml above have a wrong version as well as other xmls:
we use for example: 3.4.4.1, 3.4.4.2 etc. for users to know which version is installed on their site.
It is specially important in the pkg xml for updates to function properly.

It's not about if the language packs are named correctly or if the XMLs are written properly. That's a different topic and something the translators have (and will) fix before releasing any pack.
These linked packs currently are only meant for testing purposes, not for a productive use.

@infograf768
Copy link
Member

I have tested this item ✅ successfully on 2c7ce58

This is what I said when I wrote:

unrelated to the possibility or not to include folders instead of .zip for admin and site

Anyway, this patch works, for the packs you posted as well as former format.


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

@matrikular
Copy link
Contributor

I have tested this item 🔴 unsuccessfully on 2c7ce58

Before applying the patch I get the following error messages when I try to install the provied bg.zip:

Local environment:
Fatal error: Maximum function nesting level of '256' reached, aborting! in C:\xampp\htdocs\patchtester\libraries\joomla\table\table.php on line 136

http://de.joomlapbf.com Installation:
Fatal error: Allowed memory size of 268435456 bytes exhausted (tried to allocate 32 bytes) in /home/eabbcbdv/public_html/de/joomla1/libraries/joomla/database/driver/mysqli.php on line 567

Indonesian package:
Local environment:
Error
There was an error uploading this file to the server.
Unable to find install package

http://de.joomlapbf.com Installation:
Fatal error: Allowed memory size of 268435456 bytes exhausted (tried to allocate 64 bytes) in /home/eabbcbdv/public_html/de/joomla1/libraries/joomla/database/driver/mysqli.php on line 567


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

@Bakual
Copy link
Contributor Author

Bakual commented Oct 24, 2015

@matrikular I see where that comes from. I have recently updated the source package file on Crowdin and the packages aren't upated yet to reflect that change. Thus the generated zips are broken as of now.
I will try to see if I can get a working zip uploaded somewhere else later this day for the tests.

@coolcat-creations
Copy link
Contributor

...following for testing with another zip


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

@Bakual
Copy link
Contributor Author

Bakual commented Oct 24, 2015

@designbengel @matrikular You can try with this zip: www.bakual.ch/images/pr8089/bg.zip

@matrikular
Copy link
Contributor

I have tested this item ✅ successfully on 2c7ce58

Tested on localhost.


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

@zero-24
Copy link
Contributor

zero-24 commented Oct 24, 2015

RTC


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

@zero-24 zero-24 added this to the Joomla! 3.4.6 milestone Oct 24, 2015
@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Oct 24, 2015
rdeutz added a commit that referenced this pull request Oct 25, 2015
Allow folders in packages to be uninstalled like zips
@rdeutz rdeutz merged commit 17b9767 into joomla:staging Oct 25, 2015
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Oct 25, 2015
@Bakual Bakual deleted the FixPackageUninstall branch October 25, 2015 17:41
@zero-24 zero-24 modified the milestones: Joomla! 3.4.6, Joomla! 3.5.0 Oct 28, 2015
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.

None yet

7 participants