Merged
Conversation
Introduce an end-to-end verification flow and related artifacts. - Add a GitHub Actions `e2e` job (MariaDB service, PHP 8.1, vendor cache) and make the build-phar job depend on it. - Add e2e harness: scripts/e2e (run/provision/teardown), fixtures for OpenCart 3.0.5.0, and an E2E test + test helpers. - Add src/Support/ExtensionHelper and numerous test updates to support stabilized E2E/behavior tests. - Update composer.json: bump minimum PHP to ^7.4, set platform php to 7.4, add test:e2e script, and run phpunit with --testsuite all for CI/coverage hooks; include test:e2e in ready-for-deploy. - Update docs and contributing notes (README and CONTRIBUTING.md) to reflect PHP 7.4 requirement, E2E instructions, and updated usage/examples. - Tidy .gitignore (stop ignoring composer.lock) and minor formatting fixes. These changes add an automated E2E verification path and align CI/test scripts and docs with the new checks.
Introduce a suite of runtime-aware CLI commands and supporting runtime helpers for OpenCart 3.x. Adds database maintenance commands (db:check, db:repair, db:optimize, db:cleanup + AbstractMaintenanceCommand), cache management (cache:clear, cache:rebuild), category commands (category:create, category:list), order commands (order:list, order:view, order:update-status), product/user CRUD commands and related support classes (OpenCartRuntime, OpenCartLanguage, OpenCartModelLoader, ModificationRefresher, ProductPayloadBuilder). Application and base Command were extended to register new commands and provide runtime helpers, and phpstan bootstrap updated to include the runtime stub. New unit/integration/e2e tests were added/updated to cover the new functionality.
Expand documentation and examples to document runtime-backed commands (cache, category, order, product updates/deletes, user management, and DB maintenance like db:check/repair/optimize/cleanup), and clarify OpenCart 3.x installation-root requirements. Add a PR merge-gate job to the CI workflow that enforces required check-suite results. Minor PHP changes: increase phpstan memory limit in composer scripts, improve SQL/string concatenation formatting and multiline readability, small option/confirmation refactors across command classes, and add a phpstan ignore for OpenCart Event in OpenCartRuntime. Update unit and E2E tests to use OpenCartRuntime mocks and adjust SQL helpers for safer formatting.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the project's stability and maintainability by introducing robust end-to-end testing, strengthening CI/CD pipelines, and modernizing PHP compatibility. It also refines user and developer documentation for better clarity and removes obsolete configuration examples. Highlights
Changelog
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR stabilizes OC-CLI around a runtime-backed OpenCart 3.x execution model, adds an E2E harness (fixture + DB orchestration), and expands the CLI command surface (catalog, orders, users, cache, and DB maintenance), while updating docs/requirements and tightening CI gates.
Changes:
- Added an end-to-end test harness (OpenCart 3.0.5.0 fixture provisioning + teardown + CI job) and expanded integration/unit coverage.
- Introduced OpenCart 3.x runtime bootstrapping + new commands (DB maintenance, cache, category/order/user/product ops) and refactored several existing commands to use runtime/table guards.
- Updated requirements/documentation and CI merge gating (PHP 7.4+, required checks, E2E job).
Reviewed changes
Copilot reviewed 73 out of 76 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Unit/StabilizedCommandBehaviorTest.php | Adds unit coverage for stabilized command behaviors using FakeDb/runtime doubles. |
| tests/Unit/RuntimeBackedCommandGuardTest.php | Tests runtime-backed commands reject DB-only flags and fail fast on unsupported versions. |
| tests/Unit/ProductCreateCommandTest.php | Updates expectation to new “requires OpenCart root” guard behavior. |
| tests/Unit/ProductCommandValidationTest.php | Refactors non-public member access via shared helper trait and updates validation cases. |
| tests/Unit/ExtensionListCommandTest.php | Aligns extension:list description expectation with updated behavior. |
| tests/Unit/ExtensionInstallCommandTest.php | Updates extension:install description/arg docs to OCMOD import semantics. |
| tests/Unit/ExtensionEnableCommandTest.php | Updates extension:enable description/arg docs and expectations. |
| tests/Unit/ExtensionDisableCommandTest.php | Updates extension:disable description/arg docs and expectations. |
| tests/Unit/DatabaseMaintenanceCommandsTest.php | Adds unit tests for db:check/repair/optimize/cleanup command behaviors. |
| tests/Unit/ConfigCommandTest.php | Updates admin option description assertion to reflect deprecation messaging. |
| tests/Unit/ConfigCommandDisplayTest.php | Refactors display tests using helper trait and updates display expectations. |
| tests/Unit/CommandUtilityMethodsTest.php | Refactors tests to avoid Reflection and validates updated config parsing behavior. |
| tests/Integration/ExtensionCommandsTest.php | Aligns integration expectations with updated extension command descriptions. |
| tests/Integration/DatabaseMaintenanceCommandDefinitionsTest.php | Adds integration tests for DB maintenance command definitions. |
| tests/Integration/CommandExpansionDefinitionsTest.php | Ensures expanded command set is registered and key descriptions are correct. |
| tests/Integration/BinarySmokeTest.php | Adds a smoke test to ensure bin/oc list --raw emits no warnings. |
| tests/Helpers/InvokesNonPublicMembers.php | New test helper trait for invoking non-public methods and setting private props. |
| tests/Helpers/FakeDbResult.php | New fake DB result type to emulate OpenCart DB result shape. |
| tests/Helpers/FakeDb.php | New fake DB adapter for unit tests (query capture + configurable responses). |
| stubs/opencart-runtime.php | Adds PHPStan runtime stubs for OpenCart global classes used by runtime bootstrapping. |
| src/Support/ProductPayloadBuilder.php | Adds reusable payload builder for product create/update flows. |
| src/Support/OpenCartModelLoader.php | Adds model/language/helper/library loader to bootstrap OpenCart runtime. |
| src/Support/OpenCartLanguage.php | Adds language loader compatible with OpenCart language file conventions. |
| src/Support/ModificationRefresher.php | Implements OCMOD refresh logic + filesystem/log writing for cache rebuild flows. |
| src/Support/ExtensionHelper.php | Adds shared parsing/lookup logic for extension identifiers (type:code vs code). |
| src/Commands/User/ListCommand.php | New user:list runtime-backed command with filters and output formats. |
| src/Commands/User/DeleteCommand.php | New user:delete command with “last admin” guard and --force override. |
| src/Commands/User/CreateCommand.php | New user:create command with validation and uniqueness checks. |
| src/Commands/Product/UpdateCommand.php | New product:update command using ProductPayloadBuilder and runtime model edit. |
| src/Commands/Product/ListCommand.php | Refactors product:list to runtime-backed model access + output format handling. |
| src/Commands/Product/DeleteCommand.php | New product:delete command with confirmation and --force option. |
| src/Commands/Order/ViewCommand.php | New order:view command supporting table/json/yaml output. |
| src/Commands/Order/UpdateStatusCommand.php | New order:update-status command resolving status id by id/name. |
| src/Commands/Order/ListCommand.php | New order:list runtime-backed command with filters, sorting, paging, formats. |
| src/Commands/Extension/ListCommand.php | Updates extension:list description, adds table existence guard + error handling. |
| src/Commands/Extension/InstallCommand.php | Reworks extension:install into OCMOD XML import into modification table. |
| src/Commands/Extension/EnableCommand.php | Reworks enable logic to operate on extension table and support type:code identifiers. |
| src/Commands/Extension/DisableCommand.php | Reworks disable logic to delete from extension table and support type:code identifiers. |
| src/Commands/Database/RepairCommand.php | New db:repair maintenance command built on shared maintenance base. |
| src/Commands/Database/OptimizeCommand.php | New db:optimize maintenance command built on shared maintenance base. |
| src/Commands/Database/CleanupCommand.php | New db:cleanup command to clear transient tables and report results. |
| src/Commands/Database/CheckCommand.php | New db:check command and shared operation runner for table maintenance ops. |
| src/Commands/Database/AbstractMaintenanceCommand.php | Adds shared plumbing for maintenance commands (table resolution + rendering). |
| src/Commands/Core/VersionCommand.php | Uses base Command version detection implementation (dedup). |
| src/Commands/Core/ConfigCommand.php | Deprecates --admin flag behavior and standardizes output title/behavior. |
| src/Commands/Core/CheckRequirementsCommand.php | Adds required/recommended metadata and failure logic respecting required=false checks. |
| src/Commands/Category/ListCommand.php | New category:list runtime-backed command with filtering and formats. |
| src/Commands/Category/CreateCommand.php | New category:create runtime-backed command building OpenCart payloads. |
| src/Commands/Cache/RebuildCommand.php | New cache:rebuild command (modification refresh + optional broader cache clears). |
| src/Commands/Cache/ClearCommand.php | New cache:clear command for data/theme/sass caches. |
| src/Command.php | Adds OpenCart runtime plumbing, version detection, tableExists helper, improved config parsing. |
| src/Application.php | Registers new commands in default set and updates run() signature typing. |
| scripts/e2e/teardown.php | Drops E2E tables by prefix from persisted state. |
| scripts/e2e/run.sh | Orchestrates E2E runs (local DB env or dockerized MariaDB fallback). |
| scripts/e2e/provision.php | Provisions OpenCart fixture into workspace and installs it against configured DB. |
| scripts/e2e/docker-compose.yml | Defines ephemeral MariaDB service for local E2E fallback path. |
| phpunit.xml.dist | Adds e2e testsuite and excludes E2E from “all” suite by default. |
| phpstan.neon.dist | Adds runtime stub bootstrap for PHPStan analysis. |
| fixtures/opencart/opencart-3.0.5.0.json | Adds fixture metadata (source + sha256). |
| docs/installation.md | Rewrites installation guide, clarifies connection modes and limitations. |
| docs/database-schema.md | Clarifies language resolution behavior and updates mapping notes accordingly. |
| config/oc-cli.yml.example | Removes example runtime config since v1 does not support runtime config files. |
| composer.json | Raises minimum PHP to 7.4, adds E2E script, and updates CI/test scripts. |
| README.md | Rewrites README to reflect stabilized command surface and runtime-backed model behavior. |
| CONTRIBUTING.md | Updates PHP requirement and adds E2E run instructions. |
| .gitignore | Stops ignoring composer.lock and normalizes ignored settings.json entry. |
| .github/workflows/ci.yml | Adds E2E job, makes PHAR build depend on it, and introduces PR merge gate job. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+67
to
+88
| private function clearThemeCache(string $cacheDir): int | ||
| { | ||
| $deleted = 0; | ||
| $directories = glob(rtrim($cacheDir, '/\\') . '/template/*', GLOB_ONLYDIR) ?: []; | ||
|
|
||
| foreach ($directories as $directory) { | ||
| foreach (glob($directory . '/*') ?: [] as $file) { | ||
| if (is_file($file)) { | ||
| unlink($file); | ||
| $deleted++; | ||
| } | ||
| } | ||
|
|
||
| if (is_dir($directory)) { | ||
| rmdir($directory); | ||
| } | ||
| } | ||
|
|
||
| return $deleted; | ||
| } | ||
|
|
||
| private function clearSassCache(string $root): int |
There was a problem hiding this comment.
clearThemeCache() / clearSassCache() are duplicated here and in Cache\RebuildCommand, which increases the chance of the two implementations drifting (bug fixes, path changes, etc.). Consider extracting the shared cache-clearing helpers into a shared support class/trait so both commands reuse the same logic.
| { | ||
| try { | ||
| $escapedTable = $db->escape($table); | ||
| $result = $db->query("SHOW TABLES LIKE '{$escapedTable}'"); |
There was a problem hiding this comment.
tableExists() uses SHOW TABLES LIKE ... with the raw (escaped) table name. In MySQL LIKE, _ and % are wildcards, so table names such as oc_extension can yield false positives/negatives when prefixes contain underscores. Consider escaping LIKE wildcards (e.g., converting _ to \_ and % to \% and adding ESCAPE '\\'), or querying information_schema.tables for an exact match.
Suggested change
| $result = $db->query("SHOW TABLES LIKE '{$escapedTable}'"); | |
| $likePattern = strtr($escapedTable, [ | |
| '\\' => '\\\\', | |
| '%' => '\%', | |
| '_' => '\_', | |
| ]); | |
| $result = $db->query("SHOW TABLES LIKE '" . $likePattern . "' ESCAPE '\\\\'"); |
| - stubs/opencart-runtime.php | ||
|
|
||
| # Check PHP compatibility | ||
| phpVersion: 70100 |
There was a problem hiding this comment.
phpVersion is set to 70100 (PHP 7.1), but this PR raises the minimum supported PHP version to 7.4. This mismatch can cause PHPStan to miss newer language features used in the codebase (or report invalid compatibility issues). Update phpVersion to 70400 (or align it with composer.json's platform PHP version).
Suggested change
| phpVersion: 70100 | |
| phpVersion: 70400 |
Comment on lines
+39
to
+41
| if [[ $status -eq 0 ]]; then | ||
| php "$ROOT/scripts/e2e/teardown.php" "$STATE_FILE" >/dev/null 2>&1 || true | ||
| fi |
There was a problem hiding this comment.
The EXIT trap only runs teardown.php when the E2E run succeeds. If tests fail while using a caller-provided DB (or if provision partially succeeds), prefixed tables are left behind, which can pollute subsequent runs and CI retries. Consider always running teardown (and optionally still preserve the build artifacts/state file on failure).
Suggested change
| if [[ $status -eq 0 ]]; then | |
| php "$ROOT/scripts/e2e/teardown.php" "$STATE_FILE" >/dev/null 2>&1 || true | |
| fi | |
| php "$ROOT/scripts/e2e/teardown.php" "$STATE_FILE" >/dev/null 2>&1 || true |
Comment on lines
+73
to
+79
| $rows = $this->fetchBySearch($productModel, $filterData, $search); | ||
| $rows = array_values(array_filter($rows, function (array $row) use ($category): bool { | ||
| return stripos( | ||
| (string) $this->resolveCategoryName((int) $row['product_id']), | ||
| (string) $category | ||
| ) !== false; | ||
| })); |
There was a problem hiding this comment.
When filtering by non-numeric category, resolveCategoryName() is called inside the filter callback and then again when mapping the final rows. Since resolveCategoryName() performs a DB query, this becomes 2 queries per product (plus repeated work). Consider resolving category names once per product_id (memoize) or changing the product query to include category data.
Comment on lines
+130
to
+145
| private function resolveCategoryName(int $productId): string | ||
| { | ||
| $runtime = $this->getAdminRuntime(); | ||
| $db = $runtime->database(); | ||
| $prefix = $runtime->getDatabasePrefix(); | ||
| $languageId = (int) $runtime | ||
| ->registry() | ||
| ->get('config') | ||
| ->get('config_language_id'); | ||
|
|
||
| $query = $db->query( | ||
| "SELECT cd.name FROM `" . $prefix . "product_to_category` ptc " . | ||
| "LEFT JOIN `" . $prefix . "category_description` cd ON (ptc.category_id = cd.category_id) " . | ||
| "WHERE ptc.product_id = '" . $productId . "' AND cd.language_id = '" . $languageId . "' " | ||
| . "ORDER BY ptc.category_id ASC LIMIT 1" | ||
| ); |
There was a problem hiding this comment.
resolveCategoryName() issues a query per product to fetch the category name, which creates an N+1 query pattern for product:list output. With larger limits (or repeated calls as part of filtering), this can become a noticeable bottleneck. Consider fetching categories in bulk (single query for all product_ids) or joining category information in the initial product query/model call.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is a substantial pull request that introduces a full end-to-end testing harness, adds many new commands to expand the CLI's functionality, and refactors existing commands to use a new OpenCart runtime integration layer. The changes significantly improve the project's testability and capabilities. The documentation has also been completely overhauled to reflect the new features, which is a great improvement for users.
My feedback focuses on a few key areas:
- Performance: An N+1 query problem was identified in the product listing command, which could impact performance on large stores.
- Security: A potential SQL injection vulnerability was found in a test helper method due to manual query construction.
- Error Handling: Some of the new scripts could benefit from more robust error handling to avoid silently failing and to make debugging easier.
Overall, this is a very impressive and well-executed set of changes. Addressing the points above will further enhance the quality and robustness of the codebase.
Comment on lines
+73
to
80
| $rows = $this->fetchBySearch($productModel, $filterData, $search); | ||
| $rows = array_values(array_filter($rows, function (array $row) use ($category): bool { | ||
| return stripos( | ||
| (string) $this->resolveCategoryName((int) $row['product_id']), | ||
| (string) $category | ||
| ) !== false; | ||
| })); | ||
| } |
There was a problem hiding this comment.
This filtering logic introduces a performance issue known as the N+1 query problem. The resolveCategoryName method is called inside a loop (array_filter), which executes a separate database query for each product to get its category name. For a large number of products, this will result in many queries and poor performance.
To optimize this, you could consider fetching all necessary category information in a more efficient manner before the loop. For example, you could collect all product IDs, then run a single query to get all their primary category names, and then perform the filtering in-memory.
Comment on lines
+622
to
+625
| foreach ($escapedParameters as $parameter) { | ||
| $quoted = "'" . $parameter . "'"; | ||
| $sql = preg_replace('/\?/', $quoted, $sql, 1); | ||
| } |
There was a problem hiding this comment.
Manually escaping and embedding parameters into an SQL string using preg_replace is not a secure way to construct queries and is vulnerable to SQL injection, especially if character sets are misconfigured. Even in a test environment, it's important to follow best practices. Please use prepared statements with mysqli_prepare, mysqli_stmt_bind_param, and mysqli_stmt_execute to ensure security and correctness.
Comment on lines
+257
to
+265
| if (!@$connection->real_connect( | ||
| $dbConfig['host'], | ||
| $dbConfig['user'], | ||
| $dbConfig['pass'], | ||
| null, | ||
| $dbConfig['port'] | ||
| )) { | ||
| throw new RuntimeException($connection->connect_error ?: 'Unknown server connection error.'); | ||
| } |
There was a problem hiding this comment.
The use of the error suppression operator (@) is generally discouraged as it can make debugging more difficult by hiding potential issues. While the logic here correctly handles the connection failure by throwing an exception, a more modern approach would be to leverage mysqli_report to have mysqli throw exceptions on connection errors. This would simplify the error handling logic and avoid the need for @.
Comment on lines
+22
to
+24
| if (!$connection->real_connect($db['host'], $db['user'], $db['pass'], $db['name'], (int) $db['port'])) { | ||
| exit(0); | ||
| } |
There was a problem hiding this comment.
The database connection failure here is silently ignored. If real_connect returns false, the script will simply exit with a success code, which might hide underlying environment issues during test runs. It would be better to explicitly handle the error, for example by exiting with a non-zero status code and printing the error message to STDERR.
if (!$connection->real_connect($db['host'], $db['user'], $db['pass'], $db['name'], (int) $db['port'])) {
fwrite(STDERR, 'Teardown failed to connect to database: ' . ($connection->connect_error ?: 'Unknown error') . PHP_EOL);
exit(1);
}Introduce an automated GitHub Actions release workflow and tooling for deterministic versioning and PHAR builds. Adds .github/workflows/release.yml, build scripts (scripts/build-phar.sh, scripts/next-release-version.php, scripts/write-build-version.php), and a BuildVersion generator. Replace the hardcoded app version with runtime resolution in src/Application.php (env override, BuildVersion class, or git describe fallback) and add a ReleaseVersion helper at src/Support/ReleaseVersion.php to compute next release numbers. Update composer build command to use the build script and adjust .gitignore. Tests updated/added to assert on the resolved runtime version and to cover ReleaseVersion behavior.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…rvices-Limited/oc-cli into updates-fixes-comments
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.
This pull request introduces a comprehensive end-to-end (E2E) test harness for OC-CLI, updates PHP requirements to 7.4+, and improves CI/CD reliability by enforcing stricter checks for pull requests and releases. It also removes the
.oc-cli.ymlexample configuration file and clarifies documentation around installation, requirements, and language resolution. The most important changes are grouped below.End-to-End Testing Infrastructure
test:e2eComposer script, E2E testsuite inphpunit.xml.dist, Docker Compose setup for MariaDB, and orchestration scripts for provisioning and teardown (scripts/e2e/run.sh,docker-compose.yml,teardown.php). The CI workflow now runs E2E tests and supports both local and ephemeral DB setups. [1] [2] [3] [4] [5] [6]CI/CD Improvements
pr-merge-gatejob to enforce all required checks on pull requests. [1] [2]PHP Requirements and Composer Scripts
composer.json, documentation, and config, and improved Composer scripts for test coverage, code analysis, and deployment. [1] [2] [3]Documentation Updates
Configuration Cleanup
.oc-cli.yml.exampleas runtime configuration is not supported in v1.End-to-End Testing Infrastructure
CI/CD Improvements
pr-merge-gate. [1] [2]PHP Requirements and Composer Scripts
Documentation Updates
Configuration Cleanup
.oc-cli.yml.exampleas runtime config is not supported in v1.