You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
feat(moduletools): provide standalone PHP 8.4 library with API 1.1 - #1
Makes xoops/moduletools a self-contained Composer package (PHP 8.4+) that installs, tests and analyses on its own while still running inside xoops_lib/vendor on XOOPS 2.8. Reconciles the 1.4.0 Core tree with the 1.5.0 consolidated cut, fixes a dozen runtime defects found on the way, and adds user documentation.
Motivation
The repository held the 1.4.0 source only: the runtime language catalog, the gate scripts and the API snapshots were missing, the test bootstrap required an autoloader three directories up, CI called a composer analyse script that did not exist, and phpstan.neon was copied from another package. Separately, two 1.5.0 trees existed with different contents (the site path repo vs. the consolidated cut with DirectoryChecker::handleRequest()), which is what broke Quotes on a fresh site.
Approach
Standalone: tests/bootstrap.php and the package scripts resolve the local vendor/autoload.php, falling back to Core's. Gate artifacts moved to resources/gate (export-ignored); the site-specific consumer audit is optional.
Dependencies: php ^8.4; xoops/xmf ^1.3 — every XMF class and method the library calls was checked against 1.3.1. No hardcoded version; sites pin it on their path repository.
Quality gate (composer qa): phpcs PSR-12 → PHPStan level 6 with stubs/xoops-core.stub generated from a real 2.8 core (baseline of 164 typing findings, no logic findings) → Rector PHP 8.4 + xoops/rector-xoops (applied) → API-surface/compat, destination-matrix, layer and legacy-global checks → PHPUnit. The API generator now records promoted constructor properties. CI on PHP 8.4 and 8.5.
Reconciliation with the 1.5.0 cut: DirectoryChecker::handleRequest() + action forms (with a fail-closed CSRF check), FileChecker constant names and catalog fallback, Blocksadmin_MI_* title tokens, the Cloner logo font, the Cloner/DirectoryChecker tests. Admin\Table and Common\Highlighter restored from the path-repo build.
Fixes: Helper::getDirname() (no such method in any XMF) → dirname() at all call sites; DynamicObject::setErrors(); Blocksadmin undefined $newid / double render(); Clonersprintf on placeholder-less constants and silent partial clones; Db::cloneRecord()fetchArray arity; fputcsv escape (8.4 deprecation); VersionChecks by-ref assignment and dead fallbacks; strict-type mismatches in Confirm/Output/ObjectFormBuilder; LetterChoice Smarty 5 setCaching(); UpdateChecker repository parameter + github_repo manifest entry; consumer-constant precedence in ModuleFeedback/ServerStats.
composer qa green on PHP 8.4.25: 61 tests / 269 assertions (15 skip unless the package sits inside a XOOPS tree).
Installed on a XOOPS 2.8.0-Alpha1 site (PHP 8.5) via the path repository; Quotes admin index, quote list, blocks admin, clone, feedback, about, migrate and the front page render with 0 errors / 0 deprecations in the XOOPS logger.
Public API: 4 symbols added, handleRequest() and one optional parameter added, no removals (composer api:compat).
Checklist
PSR-12 passes (composer cs)
PHPStan passes (composer analyse)
PHPUnit passes (composer test)
No BC break: minimum PHP raised to 8.4 (documented in CHANGELOG); public 1.x API retained
Turn the Core-bundled 1.4.0 tree into a self-contained Composer package
that installs, tests and analyses on its own while still running inside
xoops_lib/vendor on a XOOPS 2.8 site.
Package
- Ship resources/ (language catalog, Cloner font); scripts and tests
resolve the package-local vendor/autoload.php, falling back to Core's.
- Require PHP ^8.4; widen xoops/xmf to ^1.3 (every API used exists in
1.3.1). Drop the hardcoded version; sites pin it on the path repo.
- Quality gate: phpcs (PSR-12), PHPStan level 6 with stubs generated from
a real XOOPS 2.8 core, Rector (PHP 8.4 + xoops/rector-xoops) applied,
package-local API/matrix/layer/global checks, PHPUnit. CI on 8.4/8.5.
- Gate artifacts live in resources/gate (export-ignored); site-specific
audits are no longer required.
Reconcile the 1.5.0 consolidated cut
- DirectoryChecker::handleRequest() with create/chmod action forms,
fail-closed CSRF check, catalog fallback; FileChecker constant names;
Blocksadmin _MI_* title-token handling; Cloner/DirectoryChecker tests.
- Restore Admin\Table and Common\Highlighter from the path-repo build.
Fixes
- Xmf\Module\Helper::getDirname() does not exist; use dirname() at every
call site (Blocksadmin, Cloner, Confirm, LetterChoice, Testdata*).
- DynamicObject::setErrors() called a non-existent setError().
- Blocksadmin: undefined $newid on failed store(); render() echoed twice.
- Cloner: sprintf on placeholder-less constants; partial clones now throw.
- Db::cloneRecord() passed a second argument to fetchArray().
- Export: fputcsv escape argument (PHP 8.4 deprecation).
- VersionChecks: by-reference assignment from a call, dead fallbacks.
- Strict-type mismatches in Confirm, Output, ObjectFormBuilder.
- LetterChoice: setCaching(CACHING_OFF) instead of a Smarty property write.
- UpdateChecker accepts a repository and honours the github_repo manifest
entry; ModuleFeedback and ServerStats resolve consumer constants first.
Getting started covers install, Helper/ModuleContext, install hooks,
version guards, admin CRUD, DynamicObject forms, front-end helpers,
permissions, sample data and language-constant resolution. The migration
guide walks a module off copied Common classes or the mTools module in
independently shippable steps. The compatibility policy states what the
1.x line keeps and the conditions for any removal.
This PR contains 158 files, which is 58 over the limit of 100.
To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch.
Upgrade to a paid plan to raise the limit.
This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry.
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Advanced
Run ID: c2061db6-0dc9-405d-ad52-04c82f14909b
📥 Commits
Reviewing files that changed from the base of the PR and between 404a40e and 3792bea.
⛔ Files ignored due to path filters (1)
resources/fonts/VeraBd.ttf is excluded by !**/*.ttf
CodeRabbit skips pull requests above 100 files. Exclude generated gate
artifacts, stubs, the PHPStan baseline, docs, tests, scripts and repo
metadata so src/ and the tool configuration get reviewed.
Review skipped: 142 files exceed the limit of 100.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
This PR makes xoops/moduletools a standalone PHP 8.4+ Composer library while preserving XOOPS compatibility.
Changes:
Adds Composer packaging, CI, QA, API, and migration tooling.
Adds module, admin, persistence, permission, filesystem, and presentation APIs.
Adds compatibility tests and migration documentation.
Outstanding findings: Critical CSRF gaps remain in src/Admin/ObjectController.php (3 votes), src/Common/Cloner.php (3 votes), and src/Common/TestdataButtons.php (1 vote). Moderate issues remain with missing ext-mbstring (1), embedded autoloader fallbacks in three scripts and tests/bootstrap.php (1 each), ObjectTable filtering/actions (3 and 2), clone cleanup (1), database failure handling (3), text truncation (1), update-cache keys (2), persistence-handler argument forwarding (1), and SmartModules test skipping (2). The security-template link is a nit (2 votes).
…w findings
Copilot's review of XOOPS#1 found two more fail-open token checks and a set
of pre-existing defects; all are addressed here.
Security
- ObjectController::storeFromDefaultForm()/handleObjectDeletion() and
Cloner::handleAdminRequest() now reject the request when no security
service (or its check()) is available, as DirectoryChecker already did.
- TestdataButtons: the load/save/clear links carry the XOOPS token in
both renderers, not only the hide/show toggle.
Fixes
- ObjectTable::addFilter() now renders a select per filter (options from
the handler method) and applies the chosen value to the criteria,
preserving sort/order/limit/start/group-by; an explicit "All" clears a
configured default.
- ObjectTable::addWithSelectedActions() renders a checkbox column and a
"with selected" form posting op, <keyName>[] ids and the token.
- Db::cloneRecord() returns false after a failed source lookup instead of
continuing with a non-array.
- UpdateChecker: the cache key includes the repository so the optional
$repository argument is not shadowed by an earlier result.
- SmartModulesConsumerTest skips unless all four module trees exist.
- Issue template: security contact link points at this repository.
Review skipped: 142 files exceed the limit of 100.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
ObjectRow rendering ignores headers, classes, actions, and row options
src/Admin/SingleView.php:35
ObjectRow::$header and $class are part of the public row contract but are discarded here, so header rows and custom CSS classes always render as an ordinary <dt>/<dd> pair. The constructor's $actions and $headerAsRow options are likewise ignored, making callers receive incorrect detail-view markup instead of the requested configuration.
…leView rows
Second Copilot pass on XOOPS#1.
- Output::selectSorting(): the REQUEST_URI form action, sort hrefs, icon
URLs and label are escaped at emission; SortControlBuilder encodes the
sort key and normalises start to an integer.
- SingleView honours ObjectRow header/class and the constructor's actions
and headerAsRow, which were accepted but ignored.
- ConsumerBridgeGenerator converts boolean columns with a numeric
comparison instead of a bool cast.
Review skipped: 142 files exceed the limit of 100.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
$this->extra is never initialized when the default empty $extra_arg is used, so render() later reads an undefined property. Also, the || condition is always true for a non-empty string, so already-suffixed arguments get another & prefix; initialize the property and require both suffix checks to pass.
Handle unresolved modules before permission checks
src/Object/DynamicObject.php:181
PersistableHandler::getModuleInfo() is declared to return object|false; when the module cannot be resolved, passing that false value to ItemPermission::forModule(object $module) raises a TypeError instead of making accessGranted() return its advertised boolean result. Check the module object before constructing the permission service.
- ObjectController: return after the not-found redirect in handleObjectDeletion
- Blocksadmin: escape the raw block title in the hidden oldtitle input
- ObjectFormBuilder: read control options only when the control is an array
- ConsumerBridgeGenerator: allowlist criteria operators and connectors in the
generated whereClause; skip the UPDATE for key-only tables
- ItemPermission::replace: restore the previous grants when an add fails
- LetterChoice: initialise $extra and require both suffix checks to fail
- DynamicObject::accessGranted: return false when the module cannot be resolved
…te audit
The consumer audit under docs/internal is never committed, so CI regenerated
the matrix with every symbol marked unused and reported the committed
artifacts as stale. When the audit is absent, carry consumer usage over from
the committed destination-matrix.json instead.
Missing module result is dereferenced without validation
src/Common/VersionChecks.php:36
XoopsModule::getByDirname() returns XoopsModule|false when the consumer module cannot be resolved (the Core stub documents that contract), but this path immediately calls getVar() on the result. A missing/invalid module therefore throws instead of returning the documented boolean failure; guard the resolved value with instanceof \XoopsModule before dereferencing it.
This issue also appears on line 65 of the same file.
This extracts only the first character/value from COLUMN_TYPE (substr(..., 5, -6)); for a normal enum('a','b') it returns just a, and SET columns are also parsed incorrectly. enumerate() therefore loses allowed values for the documented ENUM/SET contract. Parse the enum(...)/set(...) body and split the quoted values instead.
- TestdataSample::loadData()/saveData()/clearData() require an administrator session and a valid XOOPS token (TestdataButtons::isAuthorizedRequest()), so a consumer's testdata/index.php cannot run them unguarded
- Paginator HTML-escapes the generated page links; PHP_SELF and urlOther could break out of the href attribute
- Db::enumerate() validates the table name like its siblings; Db::enumValues() parses ENUM/SET column types correctly instead of returning a fragment of the first value
Escape search terms consistently before regex matching
src/Common/Highlighter.php:26
The regex is built from raw search terms but applied to $escaped text. Terms containing & (for example R&D) therefore never match, and a term exactly & matches only the start of &, producing broken output such as <mark>&</mark>amp;. Escape each term with the same HTML settings before preg_quote() so matching preserves the rendered text.
Allow autoloading before installing XOOPS class stubs
tests/fixtures/core-stubs.php:21
These guards suppress autoloading with the second false argument. In an embedded XOOPS run, the real core classes may be registered but not loaded yet, so this test defines empty stubs and permanently shadows the core definitions; later consumers then miss core methods or hit redeclaration errors. Let class_exists() autoload the real class first, or only install these stubs when no XOOPS runtime is available.
- Blocksadmin::isBlockCloned() casts request-supplied module and group ids before building the INSERT statements; updateBlock() deletes only the block's own block_read rows instead of every permission row sharing the item id
- Highlighter matches on the raw text and escapes each piece afterwards, so R&D, quotes and angle brackets are found and a term such as amp can no longer split an entity
- test core stubs let class_exists() autoload the real XOOPS class before installing an empty stand-in
URL-encode query values to preserve embedded ampersands
tests/Common/PaginatorTest.php:35
The expected href preserves the decoded value q=a&b as q=a&b, but the browser decodes & before parsing the URL, turning this into two query parameters (q=a and b). That loses the original q=a%26b value; URL-encode the parsed query values before HTML-escaping and assert %26 here.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
The dependency-review workflow cannot resolve its action ref, the API gate misses stale allowlist removals, and CSV formula protection omits line-feed-prefixed payloads.
Get a fresh assessment by requesting another Copilot review.
API_VERSION remains 1.0.0 even though this release adds public symbols, while the new compatibility policy says this value changes when a minor release adds API. Consumers therefore cannot use ConsumerRuntime::guard() to distinguish the newly expanded API from the previous 1.0 surface. Bump the API version and update the generated snapshots, allowlist, and tests consistently.
Count rendered characters when checking short HTML input
src/Common/Text.php:54
The short-input check counts the encoded spelling instead of the rendered character. For example, truncateHtml('&', 1, '...', true) currently returns the malformed &am... rather than the unchanged one-character text. Normalize entities in this length check just as the loop below does.
Fixed in cfb2947: makeSelBox() retains its original signature and options-array return contract, delegating to makeOptionsArray(). Regression coverage confirms this behavior.
mambax7
changed the title
feat(moduletools): make xoops/moduletools a standalone PHP 8.4 library
feat(moduletools): provide standalone PHP 8.4 library with API 1.1
Sep 21, 2026
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
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.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Summary
Makes
xoops/moduletoolsa self-contained Composer package (PHP 8.4+) that installs, tests and analyses on its own while still running insidexoops_lib/vendoron XOOPS 2.8. Reconciles the 1.4.0 Core tree with the 1.5.0 consolidated cut, fixes a dozen runtime defects found on the way, and adds user documentation.Motivation
The repository held the 1.4.0 source only: the runtime language catalog, the gate scripts and the API snapshots were missing, the test bootstrap required an autoloader three directories up, CI called a
composer analysescript that did not exist, andphpstan.neonwas copied from another package. Separately, two 1.5.0 trees existed with different contents (the site path repo vs. the consolidated cut withDirectoryChecker::handleRequest()), which is what broke Quotes on a fresh site.Approach
tests/bootstrap.phpand the package scripts resolve the localvendor/autoload.php, falling back to Core's. Gate artifacts moved toresources/gate(export-ignored); the site-specific consumer audit is optional.php ^8.4;xoops/xmf ^1.3— every XMF class and method the library calls was checked against 1.3.1. No hardcodedversion; sites pin it on their path repository.composer qa): phpcs PSR-12 → PHPStan level 6 withstubs/xoops-core.stubgenerated from a real 2.8 core (baseline of 164 typing findings, no logic findings) → Rector PHP 8.4 +xoops/rector-xoops(applied) → API-surface/compat, destination-matrix, layer and legacy-global checks → PHPUnit. The API generator now records promoted constructor properties. CI on PHP 8.4 and 8.5.DirectoryChecker::handleRequest()+ action forms (with a fail-closed CSRF check),FileCheckerconstant names and catalog fallback,Blocksadmin_MI_*title tokens, the Cloner logo font, the Cloner/DirectoryChecker tests.Admin\TableandCommon\Highlighterrestored from the path-repo build.Helper::getDirname()(no such method in any XMF) →dirname()at all call sites;DynamicObject::setErrors();Blocksadminundefined$newid/ doublerender();Clonersprintfon placeholder-less constants and silent partial clones;Db::cloneRecord()fetchArrayarity;fputcsvescape (8.4 deprecation);VersionChecksby-ref assignment and dead fallbacks; strict-type mismatches inConfirm/Output/ObjectFormBuilder;LetterChoiceSmarty 5setCaching();UpdateCheckerrepository parameter +github_repomanifest entry; consumer-constant precedence inModuleFeedback/ServerStats.docs/getting-started.md,docs/migrating-a-module.md,docs/compatibility-policy.md.Testing
composer qagreen on PHP 8.4.25: 61 tests / 269 assertions (15 skip unless the package sits inside a XOOPS tree).handleRequest()and one optional parameter added, no removals (composer api:compat).Checklist
composer cs)composer analyse)composer test)