Skip to content

feat(workspace): add logManagerAction for package install/upgrade/remove#16901

Open
Ibochkarev wants to merge 2 commits intomodxcms:3.xfrom
Ibochkarev:feat/16453-log-workspace-actions
Open

feat(workspace): add logManagerAction for package install/upgrade/remove#16901
Ibochkarev wants to merge 2 commits intomodxcms:3.xfrom
Ibochkarev:feat/16453-log-workspace-actions

Conversation

@Ibochkarev
Copy link
Collaborator

What does it do?

Adds logManagerAction() calls to Package Installer processors so that install, upgrade, remove, uninstall, and version remove actions are recorded in the Manager Log.

  • Install: logs package_install or package_upgrade after successful save
  • Remove: logs package_remove before removing the package
  • Uninstall, Update, Version/Remove: use getPrimaryKey() instead of get('id') for correct primary key (signature)
  • Fixes Uninstall: use lexicon() with placeholder array instead of incorrect sprintf()
  • PHPCS: formatting, line length, ELSEIF

Why is it needed?

Manager Log did not record package install/upgrade/remove actions, making it difficult to audit package management operations.

How to test

  1. Install a package via Package Management → log should show package_install
  2. Upgrade an installed package → log should show package_upgrade
  3. Remove a package → log should show package_remove
  4. Uninstall a package → verify no errors
  5. Check Manager Log for the corresponding entries

Related issue(s)/PR(s)

Resolves #16453

- Install: log package_install or package_upgrade after successful save
- Remove: log package_remove before removing package
- Uninstall, Update, Version/Remove: use getPrimaryKey() instead of get('id')
- Fix Uninstall: use lexicon() instead of incorrect sprintf()
- Fix PHPCS: formatting, line length, ELSEIF
@Ibochkarev Ibochkarev force-pushed the feat/16453-log-workspace-actions branch from 730771a to 26cacfa Compare February 25, 2026 16:36
@Ibochkarev Ibochkarev marked this pull request as ready for review February 25, 2026 16:37
@biz87
Copy link

biz87 commented Feb 25, 2026

Code Review

Summary

Adds logManagerAction() calls for package install, upgrade, remove, and uninstall operations. Also fixes a bug where sprintf was used instead of lexicon() with placeholders in Uninstall, and replaces incorrect get('id') with getPrimaryKey() since modTransportPackage uses signature as its primary key. 5 files, +22/-5.

Issues

Install.php — logging is done inline, while Remove, Uninstall, Update, and Version/Remove use a dedicated logManagerAction() method.
For consistency, Install should also use a method:

public function logManagerAction()
{
    $action = $this->package->previousVersionInstalled() ? 'package_upgrade' : 'package_install';
    $this->modx->logManagerAction($action, modTransportPackage::class, $this->package->getPrimaryKey());
}

Remove.phplogManagerAction() is called after removing transport files but before cleanup() (which fires the event and returns success). If zip/directory removal partially fails, the action is still logged. Consider placing the log call right after the successful removePackage() or inside cleanup() before the event — similar to how Install.php places the log after the event. Non-blocking.

Suggestions

  • Uninstall.php:82 — The sprintflexicon() fix is correct. Worth verifying that the package_err_uninstall lexicon entry actually contains the [[+signature]] placeholder.

Assessment

Good targeted fixes. The get('id')getPrimaryKey() change is important since modTransportPackage uses signature as PK, meaning get('id') would return null. The sprintf fix resolves a real bug where the placeholder array was silently ignored. PHPStan clean across all 5 files.

Verdict

Approve — consistency and ordering remarks are optional.

@Ibochkarev Ibochkarev added the feature Request about implementing a brand new function or possibility. label Feb 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature Request about implementing a brand new function or possibility.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Log Workspace Actions

2 participants