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

[5.3] ExtensionInstallCommand more consistent with administrator #43700

Open
wants to merge 5 commits into
base: 5.3-dev
Choose a base branch
from

Conversation

brbrbr
Copy link

@brbrbr brbrbr commented Jun 25, 2024

Maybe mark this with RFC?

Summary of Issues

Deletion of package

with --path processPathInstallation would delete the package file if it's placed in the tmp_path.
There are two flaws with this:

  1. If the file is in tmp_path it is deleted, otherwise it's not. This is inconsistent.
  2. Extension:install --path` never downloads the package itself, so it should not delete with is not its.

So --path should only clean up the extracted files. (--path does not accept a URL as parameter, it never downloads a file)

Inconsistent names with the web interface

In the web interface we have 'URL', 'folder' and 'package'  (and web, that one is not applicable for the CLI)
'URL' can be found in extension:install, no problems there.

--path is slightly ambiguous, at a first glance you might expect this to point to a folder. Only after careful reading of the help information, it is clear that it must be a package. 

An option to install from a folder is missing at all

To be more consistent, extension:install should support 'folder' and 'package' as well

Summary of Changes

 - adding an option --package to point to a zip file (processPackageInstallation)
 - Adding an option --folder to point to a folder with extracted files (processFolderInstallation)
 - Modifying --path to accept a folder or a file (B/C). Delegating the work to processPackageInstallation or processFolderInstallation
 - Package installation only deletes the extracted files. Not the original package.
 - Minor changes to the help info, extension:install can update as well.- Added short versions of the parameters

Testing Instructions

--url is omitted.
Ensure the CLI command is executed with the right permissions. Installation fails otherwise without a clear messsage.

Actual result BEFORE applying this Pull Request

 - php joomla extension:install --path <zipfile> - works
 - php joomla extension:install --path <tmp_dir/zipfile> - works - file deleted
 - php joomla extension:install --path <folder> - fails

Expected result AFTER applying this Pull Request

 - php joomla extension:install --path <zipfile> - works (B/C)
 - php joomla extension:install --path <folder> - works
 - php joomla extension:install --path <tmp_dir/zipfile> - works - zipfile not deleted 
 
 - php joomla extension:install --package | -p <folder> - works
 - php joomla extension:install --folder | -f <folder> - works
 
 - php joomla extension:install --package | -p <folder> - fails
 - php joomla extension:install --folder | -f <zipfile> - fails

…nstallation)

 - Adding an option --folder to point to a folder with extracted files (processFolderInstallation)
 - Modifying --path to accept a folder or a file (B/C). Delegating the work to  processPackageInstallation or processFolderInstallation
 - Package installation only deletes the extracted files. Not the original package.
 - Changes to the help info, `extension:install` can update as well.- Added short versions of the parameters
@degobbis
Copy link
Contributor

Hmm, changing the expected behaviour would be a B/C break, wouldn't it?

So the change from
php joomla extension:install --path <tmp_dir/zipfile> - works - file deleted to
php joomla extension:install --path <tmp_dir/zipfile> - works - zipfile not deleted is, in my opinion, a B/C break.

Basically, I think the default behaviour should always be to clean up after yourself. If this doesn't happen, I think it's more of a bug.

If something is not to be cleaned up after processing, an option such as “--doNotClean” would be the better approach, wouldn't it?

@brbrbr
Copy link
Author

brbrbr commented Aug 24, 2024

Basically, I think the default behaviour should always be to clean up after yourself. If this doesn't happen, I think it's more of a bug.

Since the --path option does not download the file, the command should not be responsible for cleanup as yourself.

The webclients administrator 'folder' install does not delete the used folder, neither.

This is different with the (proposed) --url option which downloads the file, therefor is responsible for cleanup as yourself and should clean up.

Besides the deletion of the file depends on the location of the file.

@HLeithner HLeithner changed the base branch from 5.2-dev to 5.3-dev September 2, 2024 08:50
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 5.3-dev.

@HLeithner HLeithner changed the title ExtensionInstallCommand more consistent with administrator [5.3] ExtensionInstallCommand more consistent with administrator Sep 2, 2024
@Hackwar Hackwar removed the PR-5.2-dev label Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants