refactor: replace Doctrine ORM and ExtJS 2.2 with Cacti-native patterns - #21
Open
somethingwithproof wants to merge 5 commits into
Open
somethingwithproof wants to merge 5 commits into
somethingwithproof wants to merge 5 commits into
Conversation
Remove vendored ExtJS 2.2 framework (2.5MB) and all ExtJS app code. Replace with Cacti-idiomatic jQuery using standard page structure, html_start_box/html_end_box table patterns, and jQuery UI dialogs. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
13 test suites covering: prepared statement consistency, PHP 7.4 compatibility, auth guards, output escaping, SQL injection prevention, redirect safety, setup structure, Doctrine removal verification, ExtJS removal verification, and controller unit tests. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR modernizes the NPC plugin by removing legacy vendored dependencies (Doctrine 1.x ORM and ExtJS 2.2 assets) and migrating server-side data access to Cacti-native db_fetch_*_prepared() calls, while updating the UI entrypoint to a Cacti-native page/tab structure.
Changes:
- Removed ExtJS 2.2 resources (CSS/themes/adapter) and legacy build artifacts (Ant build).
- Replaced multiple Doctrine query paths with parameterized Cacti DB helpers and updated controllers accordingly.
- Added Composer dev tooling configuration for Pest test execution.
Reviewed changes
Copilot reviewed 56 out of 1290 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| js/ext/resources/css/xtheme-olive.css | Removed vendored ExtJS theme asset. |
| js/ext/resources/css/xtheme-gray.css | Removed vendored ExtJS theme asset. |
| js/ext/resources/css/xtheme-darkgray.css | Removed vendored ExtJS theme asset. |
| js/ext/resources/css/window.css | Removed vendored ExtJS CSS. |
| js/ext/resources/css/tree.css | Removed vendored ExtJS CSS. |
| js/ext/resources/css/toolbar.css | Removed vendored ExtJS CSS. |
| js/ext/resources/css/tabs.css | Removed vendored ExtJS CSS. |
| js/ext/resources/css/slider.css | Removed vendored ExtJS CSS. |
| js/ext/resources/css/resizable.css | Removed vendored ExtJS CSS. |
| js/ext/resources/css/reset.css | Removed vendored ExtJS CSS reset. |
| js/ext/resources/css/reset-min.css | Removed vendored ExtJS CSS reset (min). |
| js/ext/resources/css/qtips.css | Removed vendored ExtJS CSS. |
| js/ext/resources/css/progress.css | Removed vendored ExtJS CSS. |
| js/ext/resources/css/panel.css | Removed vendored ExtJS CSS. |
| js/ext/resources/css/menu.css | Removed vendored ExtJS CSS. |
| js/ext/resources/css/layout.css | Removed vendored ExtJS CSS. |
| js/ext/resources/css/grid.css | Removed vendored ExtJS CSS. |
| js/ext/resources/css/form.css | Removed vendored ExtJS CSS. |
| js/ext/resources/css/editor.css | Removed vendored ExtJS CSS. |
| js/ext/resources/css/dialog.css | Removed vendored ExtJS CSS. |
| js/ext/resources/css/debug.css | Removed vendored ExtJS debug CSS. |
| js/ext/resources/css/dd.css | Removed vendored ExtJS drag/drop CSS. |
| js/ext/resources/css/date-picker.css | Removed vendored ExtJS date picker CSS. |
| js/ext/resources/css/core.css | Removed vendored ExtJS core CSS. |
| js/ext/resources/css/combo.css | Removed vendored ExtJS combo CSS. |
| js/ext/resources/css/button.css | Removed vendored ExtJS button CSS. |
| js/ext/resources/css/box.css | Removed vendored ExtJS box CSS. |
| js/ext/resources/css/borders.css | Removed vendored ExtJS borders CSS. |
| js/ext/resources/css/README.txt | Removed ExtJS CSS packaging note. |
| js/ext/license.txt | Removed ExtJS license file (dependency removed). |
| js/ext/adapter/jquery/ext-jquery-adapter.js | Removed ExtJS jQuery adapter (dependency removed). |
| css/main.css | Removed ExtJS-era plugin CSS (portlets/icons/status). |
| css/ext-ux-livegrid.css | Removed ExtJS UX LiveGrid CSS. |
| controllers/statehistory.php | Migrated state history paging/querying from Doctrine to prepared DB calls. |
| controllers/settings.php | Migrated settings storage retrieval/update off Doctrine to prepared DB calls. |
| controllers/servicegroups.php | Refactored servicegroup aggregation to DB helpers and simplified JSON response building. |
| controllers/notifications.php | Migrated notifications paging/querying from Doctrine to prepared DB calls. |
| controllers/nagios.php | Migrated several Doctrine queries to prepared DB calls; adjusted process info formatting output. |
| controllers/logentries.php | Migrated log entries paging/querying to prepared DB calls; updated search binding behavior. |
| controllers/layoutDev.php | Removed ExtJS-based dev layout controller. |
| controllers/layout.php | Replaced ExtJS entry layout with Cacti-native header/footer and tab navigation + JS bootstrap. |
| controllers/hosts.php | Migrated host listing/summary/mapping logic to prepared DB calls, added sort allowlist. |
| controllers/hostgroups.php | Migrated hostgroup queries to prepared DB calls and simplified JSON responses. |
| controllers/downtime.php | Migrated scheduled downtime and downtime history queries to prepared DB calls. |
| controllers/controller.php | Updated shared searchClause() to generate parameterized LIKE conditions and bind params. |
| controllers/comments.php | Migrated comment queries to prepared DB calls and updated icon lookup. |
| config.php | Removed Doctrine bootstrap and kept Controller bootstrap only. |
| composer.json | Added Composer config for Pest dev dependency + test bootstrap autoload. |
| build.xml | Removed legacy Ant/YUICompressor build script. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…t SQL injection The $this->sort and $this->dir values flow from user request into an ORDER BY clause without any allowlist. Restrict $sort to known columns and $dir to ASC/DESC. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
somethingwithproof
marked this pull request as draft
April 11, 2026 00:10
Member
Author
|
Converted to draft to serialize the stack in this repo. Blocked by #18; will un-draft after that merges to avoid cross-PR merge conflicts. |
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
xmacan
approved these changes
Sep 15, 2026
TheWitness
requested changes
Sep 17, 2026
TheWitness
left a comment
TheWitness
left a comment
Member
There was a problem hiding this comment.
This was an interesting concept. Would have to see how it looks like live before merging.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
composer install && vendor/bin/pestpasses all 13 test suites