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.

Sign up for GitHub

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

66 add automatic plugin type detection in cli plugin installation #67

Merged
merged 5 commits into from
Oct 20, 2024
Merged

66 add automatic plugin type detection in cli plugin installation #67

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8c9abba
feat: Add support for installing plugins from URL with type parameter
erseco Oct 19, 2024
ac0fa2e
fix: Correct closing bracket for if statement in detect_and_move_plug…
erseco Oct 19, 2024
314d79d
feat: Add --debug option to enable detailed error messages in plugin …
erseco Oct 19, 2024
047c51a
feat: Require URL for plugin installation and remove plugin name option
erseco Oct 19, 2024
1b4fb86
Fixed pipeline and improved plugin install script
erseco Oct 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .github/workflows/check-moodle-version.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ jobs:
- name: Get Current Version from Git Tags
id: get_current_version
run: |
CURRENT_VERSION=$(git tag | sort -V | tail -n 1)
# Get the latest stable tag (no pre-release)
CURRENT_VERSION=$(git tag | grep -vE '(-rc|-beta|-alpha)' | sort -V | tail -n 1)
echo "CURRENT_VERSION=$CURRENT_VERSION" >> $GITHUB_ENV

- name: Compare Versions
Expand Down
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@
/Persistent
.data
.DS_Store
.aider*
78 changes: 37 additions & 41 deletions rootfs/var/www/html/admin/cli/install_plugin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,11 @@

Options:
-h --help Print this help.
--plugin=<pluginname> The name of the plugin to install.
--url=<pluginurl> The URL to download the plugin from.
--type=<plugintype> The type of the plugin (e.g., mod, block, filter, theme, local). Defaults to mod.
--run Execute install. If this option is not set, the script will run in dry mode.
--force Force install even if plugin exists (useful for development).
--debug Enable debug mode to see detailed error messages.
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Issue: The --force option is specified but not implemented.

The --force option is listed in the help text on line 17, but the script does not utilize $options['force'] anywhere in the code. As a result, this option has no effect on the plugin installation process.

Solution:

Implement logic to handle the --force option when checking for existing plugins. This would allow users to overwrite existing plugin directories if they specify the --force flag.

--showsql Show SQL queries before they are executed.
--showdebugging Show developer level debugging information.

Expand All @@ -32,9 +33,11 @@
'help' => false,
'plugin' => false,
'url' => false,
'type' => 'mod', // Default plugin type
'run' => false,
'force' => false,
'showsql' => false,
'debug' => false,
'showdebugging' => false,
], [
'h' => 'help'
Expand All @@ -45,6 +48,11 @@
cli_error(get_string('cliunknowoption', 'core_admin', $unrecognised));
}

if ($options['debug']) {
set_debugging(DEBUG_DEVELOPER, true);
cli_writeln('Debug mode enabled.');
}

if ($options['help']) {
cli_writeln($help);
exit(0);
Expand All @@ -58,8 +66,8 @@
$DB->set_debug(true);
}

if (!$options['plugin'] && !$options['url']) {
cli_writeln('You must specify either a plugin name or a URL.');
if (!$options['url']) {
cli_writeln('You must specify a URL to download the plugin.');
Comment on lines +69 to +70
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Issue: --plugin option appears unsupported but is still referenced.

The script now mandates the --url option and exits if it's not provided. However, the help text and examples still reference a --plugin option, which seems unsupported in the current code.

Solution:

  • Option 1: Re-implement support for the --plugin option if it's intended to be used.
  • Option 2: Update the help text and examples to remove references to the --plugin option, ensuring consistency.

cli_writeln($help);
exit(0);
}
Expand Down Expand Up @@ -96,31 +104,20 @@
cli_error('Failed to detect plugin name.');
}

$plugins[] = (object)[
'component' => $pluginname,
'zipfilepath' => $tempfile,
];
} else {
$pluginDir = $CFG->dirroot . '/mod/' . $options['plugin'];

$plugininfo = get_plugin_info_from_version_file($pluginDir);
if (!$plugininfo) {
cli_error('Invalid plugin directory: ' . $pluginDir);
$destination = detect_and_move_plugin($pluginname, "$tempdir/$rootdir", $options['type']);
if (!$destination) {
cli_error('Failed to move plugin to the correct directory.');
Comment on lines +107 to +109
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Issue: Duplicate or conflicting calls to detect_and_move_plugin.

There are two calls to detect_and_move_plugin:

  • Lines 107-109: Uses $tempdir/$rootdir as the source path.
  • Lines 112-114: Uses $pluginDir as the source path, but $pluginDir is not defined.

Solution:

  • Confirm the correct source path for moving the plugin.
  • Remove the duplicate or incorrect call.
  • Define $pluginDir if it's necessary, or correct it to use the appropriate variable.

}

$pluginname = $plugininfo->component;
cli_writeln("Preparing to install plugin: $pluginname");

// Check if the plugin is already installed, unless forced
if ($pluginman->get_plugin_info($pluginname) && !$options['force']) {
cli_error("Plugin $pluginname is already installed. Use --force to reinstall.");
$destination = detect_and_move_plugin($pluginname, $pluginDir, $options['type']);
if (!$destination) {
cli_error('Failed to move plugin to the correct directory.');
Comment on lines +112 to +114
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Issue: Undefined variable $pluginDir used in function call.

The variable $pluginDir is not defined before it's used in detect_and_move_plugin($pluginname, $pluginDir, $options['type']); on line 112. This will lead to an undefined variable error.

Solution:

Replace $pluginDir with the correct variable. If you intended to use $tempdir/$rootdir, ensure consistency:

- $destination = detect_and_move_plugin($pluginname, $pluginDir, $options['type']);
+ $destination = detect_and_move_plugin($pluginname, "$tempdir/$rootdir", $options['type']);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
$destination = detect_and_move_plugin($pluginname, $pluginDir, $options['type']);
if (!$destination) {
cli_error('Failed to move plugin to the correct directory.');
$destination = detect_and_move_plugin($pluginname, "$tempdir/$rootdir", $options['type']);
if (!$destination) {
cli_error('Failed to move plugin to the correct directory.');

}

$plugins[] = (object)[
'component' => $pluginname,
'zipfilepath' => null,
'zipfilepath' => $tempfile,
];
}

if ($options['run']) {
cli_writeln('Installing plugin...');
Expand Down Expand Up @@ -226,9 +223,10 @@ function detect_plugin_name($dir) {
*
* @param string $pluginname Name of the plugin.
* @param string $sourcepath Path to the extracted plugin.
* @param string $type The type of the plugin.
* @return bool|string Returns the new path of the plugin if successful, or false on failure.
*/
function detect_and_move_plugin($pluginname, $sourcepath) {
function detect_and_move_plugin($pluginname, $sourcepath, $type) {
global $CFG;

// Define the base paths for different plugin types
Expand All @@ -240,28 +238,26 @@ function detect_and_move_plugin($pluginname, $sourcepath) {
// Add other plugin types as needed
];

// Attempt to detect the plugin type based on the name or folder structure
foreach ($plugin_types as $type => $path) {
if (strpos($pluginname, $type . '_') === 0) {
$destination = $path . basename($sourcepath);

// Ensure the directory is writable before moving the plugin
if (!is_writable($path)) {
cli_error("Directory $path is not writable. Check permissions.");
}

// Move the plugin to the correct directory
if (rename($sourcepath, $destination)) {
cli_writeln("Plugin moved to $destination.");
return $destination;
} else {
return false;
}
// Use the provided type to determine the destination path
if (array_key_exists($type, $plugin_types)) {
$destination = $plugin_types[$type] . basename($sourcepath);

// Ensure the directory is writable before moving the plugin
if (!is_writable($destination)) {
cli_error("Directory $destination is not writable. Check permissions.");
}
Comment on lines +245 to +248
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Issue: Incorrect check for directory writability on potentially non-existent path.

The check is_writable($destination) may fail if $destination does not exist yet. Since you intend to move the plugin to $destination, you should check if the parent directory is writable.

Solution:

Modify the check to verify the parent directory's writability:

- if (!is_writable($destination)) {
+ if (!is_writable(dirname($destination))) {

This ensures that the script correctly checks the ability to create the destination directory.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Ensure the directory is writable before moving the plugin
if (!is_writable($destination)) {
cli_error("Directory $destination is not writable. Check permissions.");
}
// Ensure the directory is writable before moving the plugin
if (!is_writable(dirname($destination))) {
cli_error("Directory $destination is not writable. Check permissions.");
}


// Move the plugin to the correct directory
if (rename($sourcepath, $destination)) {
cli_writeln("Plugin moved to $destination.");
return $destination;
} else {
return false;
Comment on lines +250 to +255
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Issue: Potential overwriting of existing plugins without handling --force option.

The rename($sourcepath, $destination) function may overwrite an existing plugin directory without checking if it already exists or considering the --force option.

Solution:

Before moving the plugin, check if the destination directory exists and handle it accordingly:

if (file_exists($destination)) {
    if ($options['force']) {
        // Optionally, remove the existing directory.
        // Be cautious with recursive deletions.
        remove_dir($destination); // Implement this function or use an existing one.
    } else {
        cli_error("Plugin already exists at $destination. Use --force to overwrite.");
    }
}

Ensure that you safely handle directory deletions and inform the user appropriately.

}
}

// If the plugin type cannot be detected
cli_error("Unknown plugin type for $pluginname. Plugin was not moved.");
// If the plugin type is not recognized
cli_error("Unknown plugin type: $type. Plugin was not moved.");
return false;
}

Loading