Comprehensive PHPUnit test suite for XOOPS 2.5.x#1605
Open
mambax7 wants to merge 2 commits intoXOOPS:masterfrom
Open
Comprehensive PHPUnit test suite for XOOPS 2.5.x#1605mambax7 wants to merge 2 commits intoXOOPS:masterfrom
mambax7 wants to merge 2 commits intoXOOPS:masterfrom
Conversation
Add 5,500+ tests across 130 test files covering core classes, kernel objects, form elements, frameworks, and modules. All tests pass cleanly on PHP 8.4 with PHPUnit 11.x (0 errors, 0 failures, 0 deprecations). Test coverage includes: - Core classes: XoopsLoad, XoopsSecurity, XoopsCache, XoopsFile, XoopsLogger, XoopsMediaUploader, XoopsPageNav, XoopsPreload, XoopsObjectTree, XoopsUserUtility, XoopsLocalAbstract, XoopsHttpGet, Criteria/CriteriaCompo, SqlUtility, SaxParser, model factory - Form elements: all 20+ XoopsForm* classes with data-driven tests - Kernel objects: XoopsModule (with versionCompare), XoopsUser, XoopsBlock, XoopsComment, XoopsNotification, config handlers, etc. - Frameworks: art (functions.ini/config/cache/user), moduleclasses, textsanitizer - Modules: system, pm, profile, protector Production source fixes discovered via tests: - sqlutility.php: Fix prefixQuery() DROP TABLE bug (pattern2 match used pattern1 for replacement), add IF EXISTS support - saxparser.php: Add is_final=true to xml_parse() so unclosed tags are detected as parse errors - modulesadmin.php: Remove duplicate reservedTables entries, fix array concatenation ($msgs += to array_merge), replace fread/fopen with file_get_contents, add FK checks around SQL/rollback blocks, fix success constant bug, add SET statement skipping Test infrastructure: - New bootstrap.php with XOOPS constant definitions and autoloading - All PHPUnit annotations converted to PHP 8 attributes - Windows path normalization for cross-platform compatibility - Graceful cache/DB skip guards for environment-dependent tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WalkthroughAdds extensive PHPUnit test coverage across many subsystems and introduces targeted production fixes: improved DROP TABLE prefixing, xml_parse finalization flag for string input, and safer module install/uninstall SQL handling with foreign key check wrapping and DROP TABLE IF EXISTS. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a comprehensive PHPUnit 11.x test suite covering 5,543 tests across 131 new test files for XOOPS 2.5.x core classes, modules, and frameworks. It modernizes the test bootstrap infrastructure, converts all PHPUnit annotations to PHP 8 attributes, and fixes 3 production code issues discovered during testing to ensure PHP 8.4 compatibility.
Changes:
- Added 131 test files covering kernel handlers, authentication, cache, file operations, database, modules (system, pm, profile, protector), text sanitizer, and Frameworks/art
- Replaced old bootstrap infrastructure with a robust test bootstrap supporting graceful DB connection handling and optional extensions
- Converted PHPUnit doc-comment annotations to PHP 8 attributes for PHPUnit 11.x compatibility
- Fixed 3 production code issues for PHP 8.4 compatibility (implicit nullable parameters)
Reviewed changes
Copilot reviewed 42 out of 134 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/htdocs/class/file/XoopsFolderHandlerPathTest.php | New comprehensive test for XoopsFolderHandler path operations |
| tests/unit/htdocs/class/file/XoopsFileTest.php | New tests for XoopsFile factory class |
| tests/unit/htdocs/class/database/XoopsDatabaseTest.php | New tests for XoopsDatabase abstract base with test stub |
| tests/unit/htdocs/class/database/XoopsDatabaseFactoryTest.php | New tests for XoopsDatabaseFactory singleton |
| tests/unit/htdocs/class/cache/XoopsCacheKeyTest.php | New tests for XoopsCache key methods |
| tests/unit/htdocs/class/cache/XoopsCacheEngineTest.php | New tests for XoopsCacheEngine abstract class |
| tests/unit/htdocs/class/auth/XoopsAuthXoopsTest.php | New comprehensive tests for XOOPS authentication |
| tests/unit/htdocs/class/auth/XoopsAuthTest.php | New tests for XoopsAuth base class |
| tests/unit/htdocs/class/auth/XoopsAuthLdapTest.php | New tests for LDAP authentication adapter |
| tests/unit/htdocs/class/auth/XoopsAuthFactoryTest.php | New tests for XoopsAuthFactory |
| tests/unit/htdocs/class/auth/XoopsAuthAdsTest.php | New tests for Active Directory authentication |
| tests/unit/htdocs/class/ZipfileTest.php | New tests for Zipfile class |
| tests/unit/htdocs/class/XoopsUserUtilityTest.php | New tests for XoopsUserUtility getIP() method |
| tests/unit/htdocs/class/XoopsSecurityTest.php | New tests for XoopsSecurity token and CSRF protection |
| tests/unit/htdocs/class/XoopsPreloadTest.php | New tests for XoopsPreload event dispatcher |
| tests/unit/htdocs/class/XoopsListsTest.php | New tests for XoopsLists file/directory utilities |
| tests/unit/htdocs/class/XoopsHttpGetTest.php | New tests for XoopsHttpGet HTTP client |
| tests/unit/htdocs/class/MyTextSanitizerTest.php | Updated to use new bootstrap |
| tests/unit/htdocs/Frameworks/textsanitizer/MyTextSanitizerExtendedTest.php | New tests for MyTextSanitizerExtended |
| tests/unit/htdocs/Frameworks/moduleclasses/ModuleAdminTest.php | New tests for ModuleAdmin framework class |
| tests/unit/htdocs/Frameworks/art/XoopsArtTest.php | New tests for xoopsart framework |
| tests/unit/htdocs/Frameworks/art/FunctionsUserTest.php | New tests for art framework user functions |
| tests/unit/htdocs/Frameworks/art/FunctionsIniTest.php | New tests for art framework ini functions |
| tests/unit/htdocs/Frameworks/art/FunctionsConfigTest.php | New tests for art framework config functions |
| tests/unit/htdocs/Frameworks/art/FunctionsCacheTest.php | New tests for art framework cache functions |
| tests/unit/htdocs/Frameworks/art/ArtObjectTest.php | New tests for ArtObject and ArtObjectHandler |
| tests/unit/common_phpunit.php | Removed obsolete bootstrap file |
| tests/unit/bootstrap.php | Removed obsolete bootstrap file |
| htdocs/modules/system/admin/modulesadmin/modulesadmin.php | Fixed implicit nullable parameters and array operations |
| htdocs/class/xml/saxparser.php | Fixed xml_parse call to use 3-parameter form |
| htdocs/class/database/sqlutility.php | Fixed DROP TABLE IF EXISTS pattern matching |
Closed
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| 'url with query string' => ['https://example.com/search?q=test&page=1'], | ||
| 'url with fragment' => ['https://example.com/page#section'], | ||
| 'url with port' => ['https://example.com:8080/api'], | ||
| 'url with auth' => ['https://user:pass@example.com'], |
Check failure
Code scanning / SonarCloud
Credentials should not be hard-coded
| } else { | ||
| $msgs[] = '<p>' . sprintf(_AM_SYSTEM_MODULES_FAILED_SUCESS, "<strong>{$func}</strong>") . '</p>'; | ||
| $msgs += $module->getErrors(); | ||
| $msgs = array_merge($msgs, $module->getErrors()); |
There was a problem hiding this comment.
The array merge on line 105 uses array_merge() which is correct for accumulating errors. However, on the original line 105 (now removed), the code used $msgs += $module->getErrors() which uses the array union operator. This operator will not overwrite existing numeric keys, potentially causing errors to be lost if keys overlap. The fix to array_merge() is correct, but the same issue occurs on lines 874 and 1320 in the same file. All three occurrences should use array_merge() for proper error accumulation.
| { | ||
| if (!is_resource($this->input)) { | ||
| if (!xml_parse($this->parser, $this->input)) { | ||
| if (!xml_parse($this->parser, $this->input, true)) { |
There was a problem hiding this comment.
Line 137 adds the third parameter true to xml_parse(). According to PHP documentation, xml_parse() requires three parameters when parsing a string (non-resource input): xml_parse(XMLParser $parser, string $data, bool $is_final). The original code only passed two parameters, which would cause a deprecation notice on PHP 8.4+. Adding true as the third parameter indicates this is the final chunk of data, which is correct for parsing a complete string. However, ensure that when $this->input is a resource (file handle), the code path on line 142 (not shown in diff) properly handles incremental parsing with is_final set appropriately.
htdocs/class/database/sqlutility.php
Outdated
| $pattern2 = "/^(DROP TABLE)(\s)+([`]?)([^`\s]+)\\3(\s)?$/siU"; | ||
| if (preg_match($pattern, $query, $matches) || preg_match($pattern2, $query, $matches)) { | ||
| $replace = "\\1 " . $prefix . "_\\4\\5"; | ||
| $pattern2 = "/^(DROP[\s]+TABLE(?:\s+IF\s+EXISTS)?)(\s)+([`]?)([^`\s]+)\\3(\s)?$/siU"; |
There was a problem hiding this comment.
The updated regex pattern on line 156 now correctly handles DROP TABLE IF EXISTS statements by making the IF EXISTS clause optional with (?:\s+IF\s+EXISTS)?. However, the original pattern on line 156 (before the change) would not match DROP TABLE IF EXISTS statements, causing prefixQuery() to return false and potentially skipping table prefix replacement. The fix ensures proper handling of both DROP TABLE and DROP TABLE IF EXISTS syntax. Verify that all SQL files in the codebase that use DROP TABLE IF EXISTS are now correctly prefixed during module installation/uninstallation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 60
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
htdocs/modules/system/admin/modulesadmin/modulesadmin.php (1)
119-167:⚠️ Potential issue | 🟠 MajorGuard unreadable SQL files and use exec() for FK toggles.
If
file_get_contents()fails (permissions, lock, etc.), the current flow trims to an empty string and proceeds with an “install” that creates no tables. Also,SET FOREIGN_KEY_CHECKSis a write statement; usingquery()can emit warnings in strict DB mode. Add a read guard, initialize$created_tablesbefore the read for rollback safety, and useexec()for FK toggles here and in the rollback/uninstall blocks (Lines 175-179, 720-735). Consider a more precise message constant if available.🛠️ Proposed fix (core block)
- $sql_query = file_get_contents($sql_file_path); - $sql_query = trim($sql_query); - SqlUtility::splitMySqlFile($pieces, $sql_query); - $created_tables = []; - $db->query('SET FOREIGN_KEY_CHECKS = 0'); - foreach ($pieces as $piece) { + $created_tables = []; + $sql_query = file_get_contents($sql_file_path); + if ($sql_query === false) { + $errs[] = '<p>' . sprintf(_AM_SYSTEM_MODULES_SQL_NOT_FOUND, '<strong>' . basename($sql_file_path) . '</strong>'); + $error = true; + } else { + $sql_query = trim($sql_query); + SqlUtility::splitMySqlFile($pieces, $sql_query); + $db->exec('SET FOREIGN_KEY_CHECKS = 0'); + foreach ($pieces as $piece) { // Skip SET statements if (preg_match('/^SET\s+/i', $piece)) { continue; } ... - } - $db->query('SET FOREIGN_KEY_CHECKS = 1'); + } + $db->exec('SET FOREIGN_KEY_CHECKS = 1'); + }As per coding guidelines: Use basename() in error messages to avoid exposing server paths.
Also applies to: 175-179, 720-735
🤖 Fix all issues with AI agents
In `@htdocs/class/database/sqlutility.php`:
- Around line 155-166: The DROP TABLE regex in $pattern2 is too strict about
trailing whitespace; update the pattern2 definition in sqlutility.php (the
variable named $pattern2 used by prefixQuery()/preg_match) to allow any trailing
whitespace by replacing the final group `(\s)?` with `(\s*)` (or `\s*$`) so DROP
TABLE statements with multiple spaces or newlines still match; keep the rest of
the pattern and the subsequent preg_replace usage unchanged to preserve capture
groups.
In `@tests/unit/htdocs/class/auth/XoopsAuthAdsTest.php`:
- Around line 367-385: The four trivial method-existence tests
(testHasGetUpnMethod, testHasAuthenticateMethod,
testHasGetUserDNMethodInherited, testHasLoadXoopsUserMethodInherited) are
redundant; remove them or replace each with a small behavioral assertion that
exercises the actual method (e.g., call
getUPN/getUserDN/authenticate/loadXoopsUser with a minimal/mock input and assert
expected output/side-effect) so coverage verifies behavior rather than mere
presence—locate these methods in class XoopsAuthAdsTest and either delete those
test methods or change their bodies to invoke the corresponding methods and
assert a real outcome.
- Around line 323-330: Wrap the global config setup/teardown in
testInheritedGetHtmlErrors in a try/finally so unset($GLOBALS['xoopsConfig'])
always runs: set $GLOBALS['xoopsConfig'] = ['debug_mode' => 1] before the try,
run the existing test assertions inside the try block, and move the
unset($GLOBALS['xoopsConfig']) into the finally block to guarantee cleanup even
on failure.
- Around line 7-8: Remove PHP 8 attribute usage and PHPUnit 10-only imports in
XoopsAuthAdsTest: delete the use statements for
PHPUnit\Framework\Attributes\CoversClass and
PHPUnit\Framework\Attributes\DataProvider and replace any #[CoversClass(...)]
and #[DataProvider(...)] attribute annotations with equivalent docblock
annotations (e.g., /** `@covers` \XoopsAuthAds */ above the class declaration and
/** `@dataProvider` upnDataProvider */ above the test method). Ensure you update
all occurrences referenced (including the other instances around the indicated
spots) so the class and test methods use only docblock `@covers` and `@dataProvider`
annotations compatible with PHP 7.4 and PHPUnit 9.6–11.x.
In `@tests/unit/htdocs/class/auth/XoopsAuthFactoryTest.php`:
- Around line 1-17: Add the standard XOOPS file header to the top of the file
and a class docblock for the XoopsAuthFactoryTest class: insert the XOOPS
copyright/header block before the namespace declaration and add a docblock
immediately above the XoopsAuthFactoryTest class declaration that includes the
required tags `@category`, `@package`, `@author`, `@copyright`, `@license`, and `@link`;
ensure the docblock references the class name XoopsAuthFactoryTest and matches
XOOPS formatting guidelines.
- Around line 7-114: The file XoopsAuthFactoryTest uses PHP 8 attributes and
non-standard test method names; remove all attribute imports
(PHPUnit\Framework\Attributes\CoversClass, DataProvider, Test) and delete every
attribute usage (e.g. #[CoversClass(XoopsAuthFactory::class)] and each
#[Test]/#[DataProvider]) and replace class-level coverage with a PHPDoc `@covers`
XoopsAuthFactory (above class XoopsAuthFactoryTest). Rename each test method to
the PHPUnit 7.4 convention by prefixing with "test" (e.g. classExists ->
testClassExists, classIsNotAbstract -> testClassIsNotAbstract, classIsNotFinal
-> testClassIsNotFinal, classHasGetAuthConnectionMethod ->
testClassHasGetAuthConnectionMethod, getAuthConnectionMethodIsStatic ->
testGetAuthConnectionMethodIsStatic, getAuthConnectionMethodIsPublic ->
testGetAuthConnectionMethodIsPublic, getAuthConnectionAcceptsOneParameter ->
testGetAuthConnectionAcceptsOneParameter, getAuthConnectionParameterNameIsUname
-> testGetAuthConnectionParameterNameIsUname,
getAuthConnectionParameterIsRequired ->
testGetAuthConnectionParameterIsRequired, classHasOnlyOnePublicMethod ->
testClassHasOnlyOnePublicMethod) and ensure any `@dataProvider` annotations (if
used) are added as PHPDoc above the method.
- Around line 128-199: The tests that assume
XoopsAuthFactory::getAuthConnection('...') always throws a TypeError need to
handle PHP version differences: in PHP 7.4 in_array($needle, null) emits a
warning and returns false while PHP >= 8.0 throws TypeError. Update each
affected test method (getAuthConnectionThrowsTypeErrorWithEmptyConfig,
getAuthConnectionTypeErrorMentionsInArray,
getAuthConnectionTypeErrorMentionsArrayType,
getAuthConnectionRepeatedCallsThrowSameTypeError,
getAuthConnectionNeverCachesOnError) to branch on PHP version (use
PHP_VERSION_ID or version_compare(PHP_VERSION, '8.0.0', '>=')): for PHP >= 8.0
keep expecting/catching TypeError from XoopsAuthFactory::getAuthConnection, for
PHP < 8.0 adjust to expect a warning (use
$this->expectWarning()/expectWarningMessage() or assertNoException and assert
behavior like returning false) so tests pass on both runtimes while still
calling XoopsAuthFactory::getAuthConnection to exercise the same code path.
In `@tests/unit/htdocs/class/auth/XoopsAuthLdapTest.php`:
- Around line 16-29: The class docblock for XoopsAuthLdapTest (above the class
XoopsAuthLdapTest declaration and CoversClass attribute) is missing required
XOOPS metadata tags; update that docblock to include `@category`, `@package`,
`@author`, `@copyright`, `@license`, and `@link` tags with appropriate values (e.g.,
XOOPS, unit tests package, author name/email or project placeholder, copyright
notice, SPDX or license text, and project URL) so the XoopsAuthLdapTest class
docblock complies with the coding guidelines.
- Around line 28-286: The PHP 8 attribute usage in class XoopsAuthLdapTest
(#[CoversClass(\XoopsAuthLdap::class)]) and the method-level
#[DataProvider('cp1252DataProvider')] must be converted to PHP 7.4-compatible
docblock annotations: remove the attribute lines and add a class docblock with
`@covers` \XoopsAuthLdap above the class declaration, and add a docblock with
`@dataProvider` cp1252DataProvider above the testCp1252ToUtf8WithDataProvider
method (the cp1252DataProvider method name identifies the provider).
In `@tests/unit/htdocs/class/auth/XoopsAuthProvisionningTest.php`:
- Around line 7-10: Replace PHP 8 attributes in the XoopsAuthProvisionningTest
class with PHPUnit 9.6-compatible docblock annotations: remove all occurrences
of #[CoversClass(...)] / #[Test] / #[DataProvider(...)] and instead add
corresponding docblock tags above the test class/methods — use `@covers` for the
class or specific methods, `@test` above methods that were #[Test], and
`@dataProvider` <providerName> above methods that used #[DataProvider]. Ensure the
class name XoopsAuthProvisionningTest keeps the same coverage targets and
provider names when converting attributes to `@covers` and `@dataProvider`
annotations.
In `@tests/unit/htdocs/class/auth/XoopsAuthTest.php`:
- Around line 1-16: The file is missing the standard XOOPS header and class
docblock metadata; add the XOOPS copyright header block at the very top of the
file and add a class-level PHPDoc immediately above the XoopsAuthTest class
declaration that includes `@category`, `@package`, `@author`, `@copyright`, `@license`,
and `@link` tags (retain namespace xoopsauth and the existing PHPUnit attributes
and class name XoopsAuthTest so only header and the docblock are added).
- Around line 7-37: Remove PHP 8 attribute usage in the XoopsAuthTest class and
replace them with PHPUnit docblock annotations, and rename test methods to the
test* prefix: remove imports of PHPUnit\Framework\Attributes\CoversClass, Test
and DataProvider, delete the #[CoversClass(XoopsAuth::class)] and
#[Test]/#[DataProvider(...)] attribute lines, add /** `@covers` XoopsAuth */ above
the class, use /** `@test` */ is optional but instead add /** `@test` */-style is
not needed—add /** `@covers` XoopsAuth */ and for data providers use /**
`@dataProvider` providerName */ above the test method, and rename methods like
constructorWithNullDaoDefault() to testConstructorWithNullDaoDefault() (and any
other test methods) so PHPUnit 7/7.4 will discover them; keep setUp() and
tearDown() as-is and ensure provider methods stay public static or public.
In `@tests/unit/htdocs/class/auth/XoopsAuthXoopsTest.php`:
- Around line 7-10: The file imports PHPUnit attribute classes and uses PHP 8
attributes which break PHP 7.4; remove the use lines for
PHPUnit\Framework\Attributes\CoversClass, DataProvider, and Test and keep only
use PHPUnit\Framework\TestCase; then replace each attribute decorator on the
XoopsAuthXoopsTest test methods (the #[CoversClass(...)] / #[DataProvider(...)]
/ #[Test] blocks) with equivalent docblock annotations immediately above the
method signature (e.g. /** `@covers` ClassName */ /** `@dataProvider` providerName
*/ /** `@test` */), ensuring the class name in `@covers` matches the original
CoversClass argument and the provider names match the original DataProvider
arguments so PHPUnit 9.6 on PHP 7.4 will run the tests.
In `@tests/unit/htdocs/class/cache/XoopsCacheEngineTest.php`:
- Around line 112-117: Replace the no-op assertion in
XoopsCacheEngineTest::gcDoesNotThrow by using PHPUnit's intent-revealing helper:
call $this->expectNotToPerformAssertions() at the start of gcDoesNotThrow(),
then invoke $this->engine->gc(); so the test verifies the method runs without
throwing instead of using $this->assertTrue(true).
- Around line 7-13: The PHPUnit 10-only attributes (#[CoversClass] and #[Test])
in XoopsCacheEngineTest break compatibility with PHPUnit 9.6; remove those PHP 8
attributes from the test class and test methods in XoopsCacheEngineTest and
replace them with PHPUnit 9-compatible constructs—either add docblock
annotations like `@covers` XoopsCacheEngine and `@test` above the class/methods that
used #[CoversClass] and #[Test], or keep the annotations off and rename the test
methods to start with test (e.g., testSomething) so PHPUnit 9.6 will discover
them; update references to XoopsCacheEngine and XoopsCacheEngineInterface in the
file accordingly.
In `@tests/unit/htdocs/class/cache/XoopsCacheKeyTest.php`:
- Around line 133-139: Replace the non-descriptive local variables in the test
method getInstanceReturnsSingleton by using clearer names (e.g., $instance1 and
$instance2 or $first and $second) when calling XoopsCache::getInstance(); update
the assertions to assertSame($instance1, $instance2) accordingly so the intent
of testing the singleton returned by XoopsCache::getInstance() is clearer.
- Around line 7-13: Remove PHP 8 PHPUnit attributes (#[CoversClass], #[Test],
#[DataProvider]) from the XoopsCacheKeyTest class and its methods and replace
them with cross-version-compatible docblock annotations or test method renames:
add a class docblock with `@covers` XoopsCache and for each test method either
rename the method to start with test (e.g., testSomething) or add method
docblocks with `@test` and `@dataProvider` <providerName>, ensuring any `@covers`
annotations are present on the individual test docblocks; update references to
the DataProvider function names (same symbol names) so PHPUnit 9.6–11.x can
discover the tests.
In `@tests/unit/htdocs/class/cache/XoopsCacheTest.php`:
- Around line 9-686: Large monolithic test class XoopsCacheTest makes
maintenance harder; split tests into focused classes. Create new test classes
(e.g., XoopsCacheConfigTest, XoopsCacheKeyTest, XoopsCacheWriteReadTest) and
move related methods from XoopsCacheTest into them (refer to methods like
testConfigWithDefaultName, testConfigWithCustomSettings,
testKeyGenerationWithValidKey, testKeyGenerationWithSlashes,
testWriteWithValidData, testWriteAndReadCycle, testDeleteExistingKey, etc.),
keeping setUp logic that initializes XoopsCache (the XoopsCache::getInstance()
call and the Windows path skip check) in a shared base TestCase or duplicated
minimal setUp in each class; update PHPUnit declarations/namespaces accordingly
and run tests to ensure all moved tests pass.
In `@tests/unit/htdocs/class/CriteriaTest.php`:
- Around line 1-25: Add the mandatory XOOPS file header comment block at the top
of this file and expand the class docblock for the test class to include the
required metadata tags: `@category`, `@package`, `@author`, `@copyright`, `@license`, and
`@link`; update the docblock that documents the Criteria tests (namespace
xoopsclass, class CriteriaTest extending PHPUnit\Framework\TestCase) so it
contains those tags and the standard XOOPS header above the <?php line.
- Line 10: Remove the "use PHPUnit\Framework\Attributes\DataProvider;" import
and replace every PHP 8 attribute usage of #[DataProvider('...')] on test
methods in CriteriaTest (e.g., above methods like testSomething or other test*
functions) with the equivalent PHPUnit 9 docblock annotation /** `@dataProvider`
providerName */ immediately above the method signature; ensure the provider name
string is preserved exactly, remove the attribute line, and keep the existing
data provider methods unchanged so tests run under PHPUnit 9.6–11.
In `@tests/unit/htdocs/class/database/SqlUtilityTest.php`:
- Around line 12-26: Update the class docblock at the top of
tests/unit/htdocs/class/database/SqlUtilityTest.php to include the required
XOOPS metadata tags: add `@category`, `@package`, `@author`, `@copyright`, `@license`, and
`@link` entries in the existing docblock for the SqlUtilityTest test class (the
docblock immediately above the class and describing SqlUtility tests); populate
them with appropriate values consistent with project conventions (e.g.,
project/category, package name, author name/email, copyright holder and year,
SPDX or license URL, and project/link URL).
- Around line 27-1043: The test file uses PHP 8 attributes
#[CoversClass(SqlUtility::class)] and
#[DataProvider('supportedCommandsProvider')] which break on PHP 7.4; update the
SqlUtilityTest class by removing those attributes and replacing them with
PHPUnit docblock annotations: add a class-level docblock with `@covers` SqlUtility
(or `@covers` \SqlUtility) above class SqlUtilityTest to replace #[CoversClass],
and add a method-level docblock with `@dataProvider` supportedCommandsProvider
immediately above testPrefixQuerySupportedCommands to replace #[DataProvider];
ensure the annotations are standard /** ... */ PHPDoc comments so the tests
remain compatible with PHP 7.4.
In `@tests/unit/htdocs/class/database/XoopsDatabaseFactoryTest.php`:
- Around line 1-13: Add the standard XOOPS file header block at the very top of
the file (above declare(strict_types=1)) and insert a class docblock immediately
above the XoopsDatabaseFactoryTest class declaration; the class docblock must
include the required tags: `@category`, `@package`, `@author`, `@copyright`, `@license`,
and `@link`, filled with the appropriate project/project-owner values, and should
reference the XoopsDatabaseFactoryTest class and purpose.
- Around line 7-24: Remove the PHP 8 attributes and rename the test method:
delete the #[CoversClass(XoopsDatabaseFactory::class)] and #[Test] attributes
from the XoopsDatabaseFactoryTest class, and rename the method
constructorDoesNotThrow() to testConstructorDoesNotThrow() so PHPUnit 9.6 / PHP
7.4 will discover it (ensure the assertion still uses
XoopsDatabaseFactory::class and the class remains XoopsDatabaseFactoryTest
extends TestCase).
In `@tests/unit/htdocs/class/file/XoopsFileHandlerTest.php`:
- Around line 14-52: Update the class docblock at the top of
tests/unit/htdocs/class/file/XoopsFileHandlerTest.php that documents
XoopsFileHandler tests to include the required XOOPS metadata tags: add
`@category`, `@package`, `@author`, `@copyright`, `@license`, and `@link` entries in the
docblock above the XoopsFileHandlerTest (the block that currently documents
tested API and uses #[CoversClass(XoopsFileHandler::class)]); ensure the tags
follow project formatting and contain appropriate values (author name/email,
copyright holder, license identifier and URL, package/category, and a relevant
link).
- Around line 52-1127: The test file uses PHP 8 attributes that break PHP 7.4;
remove the attribute lines #[CoversClass(XoopsFileHandler::class)] on the
XoopsFileHandlerTest class and #[DataProvider('unsafeFilenameProvider')] on the
testSafeWithDataProvider method, and replace them with PHPUnit docblock
annotations: add a class docblock with `@covers` XoopsFileHandler (above class
XoopsFileHandlerTest) and add a method docblock with `@dataProvider`
unsafeFilenameProvider (above testSafeWithDataProvider), ensuring the annotation
text matches the attribute targets so PHPUnit still associates the coverage and
data provider correctly.
In `@tests/unit/htdocs/class/file/XoopsFileTest.php`:
- Around line 7-10: The test file imports and uses the PHP 8 attribute
PHPUnit\Framework\Attributes\CoversClass which breaks on PHP 7.4; replace the
attribute usage (e.g. #[CoversClass(XoopsFile::class)] around the XoopsFileTest
class or methods) with a docblock `@covers` annotation above the class/method and
remove the Attributes import; update the use statements to remove
PHPUnit\Framework\Attributes\CoversClass and ensure the class XoopsFileTest (and
any methods referencing CoversClass) has a /** `@covers` XoopsFile */ docblock
instead so the tests remain compatible with PHP 7.4.
In `@tests/unit/htdocs/class/file/XoopsFolderHandlerPathTest.php`:
- Around line 1-15: Add the standard XOOPS file header comment block at the top
of the file and add a class-level docblock immediately above the
XoopsFolderHandlerPathTest class declaration containing the required tags
(`@category`, `@package`, `@author`, `@copyright`, `@license`, `@link`); ensure the header
uses the same copyright/license text used across the project and the class
docblock references XoopsFolderHandlerPathTest and its purpose so it matches
project conventions.
In `@tests/unit/htdocs/class/XoopsListsTest.php`:
- Around line 24-25: The private property $tempDir in class XoopsListsTest
currently has no declared type; update its declaration to include an explicit
string type (e.g., change "private $tempDir = '';" to "private string $tempDir =
'';" in the XoopsListsTest class) to improve type safety and IDE support while
keeping the existing default value.
- Line 5: The namespace declaration in XoopsListsTest.php currently uses
lowercase `xoopsclass`; update it to follow PSR-12 PascalCase (for example
`XoopsClass` or a structured test namespace like `Xoops\Tests\Unit`) by changing
the namespace statement at the top of the file and adjusting any references
(imports or fully-qualified class names) used by the XoopsListsTest class so
they reflect the new namespace.
- Around line 7-9: The test uses PHP 8 PHPUnit attributes (#[CoversClass],
#[DataProvider]) which break compatibility with PHPUnit 9.6; update the
XoopsListsTest class to use docblock annotations instead of attributes: remove
the #[CoversClass] and #[DataProvider] attributes and add equivalent "@covers
ClassName" above the test class (matching the class being covered) and
"@dataProvider providerMethodName" doc-comments above the relevant test methods
or providers; ensure you update the class declaration XoopsListsTest and any
referenced provider method names so PHPUnit 9.6, 10.x and 11.x can run the
tests.
In `@tests/unit/htdocs/class/XoopsLoadTest.php`:
- Around line 1-28: Add the standard XOOPS file header block at the top of the
file and expand the class-level PHPDoc for the XoopsLoadTest class to include
the required tags: `@category`, `@package`, `@author`, `@copyright`, `@license`, and
`@link`; ensure the header is the same standard XOOPS copyright header used across
the project and that the XoopsLoadTest docblock (above class XoopsLoadTest in
namespace xoopsclass) contains all required tags with correct project and
author/license values.
- Around line 54-62: Add missing PHPDoc tags to public parameterized test
methods (e.g., testLoadCoreConfigContainsExpectedKeys,
testLoadAllDeprecatedNames, testLoadCoreConfigContainsFormClasses): add `@param`
string $key (or appropriate type/name for each parameter), add `@return` void, and
add `@throws` \Throwable (or the specific exception type if known) to each
method's docblock so all public test methods include `@param`, `@return`, and
`@throws` per coding guidelines.
- Around line 107-120: The test testLoadCoreConfigFilesExistOnDisk is leaking
full filesystem paths in its assertion messages; when iterating $configs
returned by XoopsLoad::loadCoreConfig() replace the direct {$path} in the
assertFileExists message with basename($path) (and similarly use basename($path)
in any other failure messages) so the assertion still identifies the file by
name but does not expose full server paths; keep the existing logic for
$optional entries but use basename($path) in the assertIsString message if you
display the path there.
- Around line 7-9: The tests use PHP 8 attributes (#[CoversClass],
#[DataProvider]) which break on PHP 7.4/PHPUnit 9.6; open the XoopsLoadTest
class and remove the attribute imports (PHPUnit\Framework\Attributes\CoversClass
and DataProvider) and replace each occurrence of #[CoversClass(...)] with a
docblock `@covers` annotation immediately above the class or method, and each
#[DataProvider('name')] with a docblock `@dataProvider` annotation immediately
above the test method (apply same change for the DataProvider occurrences
referenced in the review), ensuring TestCase extends PHPUnit\Framework\TestCase
remains unchanged.
In `@tests/unit/htdocs/class/XoopsLocalAbstractTest.php`:
- Around line 12-36: Update the class docblock above the XoopsLocalAbstractTest
declaration to include the required XOOPS metadata tags: add `@category`,
`@package`, `@author`, `@copyright`, `@license`, and `@link` entries in the block that
documents the tests for XoopsLocalAbstract; ensure the tags reference
appropriate values (e.g., the XOOPS project/package and copyright holder) and
follow the existing docblock style so tools consuming the docblock (and unit
test metadata) recognize them.
- Around line 36-169: The test class uses PHP 8 attribute syntax
(#[CoversClass(XoopsLocalAbstract::class)] and
#[DataProvider('substrProvider')]) which breaks on PHP 7.4; replace these
attributes in class-level and method-level declarations with equivalent docblock
annotations (use `@covers` XoopsLocalAbstract for the CoversClass attribute and
`@dataProvider` substrProvider for the DataProvider attribute) and apply the same
replacement for any other attribute usages in this file so the tests parse under
PHP 7.4.
In `@tests/unit/htdocs/class/XoopsMediaUploaderTest.php`:
- Line 7: The test class XoopsMediaUploaderTest is using the PHP 8 attribute
DataProvider which breaks PHP 7.4 and PHPUnit 9.6; remove the import "use
PHPUnit\Framework\Attributes\DataProvider;" and replace any occurrences of the
attribute syntax (e.g. #[DataProvider('providerName')]) on test methods with the
PHPUnit docblock annotation /** `@dataProvider` providerName */ placed immediately
above the corresponding test method (ensure the referenced provider methods like
providerName are present and public/static as needed); do this for every
attribute occurrence in XoopsMediaUploaderTest (including the ones noted around
the earlier diff locations) so tests remain compatible across PHPUnit 9.6/10/11
and PHP 7.4+.
- Around line 326-346: Remove or properly mark the placeholder test method
testCheckMaxFileSizeReturnsTrueWhenNotSet in XoopsMediaUploaderTest: delete the
dead reflection/setup code and the meaningless $this->assertTrue(true), or
replace the test body with a single call to $this->markTestIncomplete() to
signal the edge-case is not testable as written; ensure any unused variables
like $arr and comments about isset/$maxFileSize are removed so the test suite no
longer contains a no-op assertion.
In `@tests/unit/htdocs/class/XoopsObjectTreeTest.php`:
- Around line 571-591: In testLargeTreeStructure inside class
XoopsObjectTreeTest remove the unused variable $id (initialized but never used)
— simply delete the "$id = 1;" declaration in the test method so the method only
builds $specs and calls self::buildTree($specs) and getTree(); no other changes
required.
- Around line 7-14: Replace the PHP 8 attribute usage (e.g.,
#[CoversClass(\XoopsObjectTree::class)] and any #[DataProvider(...)] on test
methods) in the XoopsObjectTreeTest with equivalent doc-block annotations so the
tests remain parseable on PHP 7.4; specifically remove the attribute lines and
add standard PHPUnit docblock tags (e.g., /** `@covers` \XoopsObjectTree */ above
the class and /** `@dataProvider` providerName */ above the corresponding test
methods) ensuring the provider function names (the methods referenced by
DataProvider) remain unchanged.
- Around line 551-565: Add an inline comment explaining the intentional use of
the error suppression operator in the tests that validate magic __get behavior:
in the methods testMagicGetUnknownPropertyReturnsNull and
testMagicGetAnotherUnknownPropertyReturnsNull, annotate the lines using
@$tree->nonExistentProperty and @$tree->_objects with a short comment like
"intentionally suppress warnings to test __get behavior" so future maintainers
understand the @ is deliberate and should be kept.
In `@tests/unit/htdocs/class/XoopsPageNavTest.php`:
- Around line 7-9: Replace PHP 8 attributes `#[CoversClass(...)]` and
`#[DataProvider(...)]` in the XoopsPageNavTest test file with PHPUnit docblock
annotations so the file parses on PHP 7.4 and works with PHPUnit 9.6;
specifically, remove occurrences of `#[CoversClass(...)]` (applied to the test
class) and `#[DataProvider(...)]` (applied to test methods around lines 25–26)
and add equivalent `@covers ClassName` in the class docblock for
XoopsPageNavTest and `@dataProvider providerMethodName` in the docblocks
immediately above each test method that used `DataProvider`, preserving the same
provider method names and coverage targets (use the class name referenced in the
original CoversClass attribute).
In `@tests/unit/htdocs/class/XoopsPreloadTest.php`:
- Around line 1-4: This file (tests/unit/htdocs/class/XoopsPreloadTest.php) is
missing the required XOOPS copyright header block; add the standard XOOPS file
header comment at the very top of the file before the opening <?php tag (so it
precedes declare(strict_types=1); and the class XoopsPreloadTest declaration),
ensuring it matches the project's canonical header format used across other
source files.
- Around line 13-18: Add the required metadata tags to every class docblock in
this file (e.g., the XoopsPreloadTest class) by adding `@category`, `@package`,
`@author`, `@copyright`, `@license`, and `@link` tags with appropriate values; update
each class docblock (search for "class XoopsPreloadTest" and any other "class"
declarations in this file) to include those six tags so the docblocks comply
with the coding guidelines.
- Around line 7-97: Remove the PHP 8 attribute usage and import: delete the line
"use PHPUnit\Framework\Attributes\CoversClass;" and remove the two attribute
lines #[CoversClass(XoopsPreload::class)] and
#[CoversClass(XoopsPreloadItem::class)] that appear immediately above the
XoopsPreloadTest class declaration; instead add a DocBlock above the
XoopsPreloadTest class with `@covers` XoopsPreload and `@covers` XoopsPreloadItem
annotations so PHPUnit 9.6/PHP 7.4 can parse the file and retain the same
coverage mapping.
In `@tests/unit/htdocs/class/XoopsSecurityTest.php`:
- Around line 1-3: This file is missing the mandatory XOOPS copyright header
block; insert the standard XOOPS header comment block at the top of the file
(immediately after the opening <?php and before declare(strict_types=1);) so the
file containing the XoopsSecurityTest class includes the required
copyright/license header per project guidelines.
- Around line 57-64: Add proper class-level docblocks to both
TestableXoopsSecurity and XoopsSecurityTest classes: include the required tags
`@category`, `@package`, `@author`, `@copyright`, `@license`, and `@link`, plus a short
descriptive summary; place the docblocks immediately above the class
declarations (for TestableXoopsSecurity and XoopsSecurityTest) following the
project's docblock format so automated linters and documentation tools recognize
them.
- Around line 92-95: The validateToken() method currently reads the token from
$_REQUEST directly; change that to use Xmf\Request::getVar() (or
Xmf\Request::getString()) to retrieve and filter the incoming token (e.g.
Xmf\Request::getVar($name . '_REQUEST', '', 'REQUEST') or getString($name .
'_REQUEST', '')) instead of $_REQUEST[$name . '_REQUEST'], or alternatively make
the method require an explicit $token and remove implicit request access; update
the line that sets $token so it uses the Xmf\Request call and retains the
existing default/empty checks against $_SESSION[$name . '_SESSION'].
In `@tests/unit/htdocs/class/XoopsUserUtilityTest.php`:
- Around line 1-3: Add the canonical XOOPS copyright header block at the top of
tests/unit/htdocs/class/XoopsUserUtilityTest.php by inserting the standard
header (copy from an existing repo PHP file) immediately above the existing
declare(strict_types=1); line so the file starts with the XOOPS header followed
by <?php and declare(strict_types=1);; ensure the header exactly matches the
project's canonical header format.
- Around line 62-65: The test unconditionally unsets
$GLOBALS['xoopsConfig']['proxy_env'], which can raise notices if
$GLOBALS['xoopsConfig'] isn't set; before calling
unset($GLOBALS['xoopsConfig']['proxy_env']) (in XoopsUserUtilityTest), ensure
$GLOBALS['xoopsConfig'] is an array (e.g. if (!isset($GLOBALS['xoopsConfig']) ||
!is_array($GLOBALS['xoopsConfig'])) { $GLOBALS['xoopsConfig'] = []; }) or guard
the unset with if (isset($GLOBALS['xoopsConfig']['proxy_env'])) unset(...); so
the test remains fully isolated and notice-free.
- Around line 11-21: The class docblock for XoopsUserUtilityTest is missing
standard metadata tags; update the docblock above class XoopsUserUtilityTest to
include `@category`, `@package`, `@author`, `@copyright`, `@license`, and `@link` entries
(fill values consistent with project conventions, e.g., project/category name
for `@category`, the appropriate package name for `@package`, the maintainer for
`@author`, copyright holder and year for `@copyright`, SPDX or URL for `@license`, and
the project or repo URL for `@link`) so the docblock contains all required
metadata.
In `@tests/unit/htdocs/class/ZipfileTest.php`:
- Around line 123-132: Rename the ambiguous variables $t1 and $t2 in the
testUnix2DosTimeReturnsDifferentValuesForDifferentDates to descriptive names
(for example $timestamp2000 and $timestamp2020 or $dateJan1_2000 and
$dateJun15_2020) so it's clear what dates they're holding; update their uses in
the two mktime calls and the subsequent assertions that call
$this->zip->unix2DosTime(...) so the test behavior is unchanged but the intent
is clearer.
- Around line 7-9: The file uses PHP 8 attributes (#[CoversClass] and
#[DataProvider]) which break compatibility with PHPUnit 9.6 / PHP 7.4; replace
those attributes on the ZipfileTest class and any test methods with the
equivalent PHPDoc annotations (e.g. use /** `@covers` Fully\Qualified\ClassName */
above the class and /** `@dataProvider` providerMethodName */ above the test
methods) and remove the attribute syntax and related imports so the tests run
across PHPUnit 9.6, 10.5, and 11.x on PHP 7.4–8.5.
- Around line 334-360: The 'unicode filename' case in filenameProvider() doesn't
contain real multi-byte characters—replace the value for the 'unicode filename'
key with an actual Unicode string (e.g., include non-ASCII characters) so the
test validates Unicode normalization, and change the test attribute usage from
the PHP 8 attribute #[DataProvider('filenameProvider')] to the PHPUnit docblock
annotation `@dataProvider` filenameProvider in the
testAddFileStoresCorrectFilename method to ensure the provider is recognized.
In `@tests/unit/htdocs/Frameworks/art/FunctionsIniTest.php`:
- Around line 265-271: The test assigns an unused variable $result when calling
load_functions('cache', 'art'); remove the unused assignment and simply call
load_functions('cache', 'art') (or call it without assigning to $result) in the
loadFunctionsUsesArtAsDefaultDirname test in FunctionsIniTest.php, keeping the
existing assertion $this->assertTrue(defined('FRAMEWORKS_ART_FUNCTIONS_CACHE'));
this removes the unused variable flagged by static analysis.
In `@tests/unit/htdocs/Frameworks/art/FunctionsUserTest.php`:
- Around line 7-10: The PHPUnit attribute imports (CoversFunction, Test) in the
FunctionsUserTest class break PHP 7.4 compatibility; remove the lines importing
PHPUnit\Framework\Attributes\CoversFunction and
PHPUnit\Framework\Attributes\Test, delete any #[Test] and #[CoversFunction(...)]
attribute decorators on methods in FunctionsUserTest, and replace them by either
adding an `@test` docblock above each test method or renaming the methods to start
with test (e.g., testSomething) so PHPUnit recognizes them; ensure the class
still extends TestCase and keep any existing assertions unchanged.
In `@tests/unit/htdocs/Frameworks/moduleclasses/ModuleAdminTest.php`:
- Around line 7-12: Replace PHP 8 attributes with docblock annotations to
maintain PHP 7.4 and PHPUnit 9.6+ compatibility: remove the
#[CoversClass(ModuleAdmin::class)] and any #[Test] attributes in ModuleAdminTest
and add a class-level docblock with `@covers` \ModuleAdmin above the
ModuleAdminTest class declaration, and convert any #[Test] uses to either `@test`
docblock annotations or rely on the existing test* method name prefix so PHPUnit
recognizes the tests.
- Around line 39-42: The tearDown() method currently only unsets
$GLOBALS['xoTheme'] but setUp() also sets $GLOBALS['xoopsModule'] and
$GLOBALS['xoopsConfig'], causing test pollution; update the
ModuleAdminTest::tearDown() to also unset $GLOBALS['xoopsModule'] and
$GLOBALS['xoopsConfig'] (in addition to $GLOBALS['xoTheme']) so the global state
is fully cleaned between tests.
In `@tests/unit/htdocs/Frameworks/textsanitizer/MyTextSanitizerExtendedTest.php`:
- Around line 92-105: Remove the redundant logger initialization inside the test
method stripSlashesGpcReturnsString in MyTextSanitizerExtendedTest: the
check/assignment of $GLOBALS['xoopsLogger'] is already performed in
setUpBeforeClass(), so delete the if (!isset($GLOBALS['xoopsLogger'])) {
$GLOBALS['xoopsLogger'] = \XoopsLogger::getInstance(); } block from the
stripSlashesGpcReturnsString test, leaving the rest of the test (instantiating
MyTextSanitizerExtended, calling stripSlashesGPC, and the assertions) unchanged.
Comment on lines
+7
to
+10
| use PHPUnit\Framework\Attributes\CoversClass; | ||
| use PHPUnit\Framework\Attributes\DataProvider; | ||
| use PHPUnit\Framework\Attributes\Test; | ||
| use PHPUnit\Framework\TestCase; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and read the relevant sections
cat -n tests/unit/htdocs/class/auth/XoopsAuthProvisionningTest.php | head -100Repository: XOOPS/XoopsCore25
Length of output: 1733
🌐 Web query:
PHPUnit 9.6 attribute support #[Test] #[CoversClass]
💡 Result:
PHPUnit 9.6 does not support PHP 8 attributes such as #[Test] or #[CoversClass]. In 9.6 you must use DocBlock annotations, e.g. /** @test */, /** @Covers \Foo\Bar */, /** @coversDefaultClass ... */, etc. [1]
Attribute-based metadata (including #[Test] and coverage attributes like #[CoversClass]) was introduced for PHPUnit 10; the PHPUnit manual documents these under the PHPUnit\Framework\Attributes namespace. [2] The original tracking issue for adding attribute support was targeted to the PHPUnit 10.0 milestone. [3]
What to do:
- Stay on PHPUnit 9.6 → use
@test,@covers,@coversDefaultClass, etc. [1] - Want
#[Test]/#[CoversClass]→ upgrade to PHPUnit 10.x and usePHPUnit\Framework\Attributes\Test/CoversClass. [2]
Sources: [1] [2] [3]
Replace PHPUnit attributes with docblock annotations for PHP 7.4 and PHPUnit 9.6 compatibility.
PHPUnit 9.6 (required per learnings) does not support attributes—they were introduced in PHPUnit 10. Additionally, attributes are a PHP 8.0+ feature and the codebase must support PHP 7.4. Remove all #[CoversClass], #[Test], and #[DataProvider] attributes and convert to docblock equivalents (@covers, @test, @dataProvider).
🛠️ Example conversion
-use PHPUnit\Framework\Attributes\CoversClass;
-use PHPUnit\Framework\Attributes\DataProvider;
-use PHPUnit\Framework\Attributes\Test;-#[CoversClass(XoopsAuthProvisionning::class)]
+/**
+ * `@covers` \XoopsAuthProvisionning
+ */
class XoopsAuthProvisionningTest extends TestCase- #[Test]
- #[DataProvider('usernameProvider')]
+ /**
+ * `@test`
+ * `@dataProvider` usernameProvider
+ */
public function getXoopsUserReturnsFalseForVariousUsernames(string $uname): void🤖 Prompt for AI Agents
In `@tests/unit/htdocs/class/auth/XoopsAuthProvisionningTest.php` around lines 7 -
10, Replace PHP 8 attributes in the XoopsAuthProvisionningTest class with
PHPUnit 9.6-compatible docblock annotations: remove all occurrences of
#[CoversClass(...)] / #[Test] / #[DataProvider(...)] and instead add
corresponding docblock tags above the test class/methods — use `@covers` for the
class or specific methods, `@test` above methods that were #[Test], and
`@dataProvider` <providerName> above methods that used #[DataProvider]. Ensure the
class name XoopsAuthProvisionningTest keeps the same coverage targets and
provider names when converting attributes to `@covers` and `@dataProvider`
annotations.
Comment on lines
+7
to
+10
| use PHPUnit\Framework\Attributes\CoversClass; | ||
| use PHPUnit\Framework\Attributes\DataProvider; | ||
| use PHPUnit\Framework\Attributes\Test; | ||
| use PHPUnit\Framework\TestCase; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests/unit/htdocs/class/auth/XoopsAuthXoopsTest.php | head -50Repository: XOOPS/XoopsCore25
Length of output: 1733
🌐 Web query:
PHP 8.0 attributes parser support PHP 7.4 compatibility
💡 Result:
- Runtime compatibility: PHP 7.4 cannot execute or reflect PHP 8.0 attributes; the
#[...]syntax is a PHP 8 language feature (no polyfill can make PHP 7.4 “understand” the syntax at runtime). What you can do is treat attributes as tooling-only metadata while still running on 7.4 by writing them in a backward-compatible form (below). - Parsing on PHP 7.4 (static analysis / tooling):
nikic/php-parserruns on PHP >= 7.4 and can parse PHP 7.0–PHP 8.x (incl. attributes) into an AST for analysis/transforms. [1] - Keeping source code loadable on PHP 7.4: If you put one-line attributes on their own line, PHP 7.* will interpret them as a comment line (so no parse error). Multiline attributes are not safe pre-8.0. [3]
- Generating/printing code that won’t break 7.4:
nikic/php-parser’s pretty-printer can print attributes (notably on parameters) on separate lines when targeting PHP 7.4 or older, specifically so they’re treated like comments instead of causing parse errors. [2] - Polyfills:
symfony/polyfill-php80can provide theAttributeclass (and other PHP 8 symbols) on older PHP, but this does not add attribute syntax support to PHP 7.4. [4]
Sources: [1] [2] [3] [4]
🌐 Web query:
PHPUnit 9.6 attributes vs docblocks PHP 7.4
💡 Result:
- PHP 7.4 cannot use PHP attributes (
#[...]) because attributes are a PHP 8.0 language feature. So on PHP 7.4 you must use DocBlock annotations (/**@test*/,/**@dataProvider... */, etc.). [2] - PHPUnit 9.6 is designed around DocBlock annotations (it documents them explicitly in its manual) and is compatible with PHP ≥7.3, so it runs on PHP 7.4. [4] [1]
- PHPUnit attributes (
PHPUnit\Framework\Attributes\...) are a newer approach; PHPUnit’s docs note that before PHPUnit 10, DocBlock annotations were the only way to attach this kind of test metadata, and PHPUnit prefers attributes first when they’re available. Practically, that means you need PHP 8+ (and typically a newer PHPUnit) to use attributes. [3]
So: on PHP 7.4 + PHPUnit 9.6, use DocBlock annotations; to use attributes, you must upgrade to PHP 8+ (and a PHPUnit version that supports them). [2] [3]
Sources: [1] https://phpunit.de/supported-versions.html [2] https://www.php.net/manual/en/language.attributes.overview.php [3] https://docs.phpunit.de/en/11.5/attributes.html [4] https://docs.phpunit.de/en/9.6/
Convert PHPUnit attributes to docblocks for PHP 7.4 compatibility.
Attributes require PHP 8.0+. The imports at lines 7–10 and attribute decorators (lines 16–18) must be replaced with docblock annotations to maintain PHP 7.4 support and compatibility with PHPUnit 9.6, which requires docblocks.
Example conversion
-use PHPUnit\Framework\Attributes\CoversClass;
-use PHPUnit\Framework\Attributes\DataProvider;
-use PHPUnit\Framework\Attributes\Test;
...
-#[CoversClass(XoopsAuthXoops::class)]
+/**
+ * `@covers` \XoopsAuthXoops
+ */
class XoopsAuthXoopsTest extends TestCase- #[Test]
+ /**
+ * `@test`
+ */
public function constructorSetsAuthMethodToXoops(): void🤖 Prompt for AI Agents
In `@tests/unit/htdocs/class/auth/XoopsAuthXoopsTest.php` around lines 7 - 10, The
file imports PHPUnit attribute classes and uses PHP 8 attributes which break PHP
7.4; remove the use lines for PHPUnit\Framework\Attributes\CoversClass,
DataProvider, and Test and keep only use PHPUnit\Framework\TestCase; then
replace each attribute decorator on the XoopsAuthXoopsTest test methods (the
#[CoversClass(...)] / #[DataProvider(...)] / #[Test] blocks) with equivalent
docblock annotations immediately above the method signature (e.g. /** `@covers`
ClassName */ /** `@dataProvider` providerName */ /** `@test` */), ensuring the class
name in `@covers` matches the original CoversClass argument and the provider names
match the original DataProvider arguments so PHPUnit 9.6 on PHP 7.4 will run the
tests.
Comment on lines
+7
to
+13
| use PHPUnit\Framework\Attributes\CoversClass; | ||
| use PHPUnit\Framework\Attributes\Test; | ||
| use PHPUnit\Framework\TestCase; | ||
| use XoopsCacheEngine; | ||
| use XoopsCacheEngineInterface; | ||
|
|
||
| #[CoversClass(XoopsCacheEngine::class)] |
There was a problem hiding this comment.
PHP 8 attributes break PHPUnit 9.6 compatibility.
Same issue as XoopsCacheKeyTest.php — the #[CoversClass] and #[Test] attributes require PHPUnit 10.x+. For cross-version compatibility, use doc-comment annotations or test* method prefixes.
🤖 Prompt for AI Agents
In `@tests/unit/htdocs/class/cache/XoopsCacheEngineTest.php` around lines 7 - 13,
The PHPUnit 10-only attributes (#[CoversClass] and #[Test]) in
XoopsCacheEngineTest break compatibility with PHPUnit 9.6; remove those PHP 8
attributes from the test class and test methods in XoopsCacheEngineTest and
replace them with PHPUnit 9-compatible constructs—either add docblock
annotations like `@covers` XoopsCacheEngine and `@test` above the class/methods that
used #[CoversClass] and #[Test], or keep the annotations off and rename the test
methods to start with test (e.g., testSomething) so PHPUnit 9.6 will discover
them; update references to XoopsCacheEngine and XoopsCacheEngineInterface in the
file accordingly.
Comment on lines
+112
to
+117
| #[Test] | ||
| public function gcDoesNotThrow(): void | ||
| { | ||
| $this->engine->gc(); | ||
| $this->assertTrue(true); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using expectNotToPerformAssertions() for no-op tests.
The assertTrue(true) pattern works but PHPUnit provides $this->expectNotToPerformAssertions() specifically for tests that verify code runs without throwing. Minor style suggestion.
Alternative pattern
#[Test]
public function gcDoesNotThrow(): void
{
+ $this->expectNotToPerformAssertions();
$this->engine->gc();
- $this->assertTrue(true);
}🤖 Prompt for AI Agents
In `@tests/unit/htdocs/class/cache/XoopsCacheEngineTest.php` around lines 112 -
117, Replace the no-op assertion in XoopsCacheEngineTest::gcDoesNotThrow by
using PHPUnit's intent-revealing helper: call
$this->expectNotToPerformAssertions() at the start of gcDoesNotThrow(), then
invoke $this->engine->gc(); so the test verifies the method runs without
throwing instead of using $this->assertTrue(true).
Comment on lines
+334
to
+360
| /** | ||
| * @return array<string, array{string, string}> | ||
| */ | ||
| public static function filenameProvider(): array | ||
| { | ||
| return [ | ||
| 'simple filename' => ['hello.txt', 'hello.txt'], | ||
| 'path with slashes' => ['dir/sub/file.txt', 'dir/sub/file.txt'], | ||
| 'backslash path' => ['dir\\sub\\file.txt', 'dir/sub/file.txt'], | ||
| 'mixed slashes' => ['dir\\sub/file.txt', 'dir/sub/file.txt'], | ||
| 'unicode filename' => ['data_file.txt', 'data_file.txt'], | ||
| 'filename with spaces' => ['my file.txt', 'my file.txt'], | ||
| ]; | ||
| } | ||
|
|
||
| #[DataProvider('filenameProvider')] | ||
| public function testAddFileStoresCorrectFilename(string $input, string $expected): void | ||
| { | ||
| $this->zip->addFile('test', $input); | ||
|
|
||
| $output = $this->zip->file(); | ||
|
|
||
| $this->assertNotFalse( | ||
| strpos($output, $expected), | ||
| "ZIP output should contain normalized filename '{$expected}'" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Well-structured data provider with good filename coverage.
The filenameProvider() covers important normalization cases:
- Simple filenames
- Forward slash paths (unchanged)
- Backslash paths (normalized)
- Mixed slash paths
- Filenames with spaces
One observation: the 'unicode filename' test case uses 'data_file.txt' which doesn't actually contain Unicode characters. If you intend to test Unicode support, consider using actual multi-byte characters:
💡 Suggestion for actual Unicode test
- 'unicode filename' => ['data_file.txt', 'data_file.txt'],
+ 'unicode filename' => ['données_日本語.txt', 'données_日本語.txt'],Note: The #[DataProvider] attribute needs to be converted to @dataProvider annotation as noted earlier.
📝 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
| /** | |
| * @return array<string, array{string, string}> | |
| */ | |
| public static function filenameProvider(): array | |
| { | |
| return [ | |
| 'simple filename' => ['hello.txt', 'hello.txt'], | |
| 'path with slashes' => ['dir/sub/file.txt', 'dir/sub/file.txt'], | |
| 'backslash path' => ['dir\\sub\\file.txt', 'dir/sub/file.txt'], | |
| 'mixed slashes' => ['dir\\sub/file.txt', 'dir/sub/file.txt'], | |
| 'unicode filename' => ['data_file.txt', 'data_file.txt'], | |
| 'filename with spaces' => ['my file.txt', 'my file.txt'], | |
| ]; | |
| } | |
| #[DataProvider('filenameProvider')] | |
| public function testAddFileStoresCorrectFilename(string $input, string $expected): void | |
| { | |
| $this->zip->addFile('test', $input); | |
| $output = $this->zip->file(); | |
| $this->assertNotFalse( | |
| strpos($output, $expected), | |
| "ZIP output should contain normalized filename '{$expected}'" | |
| ); | |
| } | |
| /** | |
| * `@return` array<string, array{string, string}> | |
| */ | |
| public static function filenameProvider(): array | |
| { | |
| return [ | |
| 'simple filename' => ['hello.txt', 'hello.txt'], | |
| 'path with slashes' => ['dir/sub/file.txt', 'dir/sub/file.txt'], | |
| 'backslash path' => ['dir\\sub\\file.txt', 'dir/sub/file.txt'], | |
| 'mixed slashes' => ['dir\\sub/file.txt', 'dir/sub/file.txt'], | |
| 'unicode filename' => ['données_日本語.txt', 'données_日本語.txt'], | |
| 'filename with spaces' => ['my file.txt', 'my file.txt'], | |
| ]; | |
| } | |
| #[DataProvider('filenameProvider')] | |
| public function testAddFileStoresCorrectFilename(string $input, string $expected): void | |
| { | |
| $this->zip->addFile('test', $input); | |
| $output = $this->zip->file(); | |
| $this->assertNotFalse( | |
| strpos($output, $expected), | |
| "ZIP output should contain normalized filename '{$expected}'" | |
| ); | |
| } |
🤖 Prompt for AI Agents
In `@tests/unit/htdocs/class/ZipfileTest.php` around lines 334 - 360, The 'unicode
filename' case in filenameProvider() doesn't contain real multi-byte
characters—replace the value for the 'unicode filename' key with an actual
Unicode string (e.g., include non-ASCII characters) so the test validates
Unicode normalization, and change the test attribute usage from the PHP 8
attribute #[DataProvider('filenameProvider')] to the PHPUnit docblock annotation
`@dataProvider` filenameProvider in the testAddFileStoresCorrectFilename method to
ensure the provider is recognized.
Comment on lines
+7
to
+10
| use PHPUnit\Framework\Attributes\CoversFunction; | ||
| use PHPUnit\Framework\Attributes\Test; | ||
| use PHPUnit\Framework\TestCase; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests/unit/htdocs/Frameworks/art/FunctionsUserTest.phpRepository: XOOPS/XoopsCore25
Length of output: 1733
Remove PHPUnit Attributes imports and replace decorators with @test docblocks.
The imports from PHPUnit\Framework\Attributes violate PHP 7.4 compatibility. Replace any #[Test] and #[CoversFunction] decorators with PHPUnit @test docblocks, or rename methods to follow the test* prefix convention. Remove unused attribute imports.
Example conversion
-use PHPUnit\Framework\Attributes\CoversFunction;
-use PHPUnit\Framework\Attributes\Test;
use PHPUnit\Framework\TestCase;
- #[Test]
+ /**
+ * `@test`
+ */
public function getIpReturnsValue(): void🤖 Prompt for AI Agents
In `@tests/unit/htdocs/Frameworks/art/FunctionsUserTest.php` around lines 7 - 10,
The PHPUnit attribute imports (CoversFunction, Test) in the FunctionsUserTest
class break PHP 7.4 compatibility; remove the lines importing
PHPUnit\Framework\Attributes\CoversFunction and
PHPUnit\Framework\Attributes\Test, delete any #[Test] and #[CoversFunction(...)]
attribute decorators on methods in FunctionsUserTest, and replace them by either
adding an `@test` docblock above each test method or renaming the methods to start
with test (e.g., testSomething) so PHPUnit recognizes them; ensure the class
still extends TestCase and keep any existing assertions unchanged.
Comment on lines
+7
to
+12
| use PHPUnit\Framework\Attributes\CoversClass; | ||
| use PHPUnit\Framework\Attributes\Test; | ||
| use PHPUnit\Framework\TestCase; | ||
| use ModuleAdmin; | ||
|
|
||
| #[CoversClass(ModuleAdmin::class)] |
There was a problem hiding this comment.
PHP 8 attributes break PHP 7.4 and PHPUnit 9.6 compatibility.
The #[CoversClass] and #[Test] attributes are PHP 8.0+ syntax. Per the coding guidelines, code must run on PHP 7.4 through 8.5 and tests must work across PHPUnit 9.6, 10.5, and 11.x. Attributes will cause a parse error on PHP < 8.0.
For cross-version compatibility, use docblock annotations instead:
/**
* `@covers` \ModuleAdmin
*/
class ModuleAdminTest extends TestCaseAnd replace #[Test] with either @test annotations or the conventional test method name prefix (which you're already using).
🔧 Proposed fix for class-level annotation
-use PHPUnit\Framework\Attributes\CoversClass;
-use PHPUnit\Framework\Attributes\Test;
use PHPUnit\Framework\TestCase;
use ModuleAdmin;
-#[CoversClass(ModuleAdmin::class)]
+/**
+ * `@covers` \ModuleAdmin
+ */
class ModuleAdminTest extends TestCaseThen remove all #[Test] attributes throughout the file—the test prefix on method names already signals they are test methods to PHPUnit.
Based on learnings: "Tests must work across PHPUnit 9.6, 10.5, and 11.x" and coding guidelines: "Do not use features exclusive to PHP 8.0+ ... attributes #[...]"
🤖 Prompt for AI Agents
In `@tests/unit/htdocs/Frameworks/moduleclasses/ModuleAdminTest.php` around lines
7 - 12, Replace PHP 8 attributes with docblock annotations to maintain PHP 7.4
and PHPUnit 9.6+ compatibility: remove the #[CoversClass(ModuleAdmin::class)]
and any #[Test] attributes in ModuleAdminTest and add a class-level docblock
with `@covers` \ModuleAdmin above the ModuleAdminTest class declaration, and
convert any #[Test] uses to either `@test` docblock annotations or rely on the
existing test* method name prefix so PHPUnit recognizes the tests.
Comment on lines
+7
to
+8
| use PHPUnit\Framework\Attributes\CoversClass; | ||
| use PHPUnit\Framework\Attributes\DataProvider; |
There was a problem hiding this comment.
PHP 8 attributes break compatibility with PHP 7.4 and PHPUnit 9.6.
The #[CoversClass] and #[DataProvider] attributes are PHP 8.0+ syntax features that will cause parse errors on PHP 7.4. Additionally, these PHPUnit attribute classes were introduced in PHPUnit 10 and do not exist in PHPUnit 9.6.
For cross-version compatibility, use doc-comment annotations instead:
/**
* `@covers` \XoopsAuthAds
*/
class XoopsAuthAdsTest extends TestCase
/**
* `@dataProvider` upnDataProvider
*/
public function testGetUpnWithDataProvider(...)As per coding guidelines: "Code must run on PHP 7.4 through 8.5. Do not use features exclusive to PHP 8.0+ ... attributes #[...]" and "Tests must work across PHPUnit 9.6, 10.5, and 11.x".
Also applies to: 34-34, 210-210
🤖 Prompt for AI Agents
In `@tests/unit/htdocs/class/auth/XoopsAuthAdsTest.php` around lines 7 - 8, Remove
PHP 8 attribute usage and PHPUnit 10-only imports in XoopsAuthAdsTest: delete
the use statements for PHPUnit\Framework\Attributes\CoversClass and
PHPUnit\Framework\Attributes\DataProvider and replace any #[CoversClass(...)]
and #[DataProvider(...)] attribute annotations with equivalent docblock
annotations (e.g., /** `@covers` \XoopsAuthAds */ above the class declaration and
/** `@dataProvider` upnDataProvider */ above the test method). Ensure you update
all occurrences referenced (including the other instances around the indicated
spots) so the class and test methods use only docblock `@covers` and `@dataProvider`
annotations compatible with PHP 7.4 and PHPUnit 9.6–11.x.
Comment on lines
+323
to
+330
| public function testInheritedGetHtmlErrors(): void | ||
| { | ||
| $GLOBALS['xoopsConfig'] = ['debug_mode' => 1]; | ||
| $this->ads->setErrors(0, 'ADS Error'); | ||
| $html = $this->ads->getHtmlErrors(); | ||
| $this->assertStringContainsString('ADS Error', $html); | ||
| unset($GLOBALS['xoopsConfig']); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good cleanup, but consider using try/finally for robustness.
The unset($GLOBALS['xoopsConfig']) on line 329 ensures cleanup, but if the test fails before reaching that line, the global remains polluted for subsequent tests.
♻️ Suggested improvement for guaranteed cleanup
public function testInheritedGetHtmlErrors(): void
{
$GLOBALS['xoopsConfig'] = ['debug_mode' => 1];
- $this->ads->setErrors(0, 'ADS Error');
- $html = $this->ads->getHtmlErrors();
- $this->assertStringContainsString('ADS Error', $html);
- unset($GLOBALS['xoopsConfig']);
+ try {
+ $this->ads->setErrors(0, 'ADS Error');
+ $html = $this->ads->getHtmlErrors();
+ $this->assertStringContainsString('ADS Error', $html);
+ } finally {
+ unset($GLOBALS['xoopsConfig']);
+ }
}🧰 Tools
🪛 PHPMD (2.15.0)
[error] 323-330: testInheritedGetHtmlErrors accesses the super-global variable $GLOBALS. (undefined)
(Superglobals)
[error] 323-330: testInheritedGetHtmlErrors accesses the super-global variable $GLOBALS. (undefined)
(Superglobals)
🤖 Prompt for AI Agents
In `@tests/unit/htdocs/class/auth/XoopsAuthAdsTest.php` around lines 323 - 330,
Wrap the global config setup/teardown in testInheritedGetHtmlErrors in a
try/finally so unset($GLOBALS['xoopsConfig']) always runs: set
$GLOBALS['xoopsConfig'] = ['debug_mode' => 1] before the try, run the existing
test assertions inside the try block, and move the
unset($GLOBALS['xoopsConfig']) into the finally block to guarantee cleanup even
on failure.
Comment on lines
+367
to
+385
| public function testHasGetUpnMethod(): void | ||
| { | ||
| $this->assertTrue(method_exists($this->ads, 'getUPN')); | ||
| } | ||
|
|
||
| public function testHasAuthenticateMethod(): void | ||
| { | ||
| $this->assertTrue(method_exists($this->ads, 'authenticate')); | ||
| } | ||
|
|
||
| public function testHasGetUserDNMethodInherited(): void | ||
| { | ||
| $this->assertTrue(method_exists($this->ads, 'getUserDN')); | ||
| } | ||
|
|
||
| public function testHasLoadXoopsUserMethodInherited(): void | ||
| { | ||
| $this->assertTrue(method_exists($this->ads, 'loadXoopsUser')); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Method existence tests add limited value.
These method_exists() checks will always pass if the class loads successfully, since the methods are defined in the class hierarchy. They primarily guard against typos in method names during refactoring but don't test actual behavior.
Consider whether these provide sufficient value or if the behavioral tests (lines 301-330) already provide adequate coverage of inherited method functionality.
🤖 Prompt for AI Agents
In `@tests/unit/htdocs/class/auth/XoopsAuthAdsTest.php` around lines 367 - 385,
The four trivial method-existence tests (testHasGetUpnMethod,
testHasAuthenticateMethod, testHasGetUserDNMethodInherited,
testHasLoadXoopsUserMethodInherited) are redundant; remove them or replace each
with a small behavioral assertion that exercises the actual method (e.g., call
getUPN/getUserDN/authenticate/loadXoopsUser with a minimal/mock input and assert
expected output/side-effect) so coverage verifies behavior rather than mere
presence—locate these methods in class XoopsAuthAdsTest and either delete those
test methods or change their bodies to invoke the corresponding methods and
assert a real outcome.
Comment on lines
+1
to
+17
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace xoopsauth; | ||
|
|
||
| use PHPUnit\Framework\Attributes\CoversClass; | ||
| use PHPUnit\Framework\Attributes\DataProvider; | ||
| use PHPUnit\Framework\Attributes\Test; | ||
| use PHPUnit\Framework\TestCase; | ||
| use ReflectionClass; | ||
| use ReflectionMethod; | ||
| use XoopsAuth; | ||
| use XoopsAuthFactory; | ||
|
|
||
| #[CoversClass(XoopsAuthFactory::class)] | ||
| class XoopsAuthFactoryTest extends TestCase |
There was a problem hiding this comment.
Missing XOOPS header and required class docblock metadata.
Please add the standard XOOPS copyright header and a class docblock with the required tags.
As per coding guidelines, "Every source file begins with the XOOPS copyright header block" and "Class docblocks include @category, @package, @author, @copyright, @license, and @link tags."
🤖 Prompt for AI Agents
In `@tests/unit/htdocs/class/auth/XoopsAuthFactoryTest.php` around lines 1 - 17,
Add the standard XOOPS file header to the top of the file and a class docblock
for the XoopsAuthFactoryTest class: insert the XOOPS copyright/header block
before the namespace declaration and add a docblock immediately above the
XoopsAuthFactoryTest class declaration that includes the required tags
`@category`, `@package`, `@author`, `@copyright`, `@license`, and `@link`; ensure the
docblock references the class name XoopsAuthFactoryTest and matches XOOPS
formatting guidelines.
Comment on lines
+7
to
+114
| use PHPUnit\Framework\Attributes\CoversClass; | ||
| use PHPUnit\Framework\Attributes\DataProvider; | ||
| use PHPUnit\Framework\Attributes\Test; | ||
| use PHPUnit\Framework\TestCase; | ||
| use ReflectionClass; | ||
| use ReflectionMethod; | ||
| use XoopsAuth; | ||
| use XoopsAuthFactory; | ||
|
|
||
| #[CoversClass(XoopsAuthFactory::class)] | ||
| class XoopsAuthFactoryTest extends TestCase | ||
| { | ||
| protected function setUp(): void | ||
| { | ||
| require_once XOOPS_ROOT_PATH . '/class/auth/auth.php'; | ||
| require_once XOOPS_ROOT_PATH . '/class/auth/authfactory.php'; | ||
|
|
||
| // Ensure xoopsLogger is available (factory uses $GLOBALS['xoopsLogger']) | ||
| if (!isset($GLOBALS['xoopsLogger'])) { | ||
| $GLOBALS['xoopsLogger'] = \XoopsLogger::getInstance(); | ||
| } | ||
| } | ||
|
|
||
| // --------------------------------------------------------------- | ||
| // Class structure tests | ||
| // --------------------------------------------------------------- | ||
|
|
||
| #[Test] | ||
| public function classExists(): void | ||
| { | ||
| $this->assertTrue(class_exists('XoopsAuthFactory')); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function classIsNotAbstract(): void | ||
| { | ||
| $ref = new ReflectionClass(XoopsAuthFactory::class); | ||
| $this->assertFalse($ref->isAbstract()); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function classIsNotFinal(): void | ||
| { | ||
| $ref = new ReflectionClass(XoopsAuthFactory::class); | ||
| $this->assertFalse($ref->isFinal()); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function classHasGetAuthConnectionMethod(): void | ||
| { | ||
| $ref = new ReflectionClass(XoopsAuthFactory::class); | ||
| $this->assertTrue($ref->hasMethod('getAuthConnection')); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function getAuthConnectionMethodIsStatic(): void | ||
| { | ||
| $ref = new ReflectionClass(XoopsAuthFactory::class); | ||
| $method = $ref->getMethod('getAuthConnection'); | ||
| $this->assertTrue($method->isStatic()); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function getAuthConnectionMethodIsPublic(): void | ||
| { | ||
| $ref = new ReflectionClass(XoopsAuthFactory::class); | ||
| $method = $ref->getMethod('getAuthConnection'); | ||
| $this->assertTrue($method->isPublic()); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function getAuthConnectionAcceptsOneParameter(): void | ||
| { | ||
| $ref = new ReflectionClass(XoopsAuthFactory::class); | ||
| $method = $ref->getMethod('getAuthConnection'); | ||
| $params = $method->getParameters(); | ||
| $this->assertCount(1, $params); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function getAuthConnectionParameterNameIsUname(): void | ||
| { | ||
| $ref = new ReflectionClass(XoopsAuthFactory::class); | ||
| $method = $ref->getMethod('getAuthConnection'); | ||
| $params = $method->getParameters(); | ||
| $this->assertSame('uname', $params[0]->getName()); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function getAuthConnectionParameterIsRequired(): void | ||
| { | ||
| $ref = new ReflectionClass(XoopsAuthFactory::class); | ||
| $method = $ref->getMethod('getAuthConnection'); | ||
| $params = $method->getParameters(); | ||
| $this->assertFalse($params[0]->isOptional()); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function classHasOnlyOnePublicMethod(): void | ||
| { | ||
| $ref = new ReflectionClass(XoopsAuthFactory::class); | ||
| $publicMethods = $ref->getMethods(ReflectionMethod::IS_PUBLIC); | ||
| // Filter to only methods declared on XoopsAuthFactory itself | ||
| $declared = array_filter($publicMethods, function ($m) { | ||
| return $m->getDeclaringClass()->getName() === 'XoopsAuthFactory'; | ||
| }); | ||
| $this->assertCount(1, $declared); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests/unit/htdocs/class/auth/XoopsAuthFactoryTest.php | head -120Repository: XOOPS/XoopsCore25
Length of output: 1733
🏁 Script executed:
# Check for all PHP 8 attribute usage (lines with #[)
rg -n "^\s*#\[" tests/unit/htdocs/class/auth/XoopsAuthFactoryTest.phpRepository: XOOPS/XoopsCore25
Length of output: 1733
🏁 Script executed:
# Check test method names - should start with 'test' followed by uppercase letter
rg -n "public function (class|get)" tests/unit/htdocs/class/auth/XoopsAuthFactoryTest.phpRepository: XOOPS/XoopsCore25
Length of output: 1733
Remove PHP 8 attributes and rename test methods to test{MethodName} convention.
This test file uses PHP 8-only attributes (#[CoversClass], #[DataProvider], #[Test]) which must be replaced with PHPDoc metadata for PHP 7.4 compatibility. Additionally, test method names must follow the test{MethodName} pattern per the guidelines.
Suggested pattern (apply across this class)
-use PHPUnit\Framework\Attributes\CoversClass;
-use PHPUnit\Framework\Attributes\DataProvider;
-use PHPUnit\Framework\Attributes\Test;
-
-#[CoversClass(XoopsAuthFactory::class)]
+/**
+ * `@coversDefaultClass` \XoopsAuthFactory
+ */
class XoopsAuthFactoryTest extends TestCase
{
- #[Test]
- public function classExists(): void
+ public function testClassExists(): void
{
$this->assertTrue(class_exists('XoopsAuthFactory'));
}
- #[Test]
- public function classIsNotAbstract(): void
+ public function testClassIsNotAbstract(): void
{
$ref = new ReflectionClass(XoopsAuthFactory::class);
$this->assertFalse($ref->isAbstract());
}🧰 Tools
🪛 PHPMD (2.15.0)
[error] 19-28: setUp accesses the super-global variable $GLOBALS. (undefined)
(Superglobals)
[error] 19-28: setUp accesses the super-global variable $GLOBALS. (undefined)
(Superglobals)
[error] 26-26: Avoid using static access to class '\XoopsLogger' in method 'setUp'. (undefined)
(StaticAccess)
[warning] 110-110: Avoid variables with short names like $m. Configured minimum length is 3. (undefined)
(ShortVariable)
🤖 Prompt for AI Agents
In `@tests/unit/htdocs/class/auth/XoopsAuthFactoryTest.php` around lines 7 - 114,
The file XoopsAuthFactoryTest uses PHP 8 attributes and non-standard test method
names; remove all attribute imports (PHPUnit\Framework\Attributes\CoversClass,
DataProvider, Test) and delete every attribute usage (e.g.
#[CoversClass(XoopsAuthFactory::class)] and each #[Test]/#[DataProvider]) and
replace class-level coverage with a PHPDoc `@covers` XoopsAuthFactory (above class
XoopsAuthFactoryTest). Rename each test method to the PHPUnit 7.4 convention by
prefixing with "test" (e.g. classExists -> testClassExists, classIsNotAbstract
-> testClassIsNotAbstract, classIsNotFinal -> testClassIsNotFinal,
classHasGetAuthConnectionMethod -> testClassHasGetAuthConnectionMethod,
getAuthConnectionMethodIsStatic -> testGetAuthConnectionMethodIsStatic,
getAuthConnectionMethodIsPublic -> testGetAuthConnectionMethodIsPublic,
getAuthConnectionAcceptsOneParameter ->
testGetAuthConnectionAcceptsOneParameter, getAuthConnectionParameterNameIsUname
-> testGetAuthConnectionParameterNameIsUname,
getAuthConnectionParameterIsRequired ->
testGetAuthConnectionParameterIsRequired, classHasOnlyOnePublicMethod ->
testClassHasOnlyOnePublicMethod) and ensure any `@dataProvider` annotations (if
used) are added as PHPDoc above the method.
Comment on lines
+52
to
+1127
| #[CoversClass(XoopsFileHandler::class)] | ||
| class XoopsFileHandlerTest extends TestCase | ||
| { | ||
| /** | ||
| * Temporary directory for all test files. | ||
| */ | ||
| private string $tempDir; | ||
|
|
||
| /** | ||
| * Path to the primary temporary test file. | ||
| */ | ||
| private string $tempFile; | ||
|
|
||
| /** | ||
| * Additional temporary files to clean up. | ||
| * | ||
| * @var string[] | ||
| */ | ||
| private array $additionalFiles = []; | ||
|
|
||
| /** | ||
| * Additional temporary directories to clean up. | ||
| * | ||
| * @var string[] | ||
| */ | ||
| private array $additionalDirs = []; | ||
|
|
||
| /** | ||
| * Creates a temp directory and a test file with known content before each test. | ||
| */ | ||
| protected function setUp(): void | ||
| { | ||
| // Ensure the handler classes are loaded | ||
| XoopsFile::load('file'); | ||
| XoopsFile::load('folder'); | ||
|
|
||
| $this->tempDir = sys_get_temp_dir() . '/xoops_file_test_' . uniqid('', true); | ||
| // Normalize to forward slashes so XoopsFolderHandler::isAbsolute() works on Windows | ||
| $this->tempDir = str_replace('\\', '/', $this->tempDir); | ||
| mkdir($this->tempDir, 0755, true); | ||
| $this->tempFile = $this->tempDir . '/testfile.txt'; | ||
| file_put_contents($this->tempFile, 'Hello World'); | ||
| } | ||
|
|
||
| /** | ||
| * Recursively removes temp directory and all its contents after each test. | ||
| */ | ||
| protected function tearDown(): void | ||
| { | ||
| // Clean additional tracked files | ||
| foreach ($this->additionalFiles as $file) { | ||
| if (is_file($file)) { | ||
| @unlink($file); | ||
| } | ||
| } | ||
|
|
||
| // Clean primary temp dir recursively | ||
| $this->removeDirectory($this->tempDir); | ||
|
|
||
| // Clean additional tracked directories | ||
| foreach (array_reverse($this->additionalDirs) as $dir) { | ||
| $this->removeDirectory($dir); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Recursively removes a directory and all its contents. | ||
| */ | ||
| private function removeDirectory(string $dir): void | ||
| { | ||
| if (!is_dir($dir)) { | ||
| return; | ||
| } | ||
| $items = scandir($dir); | ||
| if ($items === false) { | ||
| return; | ||
| } | ||
| foreach ($items as $item) { | ||
| if ($item === '.' || $item === '..') { | ||
| continue; | ||
| } | ||
| $path = $dir . '/' . $item; | ||
| if (is_dir($path)) { | ||
| $this->removeDirectory($path); | ||
| } else { | ||
| @unlink($path); | ||
| } | ||
| } | ||
| @rmdir($dir); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------- | ||
| // Constructor tests | ||
| // --------------------------------------------------------------- | ||
|
|
||
| /** | ||
| * Constructor with an existing file sets the name and folder properties. | ||
| */ | ||
| public function testConstructorWithExistingFileSetsNameAndFolder(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
|
|
||
| $this->assertSame('testfile.txt', $handler->name); | ||
| $this->assertInstanceOf(XoopsFolderHandler::class, $handler->folder); | ||
| } | ||
|
|
||
| /** | ||
| * Constructor with a non-existent file and create=true creates the file. | ||
| */ | ||
| public function testConstructorWithCreateTrueCreatesFile(): void | ||
| { | ||
| $newFile = $this->tempDir . '/created_by_ctor.txt'; | ||
| $this->additionalFiles[] = $newFile; | ||
|
|
||
| $handler = new XoopsFileHandler($newFile, true); | ||
|
|
||
| $this->assertFileExists($newFile); | ||
| $this->assertSame('created_by_ctor.txt', $handler->name); | ||
| } | ||
|
|
||
| /** | ||
| * Constructor with a non-existent file and create=false does not create the file. | ||
| */ | ||
| public function testConstructorWithCreateFalseDoesNotCreateFile(): void | ||
| { | ||
| $newFile = $this->tempDir . '/not_created.txt'; | ||
|
|
||
| $handler = new XoopsFileHandler($newFile, false); | ||
|
|
||
| $this->assertFileDoesNotExist($newFile); | ||
| $this->assertSame('not_created.txt', $handler->name); | ||
| } | ||
|
|
||
| /** | ||
| * Constructor sets the folder property to a XoopsFolderHandler for the parent directory. | ||
| */ | ||
| public function testConstructorSetsFolderToParentDirectory(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
|
|
||
| $folderPath = $handler->folder->pwd(); | ||
| // Normalize directory separators for comparison | ||
| $expected = str_replace('\\', '/', $this->tempDir); | ||
| $actual = str_replace('\\', '/', $folderPath); | ||
| $this->assertSame($expected, $actual); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------- | ||
| // read() tests | ||
| // --------------------------------------------------------------- | ||
|
|
||
| /** | ||
| * read() returns file contents as a string. | ||
| */ | ||
| public function testReadReturnsFileContents(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
|
|
||
| $contents = $handler->read(); | ||
| $this->assertSame('Hello World', $contents); | ||
| } | ||
|
|
||
| /** | ||
| * read(false) uses file_get_contents internally. | ||
| */ | ||
| public function testReadFalseUsesFileGetContents(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
|
|
||
| $contents = $handler->read(false); | ||
| $this->assertSame('Hello World', $contents); | ||
| } | ||
|
|
||
| /** | ||
| * read(int) reads exactly N bytes with fread. | ||
| */ | ||
| public function testReadIntReadsNBytes(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
|
|
||
| $contents = $handler->read(5, 'rb', true); | ||
| $this->assertSame('Hello', $contents); | ||
| } | ||
|
|
||
| /** | ||
| * read() on an empty file returns an empty string. | ||
| */ | ||
| public function testReadEmptyFileReturnsEmptyString(): void | ||
| { | ||
| $emptyFile = $this->tempDir . '/empty.txt'; | ||
| file_put_contents($emptyFile, ''); | ||
|
|
||
| $handler = new XoopsFileHandler($emptyFile); | ||
| $contents = $handler->read(); | ||
|
|
||
| $this->assertSame('', $contents); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------- | ||
| // write() tests | ||
| // --------------------------------------------------------------- | ||
|
|
||
| /** | ||
| * write() writes data to the file and returns true. | ||
| */ | ||
| public function testWriteWritesDataToFile(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $result = $handler->write('New Content'); | ||
|
|
||
| $this->assertTrue($result); | ||
| $handler->close(); | ||
|
|
||
| $this->assertSame('New Content', file_get_contents($this->tempFile)); | ||
| } | ||
|
|
||
| /** | ||
| * write() overwrites existing content by default (mode 'w'). | ||
| */ | ||
| public function testWriteOverwritesExistingContent(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $handler->write('Overwritten'); | ||
| $handler->close(); | ||
|
|
||
| $this->assertSame('Overwritten', file_get_contents($this->tempFile)); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------- | ||
| // append() tests | ||
| // --------------------------------------------------------------- | ||
|
|
||
| /** | ||
| * append() appends data to the existing file content. | ||
| */ | ||
| public function testAppendAppendsDataToFile(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $result = $handler->append(' Appended'); | ||
|
|
||
| $this->assertTrue($result); | ||
| $handler->close(); | ||
|
|
||
| $this->assertSame('Hello World Appended', file_get_contents($this->tempFile)); | ||
| } | ||
|
|
||
| /** | ||
| * Multiple append calls accumulate content. | ||
| */ | ||
| public function testAppendMultipleTimes(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $handler->append(' One'); | ||
| $handler->close(); | ||
|
|
||
| $handler2 = new XoopsFileHandler($this->tempFile); | ||
| $handler2->append(' Two'); | ||
| $handler2->close(); | ||
|
|
||
| $this->assertSame('Hello World One Two', file_get_contents($this->tempFile)); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------- | ||
| // open() and close() tests | ||
| // --------------------------------------------------------------- | ||
|
|
||
| /** | ||
| * open() returns true when a file is successfully opened. | ||
| */ | ||
| public function testOpenReturnsTrueOnSuccess(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $result = $handler->open('r'); | ||
|
|
||
| $this->assertTrue($result); | ||
| $handler->close(); | ||
| } | ||
|
|
||
| /** | ||
| * close() returns true on a previously opened handle. | ||
| */ | ||
| public function testCloseReturnsTrueAfterOpen(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $handler->open('r'); | ||
|
|
||
| $result = $handler->close(); | ||
| $this->assertTrue($result); | ||
| } | ||
|
|
||
| /** | ||
| * close() returns true when called on an already-closed (no handle) handler. | ||
| */ | ||
| public function testCloseReturnsTrueWhenNoHandleOpen(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
|
|
||
| $result = $handler->close(); | ||
| $this->assertTrue($result); | ||
| } | ||
|
|
||
| /** | ||
| * open() without force does not re-open an already opened handle. | ||
| */ | ||
| public function testOpenWithoutForceDoesNotReopen(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $handler->open('r'); | ||
| $handle1 = $handler->handle; | ||
|
|
||
| $handler->open('r', false); | ||
| $handle2 = $handler->handle; | ||
|
|
||
| // Same resource, not re-opened | ||
| $this->assertSame($handle1, $handle2); | ||
| $handler->close(); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------- | ||
| // delete() tests | ||
| // --------------------------------------------------------------- | ||
|
|
||
| /** | ||
| * delete() removes the file and returns true. | ||
| */ | ||
| public function testDeleteRemovesFile(): void | ||
| { | ||
| $deleteFile = $this->tempDir . '/to_delete.txt'; | ||
| file_put_contents($deleteFile, 'delete me'); | ||
|
|
||
| $handler = new XoopsFileHandler($deleteFile); | ||
| $result = $handler->delete(); | ||
|
|
||
| $this->assertTrue($result); | ||
| $this->assertFileDoesNotExist($deleteFile); | ||
| } | ||
|
|
||
| /** | ||
| * exists() returns true for an existing file. | ||
| */ | ||
| public function testExistsReturnsTrueForExistingFile(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $this->assertTrue($handler->exists()); | ||
| } | ||
|
|
||
| /** | ||
| * exists() returns false after the file has been deleted. | ||
| */ | ||
| public function testExistsReturnsFalseAfterDelete(): void | ||
| { | ||
| $deleteFile = $this->tempDir . '/exists_test.txt'; | ||
| file_put_contents($deleteFile, 'test'); | ||
|
|
||
| $handler = new XoopsFileHandler($deleteFile); | ||
| $handler->delete(); | ||
|
|
||
| $this->assertFalse($handler->exists()); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------- | ||
| // pwd() tests | ||
| // --------------------------------------------------------------- | ||
|
|
||
| /** | ||
| * pwd() returns the full path to the file. | ||
| */ | ||
| public function testPwdReturnsFullPath(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $pwd = $handler->pwd(); | ||
|
|
||
| // Normalize separators for cross-platform comparison | ||
| $expected = str_replace('\\', '/', $this->tempFile); | ||
| $actual = str_replace('\\', '/', $pwd); | ||
| $this->assertSame($expected, $actual); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------- | ||
| // info() tests | ||
| // --------------------------------------------------------------- | ||
|
|
||
| /** | ||
| * info() returns a pathinfo array with all standard keys. | ||
| */ | ||
| public function testInfoReturnsPathinfoArray(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $info = $handler->info(); | ||
|
|
||
| $this->assertIsArray($info); | ||
| $this->assertArrayHasKey('dirname', $info); | ||
| $this->assertArrayHasKey('basename', $info); | ||
| $this->assertArrayHasKey('extension', $info); | ||
| $this->assertArrayHasKey('filename', $info); | ||
| } | ||
|
|
||
| /** | ||
| * info() returns correct extension value. | ||
| */ | ||
| public function testInfoExtensionIsCorrect(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $info = $handler->info(); | ||
|
|
||
| $this->assertSame('txt', $info['extension']); | ||
| } | ||
|
|
||
| /** | ||
| * info() returns correct basename. | ||
| */ | ||
| public function testInfoBasenameIsCorrect(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $info = $handler->info(); | ||
|
|
||
| $this->assertSame('testfile.txt', $info['basename']); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------- | ||
| // ext() tests | ||
| // --------------------------------------------------------------- | ||
|
|
||
| /** | ||
| * ext() returns the file extension. | ||
| */ | ||
| public function testExtReturnsFileExtension(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $this->assertSame('txt', $handler->ext()); | ||
| } | ||
|
|
||
| /** | ||
| * ext() returns the correct extension for a .php file. | ||
| */ | ||
| public function testExtReturnsPhpExtension(): void | ||
| { | ||
| $phpFile = $this->tempDir . '/script.php'; | ||
| file_put_contents($phpFile, '<?php'); | ||
|
|
||
| $handler = new XoopsFileHandler($phpFile); | ||
| $this->assertSame('php', $handler->ext()); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------- | ||
| // name() tests | ||
| // --------------------------------------------------------------- | ||
|
|
||
| /** | ||
| * name() returns filename without extension. | ||
| */ | ||
| public function testNameReturnsFilenameWithoutExtension(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $this->assertSame('testfile', $handler->name()); | ||
| } | ||
|
|
||
| /** | ||
| * name() handles multi-dot filenames correctly. | ||
| */ | ||
| public function testNameWithMultipleDots(): void | ||
| { | ||
| $dotFile = $this->tempDir . '/my.config.json'; | ||
| file_put_contents($dotFile, '{}'); | ||
|
|
||
| $handler = new XoopsFileHandler($dotFile); | ||
| $this->assertSame('my.config', $handler->name()); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------- | ||
| // safe() tests | ||
| // --------------------------------------------------------------- | ||
|
|
||
| /** | ||
| * safe() replaces non-word characters with underscores. | ||
| */ | ||
| public function testSafeReplacesUnsafeCharacters(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
|
|
||
| $result = $handler->safe('file name (copy).txt', '.txt'); | ||
| $this->assertSame('file_name_copy_', $result); | ||
| } | ||
|
|
||
| /** | ||
| * safe() with no arguments uses the handler's own name. | ||
| */ | ||
| public function testSafeUsesHandlerName(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $result = $handler->safe(); | ||
|
|
||
| // 'testfile.txt' with ext 'txt' => safe('testfile.txt', 'txt') | ||
| // basename('testfile.txt', 'txt') => 'testfile.' => preg_replace => 'testfile_' | ||
| $this->assertIsString($result); | ||
| $this->assertNotEmpty($result); | ||
| } | ||
|
|
||
| /** | ||
| * safe() with a custom name parameter. | ||
| */ | ||
| public function testSafeWithCustomNameParameter(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $result = $handler->safe('my-file_v2.txt', '.txt'); | ||
|
|
||
| // 'my-file_v2' after basename removal, then preg_replace /[^\w\.-]+/ => '_' | ||
| $this->assertSame('my-file_v2', $result); | ||
| } | ||
|
|
||
| /** | ||
| * safe() preserves already-safe characters. | ||
| */ | ||
| public function testSafePreservesSafeCharacters(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $result = $handler->safe('safe_file-name.log', '.log'); | ||
|
|
||
| $this->assertSame('safe_file-name', $result); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------- | ||
| // size() tests | ||
| // --------------------------------------------------------------- | ||
|
|
||
| /** | ||
| * size() returns the file size in bytes. | ||
| */ | ||
| public function testSizeReturnsFileSizeInBytes(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $size = $handler->size(); | ||
|
|
||
| $this->assertSame(strlen('Hello World'), $size); | ||
| } | ||
|
|
||
| /** | ||
| * size() returns 0 for an empty file. | ||
| */ | ||
| public function testSizeReturnsZeroForEmptyFile(): void | ||
| { | ||
| $emptyFile = $this->tempDir . '/empty_size.txt'; | ||
| file_put_contents($emptyFile, ''); | ||
|
|
||
| $handler = new XoopsFileHandler($emptyFile); | ||
| // clear stat cache so size reflects current state | ||
| clearstatcache(true, $emptyFile); | ||
| $this->assertSame(0, $handler->size()); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------- | ||
| // perms() tests | ||
| // --------------------------------------------------------------- | ||
|
|
||
| /** | ||
| * perms() returns a permission string for an existing file. | ||
| */ | ||
| public function testPermsReturnsPermissionString(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $perms = $handler->perms(); | ||
|
|
||
| $this->assertIsString($perms); | ||
| $this->assertSame(4, strlen($perms), 'Permissions should be a 4-character octal string'); | ||
| } | ||
|
|
||
| /** | ||
| * perms() returns false for a non-existent file. | ||
| */ | ||
| public function testPermsReturnsFalseForNonExistentFile(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempDir . '/nonexistent.txt', false); | ||
| $this->assertFalse($handler->perms()); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------- | ||
| // writable() / readable() / executable() tests | ||
| // --------------------------------------------------------------- | ||
|
|
||
| /** | ||
| * writable() returns true for a writable file. | ||
| */ | ||
| public function testWritableReturnsTrueForWritableFile(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $this->assertTrue($handler->writable()); | ||
| } | ||
|
|
||
| /** | ||
| * readable() returns true for a readable file. | ||
| */ | ||
| public function testReadableReturnsTrueForReadableFile(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $this->assertTrue($handler->readable()); | ||
| } | ||
|
|
||
| /** | ||
| * executable() returns a boolean value. | ||
| */ | ||
| public function testExecutableReturnsBoolean(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $this->assertIsBool($handler->executable()); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------- | ||
| // owner() and group() tests | ||
| // --------------------------------------------------------------- | ||
|
|
||
| /** | ||
| * owner() returns the file owner as an integer. | ||
| */ | ||
| public function testOwnerReturnsInteger(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $owner = $handler->owner(); | ||
|
|
||
| $this->assertIsInt($owner); | ||
| } | ||
|
|
||
| /** | ||
| * owner() returns false for a non-existent file. | ||
| */ | ||
| public function testOwnerReturnsFalseForNonExistentFile(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempDir . '/no_owner.txt', false); | ||
| $this->assertFalse($handler->owner()); | ||
| } | ||
|
|
||
| /** | ||
| * group() returns the file group as an integer. | ||
| */ | ||
| public function testGroupReturnsInteger(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $group = $handler->group(); | ||
|
|
||
| $this->assertIsInt($group); | ||
| } | ||
|
|
||
| /** | ||
| * group() returns false for a non-existent file. | ||
| */ | ||
| public function testGroupReturnsFalseForNonExistentFile(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempDir . '/no_group.txt', false); | ||
| $this->assertFalse($handler->group()); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------- | ||
| // lastAccess() and lastChange() tests | ||
| // --------------------------------------------------------------- | ||
|
|
||
| /** | ||
| * lastAccess() returns a timestamp (integer) for an existing file. | ||
| */ | ||
| public function testLastAccessReturnsTimestamp(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $ts = $handler->lastAccess(); | ||
|
|
||
| $this->assertIsInt($ts); | ||
| $this->assertGreaterThan(0, $ts); | ||
| } | ||
|
|
||
| /** | ||
| * lastAccess() returns false for a non-existent file. | ||
| */ | ||
| public function testLastAccessReturnsFalseForNonExistentFile(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempDir . '/no_access.txt', false); | ||
| $this->assertFalse($handler->lastAccess()); | ||
| } | ||
|
|
||
| /** | ||
| * lastChange() returns a timestamp (integer) for an existing file. | ||
| */ | ||
| public function testLastChangeReturnsTimestamp(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $ts = $handler->lastChange(); | ||
|
|
||
| $this->assertIsInt($ts); | ||
| $this->assertGreaterThan(0, $ts); | ||
| } | ||
|
|
||
| /** | ||
| * lastChange() returns false for a non-existent file. | ||
| */ | ||
| public function testLastChangeReturnsFalseForNonExistentFile(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempDir . '/no_change.txt', false); | ||
| $this->assertFalse($handler->lastChange()); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------- | ||
| // md5() tests | ||
| // --------------------------------------------------------------- | ||
|
|
||
| /** | ||
| * md5(true) forces checksum calculation regardless of file size. | ||
| */ | ||
| public function testMd5TrueReturnsChecksumRegardlessOfSize(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $checksum = $handler->md5(true); | ||
|
|
||
| $this->assertIsString($checksum); | ||
| $this->assertSame(32, strlen($checksum), 'MD5 hash should be 32 hex characters'); | ||
| $this->assertSame(md5_file($this->tempFile), $checksum); | ||
| } | ||
|
|
||
| /** | ||
| * md5() returns checksum for small files (below default maxsize). | ||
| */ | ||
| public function testMd5ReturnsChecksumForSmallFiles(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $checksum = $handler->md5(); | ||
|
|
||
| $this->assertIsString($checksum); | ||
| $this->assertSame(md5('Hello World'), $checksum); | ||
| } | ||
|
|
||
| /** | ||
| * md5() with a very small maxsize returns false for files larger than maxsize. | ||
| * | ||
| * The maxsize is in MB. We create a file just over the threshold to test. | ||
| * Using maxsize of 0 means threshold is 0 bytes, so any non-empty file fails. | ||
| */ | ||
| public function testMd5ReturnsFalseForFilesLargerThanMaxsize(): void | ||
| { | ||
| // Create a file with some content | ||
| $largeFile = $this->tempDir . '/large.bin'; | ||
| file_put_contents($largeFile, str_repeat('X', 100)); | ||
|
|
||
| $handler = new XoopsFileHandler($largeFile); | ||
|
|
||
| // maxsize=0 => threshold is 0 * 1024 * 1024 = 0 bytes | ||
| // The condition is: $size < ($maxsize * 1024) * 1024 | ||
| // With maxsize=0: $size < 0 is always false for non-empty files | ||
| // But $size could be 0 for empty files. Let's use a value that's clearly under. | ||
| // Actually, md5(0) => $size < 0 always false, so returns false. | ||
| $result = $handler->md5(0); | ||
|
|
||
| $this->assertFalse($result); | ||
| } | ||
|
|
||
| /** | ||
| * md5() returns the correct hash value for known content. | ||
| */ | ||
| public function testMd5ReturnsCorrectHash(): void | ||
| { | ||
| $knownContent = 'The quick brown fox jumps over the lazy dog'; | ||
| $knownFile = $this->tempDir . '/known_md5.txt'; | ||
| file_put_contents($knownFile, $knownContent); | ||
|
|
||
| $handler = new XoopsFileHandler($knownFile); | ||
| $hash = $handler->md5(true); | ||
|
|
||
| $this->assertSame(md5($knownContent), $hash); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------- | ||
| // prepare() tests | ||
| // --------------------------------------------------------------- | ||
|
|
||
| /** | ||
| * prepare() normalizes line endings based on the current OS. | ||
| */ | ||
| public function testPrepareNormalizesLineEndings(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
|
|
||
| $input = "line1\nline2\r\nline3\rline4"; | ||
| $result = $handler->prepare($input); | ||
|
|
||
| // On Windows (which is the test environment), all line breaks | ||
| // should be converted to \r\n | ||
| if (strncasecmp(PHP_OS, 'WIN', 3) === 0) { | ||
| $expected = "line1\r\nline2\r\nline3\r\nline4"; | ||
| } else { | ||
| $expected = "line1\nline2\nline3\nline4"; | ||
| } | ||
|
|
||
| $this->assertSame($expected, $result); | ||
| } | ||
|
|
||
| /** | ||
| * prepare() preserves content without line breaks. | ||
| */ | ||
| public function testPreparePreservesContentWithoutLineBreaks(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $input = 'No line breaks here'; | ||
| $result = $handler->prepare($input); | ||
|
|
||
| $this->assertSame($input, $result); | ||
| } | ||
|
|
||
| /** | ||
| * prepare() handles empty string. | ||
| */ | ||
| public function testPrepareHandlesEmptyString(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $this->assertSame('', $handler->prepare('')); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------- | ||
| // folder() tests | ||
| // --------------------------------------------------------------- | ||
|
|
||
| /** | ||
| * folder() returns a XoopsFolderHandler instance. | ||
| */ | ||
| public function testFolderReturnsXoopsFolderHandlerInstance(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $folder = $handler->folder(); | ||
|
|
||
| $this->assertInstanceOf(XoopsFolderHandler::class, $folder); | ||
| } | ||
|
|
||
| /** | ||
| * folder() returns a reference to the same folder object as the property. | ||
| */ | ||
| public function testFolderReturnsSameAsProperty(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $folder = &$handler->folder(); | ||
|
|
||
| $this->assertSame($handler->folder, $folder); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------- | ||
| // offset() tests | ||
| // --------------------------------------------------------------- | ||
|
|
||
| /** | ||
| * offset(false) returns current file pointer position when handle is open. | ||
| */ | ||
| public function testOffsetFalseReturnsCurrentPosition(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $handler->open('r'); | ||
|
|
||
| $position = $handler->offset(false); | ||
|
|
||
| $this->assertSame(0, $position); | ||
| $handler->close(); | ||
| } | ||
|
|
||
| /** | ||
| * offset(int) seeks to the specified position. | ||
| */ | ||
| public function testOffsetIntSeeksToPosition(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $handler->open('r'); | ||
|
|
||
| $result = $handler->offset(5); | ||
| $this->assertTrue($result); | ||
|
|
||
| $position = $handler->offset(false); | ||
| $this->assertSame(5, $position); | ||
|
|
||
| $handler->close(); | ||
| } | ||
|
|
||
| /** | ||
| * offset(false) returns false when no handle is open. | ||
| */ | ||
| public function testOffsetFalseReturnsFalseWithNoHandle(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| // Do not open the file — handle is not a resource | ||
| $result = $handler->offset(false); | ||
|
|
||
| $this->assertFalse($result); | ||
| } | ||
|
|
||
| /** | ||
| * offset() with a position opens the file automatically if not open. | ||
| */ | ||
| public function testOffsetWithPositionOpensFileAutomatically(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
|
|
||
| // File is not opened yet; offset should call open() internally | ||
| $result = $handler->offset(3); | ||
| $this->assertTrue($result); | ||
|
|
||
| $position = $handler->offset(false); | ||
| $this->assertSame(3, $position); | ||
|
|
||
| $handler->close(); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------- | ||
| // create() tests | ||
| // --------------------------------------------------------------- | ||
|
|
||
| /** | ||
| * create() creates a new file via touch() and returns true. | ||
| */ | ||
| public function testCreateCreatesNewFile(): void | ||
| { | ||
| $newFile = $this->tempDir . '/touch_created.txt'; | ||
| // Initialize handler without creating the file | ||
| $handler = new XoopsFileHandler($newFile, false); | ||
|
|
||
| $result = $handler->create(); | ||
|
|
||
| $this->assertTrue($result); | ||
| $this->assertFileExists($newFile); | ||
| } | ||
|
|
||
| /** | ||
| * create() returns false if the file already exists. | ||
| */ | ||
| public function testCreateReturnsFalseIfFileAlreadyExists(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
|
|
||
| $result = $handler->create(); | ||
| $this->assertFalse($result); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------- | ||
| // delete() additional test — returns false for non-existent file | ||
| // --------------------------------------------------------------- | ||
|
|
||
| /** | ||
| * delete() returns false when the file does not exist. | ||
| */ | ||
| public function testDeleteReturnsFalseForNonExistentFile(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempDir . '/phantom.txt', false); | ||
| $this->assertFalse($handler->delete()); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------- | ||
| // Integrated read/write cycle tests | ||
| // --------------------------------------------------------------- | ||
|
|
||
| /** | ||
| * A full write-then-read cycle returns the written data. | ||
| */ | ||
| public function testWriteThenReadCycle(): void | ||
| { | ||
| $cycleFile = $this->tempDir . '/cycle.txt'; | ||
| file_put_contents($cycleFile, ''); | ||
|
|
||
| $writer = new XoopsFileHandler($cycleFile); | ||
| $writer->write('Cycle Test Data'); | ||
| $writer->close(); | ||
|
|
||
| $reader = new XoopsFileHandler($cycleFile); | ||
| $content = $reader->read(); | ||
| $reader->close(); | ||
|
|
||
| $this->assertSame('Cycle Test Data', $content); | ||
| } | ||
|
|
||
| /** | ||
| * Write, append, then read returns combined content. | ||
| */ | ||
| public function testWriteAppendReadCycle(): void | ||
| { | ||
| $cycleFile = $this->tempDir . '/append_cycle.txt'; | ||
| file_put_contents($cycleFile, ''); | ||
|
|
||
| $handler = new XoopsFileHandler($cycleFile); | ||
| $handler->write('First'); | ||
| $handler->close(); | ||
|
|
||
| $handler2 = new XoopsFileHandler($cycleFile); | ||
| $handler2->append(' Second'); | ||
| $handler2->close(); | ||
|
|
||
| $handler3 = new XoopsFileHandler($cycleFile); | ||
| $content = $handler3->read(); | ||
| $handler3->close(); | ||
|
|
||
| $this->assertSame('First Second', $content); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------- | ||
| // size() after write | ||
| // --------------------------------------------------------------- | ||
|
|
||
| /** | ||
| * size() returns updated size after writing new content. | ||
| */ | ||
| public function testSizeReflectsWrittenContent(): void | ||
| { | ||
| $sizeFile = $this->tempDir . '/size_write.txt'; | ||
| file_put_contents($sizeFile, ''); | ||
|
|
||
| $handler = new XoopsFileHandler($sizeFile); | ||
| $handler->write('12345'); | ||
| $handler->close(); | ||
|
|
||
| clearstatcache(true, $sizeFile); | ||
| $handler2 = new XoopsFileHandler($sizeFile); | ||
| $this->assertSame(5, $handler2->size()); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------- | ||
| // exists() edge cases | ||
| // --------------------------------------------------------------- | ||
|
|
||
| /** | ||
| * exists() returns false for a path that is actually a directory, not a file. | ||
| */ | ||
| public function testExistsReturnsFalseForDirectory(): void | ||
| { | ||
| // XoopsFileHandler with a directory path: name won't be set (is_dir check) | ||
| // and exists() checks is_file(), so it should be false. | ||
| // But the constructor skips setting name when is_dir is true. | ||
| // We test the edge case where pwd() would point to a dir. | ||
| $subDir = $this->tempDir . '/subdir'; | ||
| mkdir($subDir, 0755, true); | ||
|
|
||
| $handler = new XoopsFileHandler($subDir, false); | ||
| // name is not set because is_dir($path) is true in constructor | ||
| // exists() calls file_exists() && is_file() on pwd() | ||
| // This is an edge case: the handler's pwd() might not be valid for exists() | ||
| $this->assertFalse($handler->exists()); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------- | ||
| // info() caching behavior | ||
| // --------------------------------------------------------------- | ||
|
|
||
| /** | ||
| * info() caches its result — subsequent calls return the same array. | ||
| */ | ||
| public function testInfoIsCached(): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); | ||
| $info1 = $handler->info(); | ||
| $info2 = $handler->info(); | ||
|
|
||
| $this->assertSame($info1, $info2); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------- | ||
| // Data providers | ||
| // --------------------------------------------------------------- | ||
|
|
||
| /** | ||
| * Provides unsafe filenames and their expected safe versions. | ||
| * | ||
| * @return array<string, array{string, string, string}> | ||
| */ | ||
| public static function unsafeFilenameProvider(): array | ||
| { | ||
| return [ | ||
| 'spaces' => ['my file.txt', '.txt', 'my_file'], | ||
| 'special chars' => ['file@#$.txt', '.txt', 'file_'], | ||
| 'parentheses' => ['copy (1).txt', '.txt', 'copy_1_'], | ||
| 'already safe' => ['safe_file.txt', '.txt', 'safe_file'], | ||
| 'hyphens allowed' => ['my-file.txt', '.txt', 'my-file'], | ||
| ]; | ||
| } | ||
|
|
||
| /** | ||
| * Tests safe() with various unsafe filenames via data provider. | ||
| */ | ||
| #[DataProvider('unsafeFilenameProvider')] | ||
| public function testSafeWithDataProvider(string $name, string $ext, string $expected): void | ||
| { | ||
| $handler = new XoopsFileHandler($this->tempFile); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "^\s*#\[" tests/unit/htdocs/class/file/XoopsFileHandlerTest.phpRepository: XOOPS/XoopsCore25
Length of output: 1733
Remove PHP 8 attributes for PHP 7.4 compatibility.
The test class uses #[CoversClass] and #[DataProvider] attributes, which are PHP 8.0+ only features. Replace with PHPUnit docblock annotations to maintain PHP 7.4–8.5 compatibility and cross-version PHPUnit support (9.6, 10.5, 11.x).
Replacement pattern
-#[CoversClass(XoopsFileHandler::class)]
-class XoopsFileHandlerTest extends TestCase
+/**
+ * `@coversDefaultClass` \XoopsFileHandler
+ */
+class XoopsFileHandlerTest extends TestCase
{
// ...
- #[DataProvider('unsafeFilenameProvider')]
- public function testSafeWithDataProvider(string $name, string $ext, string $expected): void
+ /**
+ * `@dataProvider` unsafeFilenameProvider
+ */
+ public function testSafeWithDataProvider(string $name, string $ext, string $expected): void🧰 Tools
🪛 PHPMD (2.15.0)
[error] 85-85: Avoid using static access to class '\XoopsFile' in method 'setUp'. (undefined)
(StaticAccess)
[error] 86-86: Avoid using static access to class '\XoopsFile' in method 'setUp'. (undefined)
(StaticAccess)
[error] 99-115: Remove error control operator '@' on line 104. (undefined)
(ErrorControlOperator)
[error] 120-141: Remove error control operator '@' on line 137. (undefined)
(ErrorControlOperator)
[error] 120-141: Remove error control operator '@' on line 140. (undefined)
(ErrorControlOperator)
[error] 136-138: The method removeDirectory uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them. (undefined)
(ElseExpression)
[warning] 712-712: Avoid variables with short names like $ts. Configured minimum length is 3. (undefined)
(ShortVariable)
[warning] 733-733: Avoid variables with short names like $ts. Configured minimum length is 3. (undefined)
(ShortVariable)
[error] 834-836: The method testPrepareNormalizesLineEndings uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them. (undefined)
(ElseExpression)
🤖 Prompt for AI Agents
In `@tests/unit/htdocs/class/file/XoopsFileHandlerTest.php` around lines 52 -
1127, The test file uses PHP 8 attributes that break PHP 7.4; remove the
attribute lines #[CoversClass(XoopsFileHandler::class)] on the
XoopsFileHandlerTest class and #[DataProvider('unsafeFilenameProvider')] on the
testSafeWithDataProvider method, and replace them with PHPUnit docblock
annotations: add a class docblock with `@covers` XoopsFileHandler (above class
XoopsFileHandlerTest) and add a method docblock with `@dataProvider`
unsafeFilenameProvider (above testSafeWithDataProvider), ensuring the annotation
text matches the attribute targets so PHPUnit still associates the coverage and
data provider correctly.
Comment on lines
+1
to
+15
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace xoopsfile; | ||
|
|
||
| use PHPUnit\Framework\Attributes\CoversClass; | ||
| use PHPUnit\Framework\Attributes\DataProvider; | ||
| use PHPUnit\Framework\Attributes\Test; | ||
| use PHPUnit\Framework\TestCase; | ||
| use XoopsFolderHandler; | ||
|
|
||
| #[CoversClass(XoopsFolderHandler::class)] | ||
| class XoopsFolderHandlerPathTest extends TestCase | ||
| { |
There was a problem hiding this comment.
Missing XOOPS header and required class docblock metadata.
Add the standard XOOPS header and class-level docblock tags.
As per coding guidelines, "Every source file begins with the XOOPS copyright header block" and "Class docblocks include @category, @package, @author, @copyright, @license, and @link tags."
🤖 Prompt for AI Agents
In `@tests/unit/htdocs/class/file/XoopsFolderHandlerPathTest.php` around lines 1 -
15, Add the standard XOOPS file header comment block at the top of the file and
add a class-level docblock immediately above the XoopsFolderHandlerPathTest
class declaration containing the required tags (`@category`, `@package`, `@author`,
`@copyright`, `@license`, `@link`); ensure the header uses the same copyright/license
text used across the project and the class docblock references
XoopsFolderHandlerPathTest and its purpose so it matches project conventions.
Comment on lines
+12
to
+36
| /** | ||
| * Comprehensive unit tests for XoopsLocalAbstract. | ||
| * | ||
| * XoopsLocalAbstract provides locale-aware string manipulation, encoding | ||
| * conversion, time formatting, and number/money formatting for the XOOPS | ||
| * core. Most methods have a multibyte path (XOOPS_USE_MULTIBYTES=1) and | ||
| * a single-byte fallback path (XOOPS_USE_MULTIBYTES=0). | ||
| * | ||
| * The test bootstrap defines XOOPS_USE_MULTIBYTES=0 and _CHARSET='UTF-8', | ||
| * so we primarily test the single-byte code paths here. | ||
| * | ||
| * Tested API: | ||
| * - substr() Substring with trimmarker support | ||
| * - trim() Whitespace trimming | ||
| * - convert_encoding() Character encoding conversion | ||
| * - utf8_encode() UTF-8 encoding (non-multibyte path) | ||
| * - utf8_decode() UTF-8 decoding (non-multibyte path) | ||
| * - number_format() Returns number unchanged (base implementation) | ||
| * - money_format() Returns number unchanged (base implementation) | ||
| * - __call() Magic method dispatching to PHP functions | ||
| * - getTimeFormatDesc() Returns the _TIMEFORMAT_DESC constant | ||
| * - formatTimestamp() RSS format only (minimal dependency path) | ||
| * | ||
| */ | ||
| #[CoversClass(XoopsLocalAbstract::class)] |
There was a problem hiding this comment.
Class docblock missing required XOOPS metadata tags.
Please add the mandated @category, @package, @author, @copyright, @license, and @link tags.
As per coding guidelines, "Class docblocks include @category, @package, @author, @copyright, @license, and @link tags."
🤖 Prompt for AI Agents
In `@tests/unit/htdocs/class/XoopsLocalAbstractTest.php` around lines 12 - 36,
Update the class docblock above the XoopsLocalAbstractTest declaration to
include the required XOOPS metadata tags: add `@category`, `@package`, `@author`,
`@copyright`, `@license`, and `@link` entries in the block that documents the tests
for XoopsLocalAbstract; ensure the tags reference appropriate values (e.g., the
XOOPS project/package and copyright holder) and follow the existing docblock
style so tools consuming the docblock (and unit test metadata) recognize them.
Comment on lines
+36
to
+169
| #[CoversClass(XoopsLocalAbstract::class)] | ||
| class XoopsLocalAbstractTest extends TestCase | ||
| { | ||
| /** | ||
| * Ensure the source class is loaded before any test runs. | ||
| */ | ||
| public static function setUpBeforeClass(): void | ||
| { | ||
| require_once XOOPS_ROOT_PATH . '/class/xoopslocal.php'; | ||
|
|
||
| // Define _TIMEFORMAT_DESC if not already set by the bootstrap | ||
| if (!defined('_TIMEFORMAT_DESC')) { | ||
| define('_TIMEFORMAT_DESC', 'Use the following PHP date format characters:'); | ||
| } | ||
| } | ||
|
|
||
| // --------------------------------------------------------------- | ||
| // substr() tests — single-byte path (XOOPS_USE_MULTIBYTES = 0) | ||
| // --------------------------------------------------------------- | ||
|
|
||
| /** | ||
| * A short string that fits entirely within the length limit should be | ||
| * returned without any trimmarker appended. | ||
| */ | ||
| public function testSubstrShortStringReturnsWithoutTrimmarker(): void | ||
| { | ||
| // "Hello" is 5 chars; length=10 means no trimming needed | ||
| $result = XoopsLocalAbstract::substr('Hello', 0, 10); | ||
| $this->assertSame('Hello', $result); | ||
| } | ||
|
|
||
| /** | ||
| * A string whose length exactly equals the limit should be returned | ||
| * without trimming. | ||
| */ | ||
| public function testSubstrExactLengthReturnsWithoutTrimmarker(): void | ||
| { | ||
| // "Hello" is 5 chars; start=0, length=5 -> strlen(5)-0 <= 5 -> true | ||
| $result = XoopsLocalAbstract::substr('Hello', 0, 5); | ||
| $this->assertSame('Hello', $result); | ||
| } | ||
|
|
||
| /** | ||
| * A string longer than the limit should be truncated and the default | ||
| * trimmarker '...' appended. | ||
| */ | ||
| public function testSubstrLongStringAppendsTrimmarker(): void | ||
| { | ||
| // "Hello World" is 11 chars; length=8 -> 11-0=11 > 8 -> truncates | ||
| // Effective length: 8 - strlen('...') = 8 - 3 = 5 chars + '...' | ||
| $result = XoopsLocalAbstract::substr('Hello World', 0, 8); | ||
| $this->assertSame('Hello...', $result); | ||
| } | ||
|
|
||
| /** | ||
| * A custom trimmarker should be used instead of the default '...'. | ||
| */ | ||
| public function testSubstrCustomTrimmarker(): void | ||
| { | ||
| // "Hello World" is 11 chars; length=8 -> truncates | ||
| // Effective length: 8 - strlen('~') = 7 chars + '~' | ||
| $result = XoopsLocalAbstract::substr('Hello World', 0, 8, '~'); | ||
| $this->assertSame('Hello W~', $result); | ||
| } | ||
|
|
||
| /** | ||
| * An empty trimmarker means no marker is appended, but the string is | ||
| * still truncated to the exact length. | ||
| */ | ||
| public function testSubstrEmptyTrimmarker(): void | ||
| { | ||
| // "Hello World" is 11 chars; length=5 -> truncates | ||
| // Effective length: 5 - strlen('') = 5 chars + '' | ||
| $result = XoopsLocalAbstract::substr('Hello World', 0, 5, ''); | ||
| $this->assertSame('Hello', $result); | ||
| } | ||
|
|
||
| /** | ||
| * A non-zero start offset should begin extraction at the given position. | ||
| */ | ||
| public function testSubstrWithStartOffset(): void | ||
| { | ||
| // "Hello World" with start=6, length=20 -> strlen(11)-6=5 <= 20 -> no trimming | ||
| $result = XoopsLocalAbstract::substr('Hello World', 6, 20); | ||
| $this->assertSame('World', $result); | ||
| } | ||
|
|
||
| /** | ||
| * Start offset with truncation should work correctly together. | ||
| */ | ||
| public function testSubstrStartOffsetWithTruncation(): void | ||
| { | ||
| // "Hello World" with start=6, length=4 -> strlen(11)-6=5 > 4 -> truncates | ||
| // Effective: substr('Hello World', 6, 4-3) = substr('Hello World', 6, 1) = 'W' + '...' | ||
| $result = XoopsLocalAbstract::substr('Hello World', 6, 4); | ||
| $this->assertSame('W...', $result); | ||
| } | ||
|
|
||
| /** | ||
| * An empty string should return an empty string regardless of parameters. | ||
| */ | ||
| public function testSubstrEmptyString(): void | ||
| { | ||
| $result = XoopsLocalAbstract::substr('', 0, 10); | ||
| $this->assertSame('', $result); | ||
| } | ||
|
|
||
| /** | ||
| * When the trimmarker is longer than the allowed length, the method | ||
| * should still function (though the result may be unusual). This tests | ||
| * robustness rather than a specific requirement. | ||
| */ | ||
| public function testSubstrTrimmarkerLongerThanLength(): void | ||
| { | ||
| // "Hello World" is 11 chars; length=2, trimmarker='...' (3 chars) | ||
| // 11 - 0 = 11 > 2, so: substr('Hello World', 0, 2-3) = substr('Hello World', 0, -1) + '...' | ||
| // PHP substr with negative length returns everything except last char(s) | ||
| $result = XoopsLocalAbstract::substr('Hello World', 0, 2); | ||
| // substr('Hello World', 0, -1) = 'Hello Worl' + '...' = 'Hello Worl...' | ||
| $this->assertSame('Hello Worl...', $result); | ||
| } | ||
|
|
||
| /** | ||
| * Various combinations of strings and lengths via data provider. | ||
| */ | ||
| #[DataProvider('substrProvider')] | ||
| public function testSubstrVariousCombinations( | ||
| string $input, | ||
| int $start, | ||
| int $length, | ||
| string $trimmarker, | ||
| string $expected | ||
| ): void { | ||
| $result = XoopsLocalAbstract::substr($input, $start, $length, $trimmarker); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for all PHP 8 attributes in the test file
rg -n "^\s*#\[" tests/unit/htdocs/class/XoopsLocalAbstractTest.phpRepository: XOOPS/XoopsCore25
Length of output: 1733
Replace PHP 8 attributes with docblock annotations for PHP 7.4 compatibility.
#[CoversClass] and #[DataProvider] use PHP 8.0+ attribute syntax, which prevents parsing on PHP 7.4. Replace with equivalent docblock annotations:
Suggested pattern (apply to all attribute usages)
-#[CoversClass(XoopsLocalAbstract::class)]
-class XoopsLocalAbstractTest extends TestCase
+/**
+ * `@coversDefaultClass` \XoopsLocalAbstract
+ */
+class XoopsLocalAbstractTest extends TestCase
{
@@
- #[DataProvider('substrProvider')]
- public function testSubstrVariousCombinations(
+ /**
+ * `@dataProvider` substrProvider
+ */
+ public function testSubstrVariousCombinations(
string $input,
int $start,
int $length,
string $trimmarker,
string $expected
): void {This is required by the PHP 7.4 compatibility mandate and works across PHPUnit 9.6, 10.5, and 11.x.
🧰 Tools
🪛 PHPMD (2.15.0)
[error] 63-63: Avoid using static access to class '\XoopsLocalAbstract' in method 'testSubstrShortStringReturnsWithoutTrimmarker'. (undefined)
(StaticAccess)
[error] 74-74: Avoid using static access to class '\XoopsLocalAbstract' in method 'testSubstrExactLengthReturnsWithoutTrimmarker'. (undefined)
(StaticAccess)
[error] 86-86: Avoid using static access to class '\XoopsLocalAbstract' in method 'testSubstrLongStringAppendsTrimmarker'. (undefined)
(StaticAccess)
[error] 97-97: Avoid using static access to class '\XoopsLocalAbstract' in method 'testSubstrCustomTrimmarker'. (undefined)
(StaticAccess)
[error] 109-109: Avoid using static access to class '\XoopsLocalAbstract' in method 'testSubstrEmptyTrimmarker'. (undefined)
(StaticAccess)
[error] 119-119: Avoid using static access to class '\XoopsLocalAbstract' in method 'testSubstrWithStartOffset'. (undefined)
(StaticAccess)
[error] 130-130: Avoid using static access to class '\XoopsLocalAbstract' in method 'testSubstrStartOffsetWithTruncation'. (undefined)
(StaticAccess)
[error] 139-139: Avoid using static access to class '\XoopsLocalAbstract' in method 'testSubstrEmptyString'. (undefined)
(StaticAccess)
[error] 153-153: Avoid using static access to class '\XoopsLocalAbstract' in method 'testSubstrTrimmarkerLongerThanLength'. (undefined)
(StaticAccess)
[error] 169-169: Avoid using static access to class '\XoopsLocalAbstract' in method 'testSubstrVariousCombinations'. (undefined)
(StaticAccess)
🤖 Prompt for AI Agents
In `@tests/unit/htdocs/class/XoopsLocalAbstractTest.php` around lines 36 - 169,
The test class uses PHP 8 attribute syntax
(#[CoversClass(XoopsLocalAbstract::class)] and
#[DataProvider('substrProvider')]) which breaks on PHP 7.4; replace these
attributes in class-level and method-level declarations with equivalent docblock
annotations (use `@covers` XoopsLocalAbstract for the CoversClass attribute and
`@dataProvider` substrProvider for the DataProvider attribute) and apply the same
replacement for any other attribute usages in this file so the tests parse under
PHP 7.4.
Comment on lines
+92
to
+105
| #[Test] | ||
| public function stripSlashesGpcReturnsString(): void | ||
| { | ||
| // Ensure logger exists — stripSlashesGPC may log deprecation | ||
| if (!isset($GLOBALS['xoopsLogger'])) { | ||
| $GLOBALS['xoopsLogger'] = \XoopsLogger::getInstance(); | ||
| } | ||
| $ext = MyTextSanitizerExtended::getInstance(); | ||
| $input = "test string"; | ||
| $result = $ext->stripSlashesGPC($input); | ||
| // stripSlashesGPC returns the string (no-op since magic_quotes removed) | ||
| $this->assertIsString($result); | ||
| $this->assertSame($input, $result); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant logger initialization.
The logger check on lines 96-98 duplicates what's already done in setUpBeforeClass() (lines 22-24). Since setUpBeforeClass() runs before any test method, this guard is unnecessary.
♻️ Remove redundant logger check
#[Test]
public function stripSlashesGpcReturnsString(): void
{
- // Ensure logger exists — stripSlashesGPC may log deprecation
- if (!isset($GLOBALS['xoopsLogger'])) {
- $GLOBALS['xoopsLogger'] = \XoopsLogger::getInstance();
- }
$ext = MyTextSanitizerExtended::getInstance();
$input = "test string";
$result = $ext->stripSlashesGPC($input);🧰 Tools
🪛 PHPMD (2.15.0)
[error] 93-105: stripSlashesGpcReturnsString accesses the super-global variable $GLOBALS. (undefined)
(Superglobals)
[error] 93-105: stripSlashesGpcReturnsString accesses the super-global variable $GLOBALS. (undefined)
(Superglobals)
[error] 97-97: Avoid using static access to class '\XoopsLogger' in method 'stripSlashesGpcReturnsString'. (undefined)
(StaticAccess)
[error] 99-99: Avoid using static access to class '\MyTextSanitizerExtended' in method 'stripSlashesGpcReturnsString'. (undefined)
(StaticAccess)
🤖 Prompt for AI Agents
In `@tests/unit/htdocs/Frameworks/textsanitizer/MyTextSanitizerExtendedTest.php`
around lines 92 - 105, Remove the redundant logger initialization inside the
test method stripSlashesGpcReturnsString in MyTextSanitizerExtendedTest: the
check/assignment of $GLOBALS['xoopsLogger'] is already performed in
setUpBeforeClass(), so delete the if (!isset($GLOBALS['xoopsLogger'])) {
$GLOBALS['xoopsLogger'] = \XoopsLogger::getInstance(); } block from the
stripSlashesGpcReturnsString test, leaving the rest of the test (instantiating
MyTextSanitizerExtended, calling stripSlashesGPC, and the assertions) unchanged.
Collaborator
Author
Self-review — small fixes before mergeThe 3 production changes (sqlutility.php, saxparser.php, modulesadmin.php) are solid. A few small items to clean up: Production code
Test code
|
- Relax DROP TABLE regex trailing whitespace (\s)? to (\s)* in sqlutility.php - Replace hardcoded credential with placeholder in XoopsHttpGetTest - Remove unused variables ($id in XoopsObjectTreeTest, $result in FunctionsIniTest) - Replace placeholder assertTrue(true) with markTestIncomplete in XoopsMediaUploaderTest - Fix incomplete tearDown in ModuleAdminTest (unset xoopsModule and xoopsConfig) - Add isset guard for $GLOBALS['xoopsConfig'] in XoopsUserUtilityTest setUp Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| 'url with query string' => ['https://example.com/search?q=test&page=1'], | ||
| 'url with fragment' => ['https://example.com/page#section'], | ||
| 'url with port' => ['https://example.com:8080/api'], | ||
| 'url with auth' => ['https://user:placeholder@example.com'], |
Check failure
Code scanning / SonarCloud
Credentials should not be hard-coded
|
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In `@tests/unit/htdocs/class/XoopsHttpGetTest.php`:
- Around line 7-8: Replace PHP 8 attributes with PHPUnit docblock annotations:
remove the "use PHPUnit\Framework\Attributes\CoversClass" and "use
PHPUnit\Framework\Attributes\DataProvider" imports and delete the
#[CoversClass(...)] on the XoopsHttpGetTest class (replace it with a class
doc-comment like /** `@coversDefaultClass` Fully\\Qualified\\ClassName */ above
class XoopsHttpGetTest) and replace any #[DataProvider('providerName')] on test
methods with a method doc-comment /** `@dataProvider` providerName */ directly
above the test method; ensure provider function names match and update any
`@covers/`@coversDefaultClass targets to the correct fully-qualified class name.
- Around line 79-92: Replace the hard precondition assertion in
testConstructorSetUseCurlTrueWhenCurlAvailable with a conditional test skip:
check function_exists('curl_init') and call $this->markTestSkipped('curl
extension not available') when curl isn’t present before constructing
XoopsHttpGet; keep the rest of the test (creating XoopsHttpGet, using
ReflectionProperty on XoopsHttpGet::$useCurl, and asserting true) unchanged so
the test gracefully skips in CI environments without curl.
In `@tests/unit/htdocs/class/XoopsMediaUploaderTest.php`:
- Around line 1-23: Add the standard XOOPS file header block at the top of
XoopsMediaUploaderTest.php and update the class docblock for the
XoopsMediaUploaderTest class to include the required tags; specifically insert
the XOOPS copyright/header comment used across the codebase before the namespace
declaration and ensure the class docblock contains `@category`, `@package`, `@author`,
`@copyright`, `@license`, and `@link` entries (matching format used in other XOOPS
files) so the file and class comply with project coding guidelines.
In `@tests/unit/htdocs/class/XoopsUserUtilityTest.php`:
- Around line 483-484: The PHPUnit attribute
#[\PHPUnit\Framework\Attributes\DataProvider('ipStringProvider')] is PHP 8+ only
and breaks PHP 7.4; replace it on the testVariousIpAddressesReturnCorrectString
method with the traditional docblock annotation /** `@dataProvider`
ipStringProvider */ directly above the method declaration so PHPUnit still uses
the ipStringProvider data provider while preserving PHP 7.4 compatibility.
- Around line 455-456: The PHPUnit DataProvider attribute on
testVariousIpAddressesReturnCorrectIp2long breaks PHP 7.4; replace the PHP 8
attribute usage on that method with the legacy docblock annotation form (use
`@dataProvider` ipAddressProvider in the method's PHPDoc) so the test still uses
ipAddressProvider while remaining PHP 7.4 compatible.
- Around line 6-7: In XoopsUserUtilityTest, remove the now-unused PHP 8
attribute import(s) (e.g. PHPUnit\Framework\Attributes\CoversClass) since the
class-level attributes were converted to docblock annotations; keep only the
imports still referenced (e.g. PHPUnit\Framework\TestCase) and ensure there are
no other leftover attribute imports at the top of the file.
- Line 22: Remove the PHP 8 attribute #[CoversClass(\XoopsUserUtility::class)]
from the XoopsUserUtilityTest class and replace it with the equivalent PHPUnit
docblock annotation for PHP 7.4 compatibility: add a class-level doc-comment
containing `@covers` \XoopsUserUtility (or `@covers` \XoopsUserUtility::class as
plain text) above the XoopsUserUtilityTest class declaration so tests remain
compatible with PHPUnit 9.6 and PHP 7.4.
In `@tests/unit/htdocs/Frameworks/art/FunctionsIniTest.php`:
- Around line 288-300: The anonymous mock returned by createMockModule has an
unused parameter $format in its getVar method which triggers static analysis; to
fix, rename the parameter to _format (or prefix it with an underscore) in the
getVar signature of the anonymous class so the signature still matches
XoopsObject::getVar while indicating intentional non-use; update any internal
references if present (none here) and keep behavior of getVar unchanged.
- Around line 11-21: Add a defensive tearDown() method to FunctionsIniTest that
restores/cleans the global state to prevent test pollution: implement tearDown()
to reset $GLOBALS['xoopsModule'], $GLOBALS['VAR_PREFIXU'],
$GLOBALS['MOD_DB_PREFIX'], $GLOBALS['xoopsModuleConfig'] and $_POST to their
original or null/empty values after each test; ensure it complements the
existing setUpBeforeClass() behavior (and any per-test setup) so tests that
throw exceptions still have globals restored.
- Around line 7-9: Remove the PHP 8 attribute usage and unused import: delete
the unused CoversFunction import and remove all #[Test] attributes in this test
file (e.g., the attributes applied to methods like
testParseReturnsExpectedWhenEmpty or other test* methods); since the methods are
already named with the test prefix, rely on the method name or add /** `@test` */
docblocks if needed for readability to restore compatibility with PHPUnit 9.6
and PHP 7.4–8.5. Ensure no other PHP 8-only attributes remain in this class.
Comment on lines
+7
to
+8
| use PHPUnit\Framework\Attributes\CoversClass; | ||
| use PHPUnit\Framework\Attributes\DataProvider; |
There was a problem hiding this comment.
PHP 8 attributes break PHPUnit 9.6 / PHP 7.4 compatibility.
The #[CoversClass] and #[DataProvider] attributes are PHP 8.0+ features that won't work with PHPUnit 9.6 running on PHP 7.4. Per coding guidelines, tests must work across PHPUnit 9.6, 10.5, and 11.x.
Use doc-comment annotations instead for cross-version compatibility:
🔧 Proposed fix: Replace attributes with doc-comment annotations
-use PHPUnit\Framework\Attributes\CoversClass;
-use PHPUnit\Framework\Attributes\DataProvider;
use PHPUnit\Framework\TestCase;For the class-level annotation (line 52):
/**
* Comprehensive unit tests for XoopsHttpGet.
* ...
+ *
+ * `@covers` \XoopsHttpGet
*/
-#[CoversClass(XoopsHttpGet::class)]
class XoopsHttpGetTest extends TestCaseFor the data provider (line 112):
/**
* Constructor stores various URL formats correctly.
+ *
+ * `@dataProvider` urlProvider
*/
-#[DataProvider('urlProvider')]
public function testConstructorStoresVariousUrlFormats(string $url): voidAs per coding guidelines: "Tests must work across PHPUnit 9.6, 10.5, and 11.x" and "Do not use features exclusive to PHP 8.0+ (attributes #[...])".
Also applies to: 52-52, 112-112
🤖 Prompt for AI Agents
In `@tests/unit/htdocs/class/XoopsHttpGetTest.php` around lines 7 - 8, Replace PHP
8 attributes with PHPUnit docblock annotations: remove the "use
PHPUnit\Framework\Attributes\CoversClass" and "use
PHPUnit\Framework\Attributes\DataProvider" imports and delete the
#[CoversClass(...)] on the XoopsHttpGetTest class (replace it with a class
doc-comment like /** `@coversDefaultClass` Fully\\Qualified\\ClassName */ above
class XoopsHttpGetTest) and replace any #[DataProvider('providerName')] on test
methods with a method doc-comment /** `@dataProvider` providerName */ directly
above the test method; ensure provider function names match and update any
`@covers/`@coversDefaultClass targets to the correct fully-qualified class name.
Comment on lines
+79
to
+92
| public function testConstructorSetUseCurlTrueWhenCurlAvailable(): void | ||
| { | ||
| $this->assertTrue( | ||
| function_exists('curl_init'), | ||
| 'Precondition: curl_init must be available on this system' | ||
| ); | ||
|
|
||
| $httpGet = new XoopsHttpGet('https://example.com'); | ||
|
|
||
| $ref = new ReflectionProperty(XoopsHttpGet::class, 'useCurl'); | ||
| $ref->setAccessible(true); | ||
|
|
||
| $this->assertTrue($ref->getValue($httpGet)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider conditional skip for curl-dependent tests.
This test has a hard precondition requiring curl extension. If this runs in a CI environment without curl, the assertion at line 82 fails the test rather than skipping it gracefully.
💡 Suggested improvement for graceful skip
public function testConstructorSetUseCurlTrueWhenCurlAvailable(): void
{
- $this->assertTrue(
- function_exists('curl_init'),
- 'Precondition: curl_init must be available on this system'
- );
+ if (!function_exists('curl_init')) {
+ $this->markTestSkipped('curl extension not available');
+ }
$httpGet = new XoopsHttpGet('https://example.com');📝 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
| public function testConstructorSetUseCurlTrueWhenCurlAvailable(): void | |
| { | |
| $this->assertTrue( | |
| function_exists('curl_init'), | |
| 'Precondition: curl_init must be available on this system' | |
| ); | |
| $httpGet = new XoopsHttpGet('https://example.com'); | |
| $ref = new ReflectionProperty(XoopsHttpGet::class, 'useCurl'); | |
| $ref->setAccessible(true); | |
| $this->assertTrue($ref->getValue($httpGet)); | |
| } | |
| public function testConstructorSetUseCurlTrueWhenCurlAvailable(): void | |
| { | |
| if (!function_exists('curl_init')) { | |
| $this->markTestSkipped('curl extension not available'); | |
| } | |
| $httpGet = new XoopsHttpGet('https://example.com'); | |
| $ref = new ReflectionProperty(XoopsHttpGet::class, 'useCurl'); | |
| $ref->setAccessible(true); | |
| $this->assertTrue($ref->getValue($httpGet)); | |
| } |
🤖 Prompt for AI Agents
In `@tests/unit/htdocs/class/XoopsHttpGetTest.php` around lines 79 - 92, Replace
the hard precondition assertion in
testConstructorSetUseCurlTrueWhenCurlAvailable with a conditional test skip:
check function_exists('curl_init') and call $this->markTestSkipped('curl
extension not available') when curl isn’t present before constructing
XoopsHttpGet; keep the rest of the test (creating XoopsHttpGet, using
ReflectionProperty on XoopsHttpGet::$useCurl, and asserting true) unchanged so
the test gracefully skips in CI environments without curl.
Comment on lines
+1
to
+23
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace xoopsclass; | ||
|
|
||
| use PHPUnit\Framework\Attributes\DataProvider; | ||
| use PHPUnit\Framework\TestCase; | ||
| use ReflectionClass; | ||
|
|
||
| // Load the uploader language constants | ||
| require_once XOOPS_ROOT_PATH . '/language/english/uploader.php'; | ||
|
|
||
| // Load the real uploader class | ||
| require_once XOOPS_ROOT_PATH . '/class/uploader.php'; | ||
|
|
||
| /** | ||
| * Unit tests for XoopsMediaUploader. | ||
| * | ||
| * The constructor requires $GLOBALS['xoops']->path() and language files, | ||
| * so we use ReflectionClass::newInstanceWithoutConstructor() and set | ||
| * properties directly via public access. | ||
| */ |
There was a problem hiding this comment.
Add the required XOOPS file header and class docblock tags.
This new file starts without the XOOPS copyright header, and the class docblock lacks the mandatory @category, @package, @author, @copyright, @license, and @link tags. Please align both with existing XOOPS PHP files.
As per coding guidelines: Every source file begins with the XOOPS copyright header block; Class docblocks include @category, @package, @author, @copyright, @license, and @link tags.
🤖 Prompt for AI Agents
In `@tests/unit/htdocs/class/XoopsMediaUploaderTest.php` around lines 1 - 23, Add
the standard XOOPS file header block at the top of XoopsMediaUploaderTest.php
and update the class docblock for the XoopsMediaUploaderTest class to include
the required tags; specifically insert the XOOPS copyright/header comment used
across the codebase before the namespace declaration and ensure the class
docblock contains `@category`, `@package`, `@author`, `@copyright`, `@license`, and `@link`
entries (matching format used in other XOOPS files) so the file and class comply
with project coding guidelines.
Comment on lines
+6
to
+7
| use PHPUnit\Framework\Attributes\CoversClass; | ||
| use PHPUnit\Framework\TestCase; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused attribute imports after converting to doc-comments.
Once the PHP 8 attributes are replaced with doc-comment annotations, these imports become unused and should be removed:
🔧 Proposed fix
-use PHPUnit\Framework\Attributes\CoversClass;
use PHPUnit\Framework\TestCase;🤖 Prompt for AI Agents
In `@tests/unit/htdocs/class/XoopsUserUtilityTest.php` around lines 6 - 7, In
XoopsUserUtilityTest, remove the now-unused PHP 8 attribute import(s) (e.g.
PHPUnit\Framework\Attributes\CoversClass) since the class-level attributes were
converted to docblock annotations; keep only the imports still referenced (e.g.
PHPUnit\Framework\TestCase) and ensure there are no other leftover attribute
imports at the top of the file.
| * Other methods (sendWelcome, validate, getUnameFromId, getUnameFromIds) depend | ||
| * heavily on DB/mailer/global state and are not tested here. | ||
| */ | ||
| #[CoversClass(\XoopsUserUtility::class)] |
There was a problem hiding this comment.
PHP 8 attribute breaks PHP 7.4 compatibility.
The #[CoversClass] attribute is PHP 8.0+ syntax. Per coding guidelines, attributes are prohibited to maintain PHP 7.4 compatibility. Additionally, tests must work across PHPUnit 9.6 (which supports PHP 7.4).
Use the equivalent doc-comment annotation instead:
🔧 Proposed fix
-#[CoversClass(\XoopsUserUtility::class)]
+/**
+ * `@covers` \XoopsUserUtility
+ */
class XoopsUserUtilityTest extends TestCaseAs per coding guidelines, Code must run on PHP 7.4 through 8.5. Do not use features exclusive to PHP 8.0+ (attributes #[...]).
🤖 Prompt for AI Agents
In `@tests/unit/htdocs/class/XoopsUserUtilityTest.php` at line 22, Remove the PHP
8 attribute #[CoversClass(\XoopsUserUtility::class)] from the
XoopsUserUtilityTest class and replace it with the equivalent PHPUnit docblock
annotation for PHP 7.4 compatibility: add a class-level doc-comment containing
`@covers` \XoopsUserUtility (or `@covers` \XoopsUserUtility::class as plain text)
above the XoopsUserUtilityTest class declaration so tests remain compatible with
PHPUnit 9.6 and PHP 7.4.
Comment on lines
+455
to
+456
| #[\PHPUnit\Framework\Attributes\DataProvider('ipAddressProvider')] | ||
| public function testVariousIpAddressesReturnCorrectIp2long(string $ip, int $expected): void |
There was a problem hiding this comment.
PHP 8 attribute breaks PHP 7.4 compatibility.
The #[DataProvider] attribute is PHP 8.0+ syntax. Use the doc-comment annotation instead:
🔧 Proposed fix
+ /**
+ * `@dataProvider` ipAddressProvider
+ */
- #[\PHPUnit\Framework\Attributes\DataProvider('ipAddressProvider')]
public function testVariousIpAddressesReturnCorrectIp2long(string $ip, int $expected): void📝 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
| #[\PHPUnit\Framework\Attributes\DataProvider('ipAddressProvider')] | |
| public function testVariousIpAddressesReturnCorrectIp2long(string $ip, int $expected): void | |
| /** | |
| * `@dataProvider` ipAddressProvider | |
| */ | |
| public function testVariousIpAddressesReturnCorrectIp2long(string $ip, int $expected): void |
🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 456-456: Avoid variables with short names like $ip. Configured minimum length is 3. (undefined)
(ShortVariable)
🤖 Prompt for AI Agents
In `@tests/unit/htdocs/class/XoopsUserUtilityTest.php` around lines 455 - 456, The
PHPUnit DataProvider attribute on testVariousIpAddressesReturnCorrectIp2long
breaks PHP 7.4; replace the PHP 8 attribute usage on that method with the legacy
docblock annotation form (use `@dataProvider` ipAddressProvider in the method's
PHPDoc) so the test still uses ipAddressProvider while remaining PHP 7.4
compatible.
Comment on lines
+483
to
+484
| #[\PHPUnit\Framework\Attributes\DataProvider('ipStringProvider')] | ||
| public function testVariousIpAddressesReturnCorrectString(string $ip): void |
There was a problem hiding this comment.
PHP 8 attribute breaks PHP 7.4 compatibility.
Same issue as above. Replace with the doc-comment annotation:
🔧 Proposed fix
+ /**
+ * `@dataProvider` ipStringProvider
+ */
- #[\PHPUnit\Framework\Attributes\DataProvider('ipStringProvider')]
public function testVariousIpAddressesReturnCorrectString(string $ip): void📝 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
| #[\PHPUnit\Framework\Attributes\DataProvider('ipStringProvider')] | |
| public function testVariousIpAddressesReturnCorrectString(string $ip): void | |
| /** | |
| * `@dataProvider` ipStringProvider | |
| */ | |
| public function testVariousIpAddressesReturnCorrectString(string $ip): void |
🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 484-484: Avoid variables with short names like $ip. Configured minimum length is 3. (undefined)
(ShortVariable)
🤖 Prompt for AI Agents
In `@tests/unit/htdocs/class/XoopsUserUtilityTest.php` around lines 483 - 484, The
PHPUnit attribute
#[\PHPUnit\Framework\Attributes\DataProvider('ipStringProvider')] is PHP 8+ only
and breaks PHP 7.4; replace it on the testVariousIpAddressesReturnCorrectString
method with the traditional docblock annotation /** `@dataProvider`
ipStringProvider */ directly above the method declaration so PHPUnit still uses
the ipStringProvider data provider while preserving PHP 7.4 compatibility.
Comment on lines
+7
to
+9
| use PHPUnit\Framework\Attributes\CoversFunction; | ||
| use PHPUnit\Framework\Attributes\Test; | ||
| use PHPUnit\Framework\TestCase; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -50 tests/unit/htdocs/Frameworks/art/FunctionsIniTest.phpRepository: XOOPS/XoopsCore25
Length of output: 1449
🏁 Script executed:
grep -n "#\[Test\]" tests/unit/htdocs/Frameworks/art/FunctionsIniTest.php | head -10Repository: XOOPS/XoopsCore25
Length of output: 212
🏁 Script executed:
grep -n "CoversFunction" tests/unit/htdocs/Frameworks/art/FunctionsIniTest.phpRepository: XOOPS/XoopsCore25
Length of output: 111
🏁 Script executed:
wc -l tests/unit/htdocs/Frameworks/art/FunctionsIniTest.phpRepository: XOOPS/XoopsCore25
Length of output: 118
🏁 Script executed:
grep -A2 -B2 "CoversFunction" tests/unit/htdocs/Frameworks/art/FunctionsIniTest.phpRepository: XOOPS/XoopsCore25
Length of output: 206
🌐 Web query:
PHPUnit 9.6 PHP 8 attributes support test annotation
💡 Result:
PHPUnit 9.6 does not support PHP 8 attributes (such as #[Test], #[DataProvider], etc.) as a replacement for DocBlock “annotations”. Attributes are supported starting with PHPUnit 10+; before that, PHPUnit relied on DocBlock annotations. (docs.phpunit.de)
So on PHPUnit 9.6, keep using:
- method naming (
testSomething()) and/or/**@test*/ /**@dataProvider... */,/**@Depends... */,/**@group... */,/**@Covers... */, etc. (docs.phpunit.de)
Citations:
- 1: https://docs.phpunit.de/en/11.5/attributes.html?utm_source=openai
- 2: https://docs.phpunit.de/en/9.6/
PHP 8 attributes break PHPUnit 9.6 compatibility.
The #[Test] attributes (used throughout the file) require PHPUnit 10+ and are not supported in PHPUnit 9.6. Per coding guidelines, tests must work across PHPUnit 9.6, 10.5, and 11.x. Additionally, the coding guidelines explicitly forbid PHP 8.0+ exclusive features including attributes, since code must run on PHP 7.4 through 8.5.
The CoversFunction import on line 7 is also unused and should be removed.
Replace all #[Test] attributes with the test method prefix naming convention (which already follows this pattern) or with doc-comment annotations (/** @test */) for compatibility. The current test{MethodName}{Scenario} naming already makes the explicit #[Test] attribute redundant.
🤖 Prompt for AI Agents
In `@tests/unit/htdocs/Frameworks/art/FunctionsIniTest.php` around lines 7 - 9,
Remove the PHP 8 attribute usage and unused import: delete the unused
CoversFunction import and remove all #[Test] attributes in this test file (e.g.,
the attributes applied to methods like testParseReturnsExpectedWhenEmpty or
other test* methods); since the methods are already named with the test prefix,
rely on the method name or add /** `@test` */ docblocks if needed for readability
to restore compatibility with PHPUnit 9.6 and PHP 7.4–8.5. Ensure no other PHP
8-only attributes remain in this class.
Comment on lines
+11
to
+21
| class FunctionsIniTest extends TestCase | ||
| { | ||
| private static bool $loaded = false; | ||
|
|
||
| public static function setUpBeforeClass(): void | ||
| { | ||
| if (!self::$loaded) { | ||
| require_once XOOPS_ROOT_PATH . '/Frameworks/art/functions.ini.php'; | ||
| self::$loaded = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider centralizing global cleanup in tearDown().
Multiple tests individually save/restore global state ($GLOBALS['xoopsModule'], $GLOBALS['VAR_PREFIXU'], $GLOBALS['MOD_DB_PREFIX'], $GLOBALS['xoopsModuleConfig'], $_POST). A tearDown() method could provide a safety net to reset these globals, preventing test pollution if a test throws an exception before restoration.
♻️ Add tearDown for defensive cleanup
protected function setUp(): void
{
// Ensure xoopsLogger exists for deprecation logging
if (!isset($GLOBALS['xoopsLogger'])) {
$GLOBALS['xoopsLogger'] = \XoopsLogger::getInstance();
}
}
+ protected function tearDown(): void
+ {
+ // Defensive cleanup of globals that tests may set
+ unset(
+ $GLOBALS['VAR_PREFIXU'],
+ $GLOBALS['MOD_DB_PREFIX'],
+ $GLOBALS['xoopsModuleConfig']
+ );
+ }🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 11-301: The class FunctionsIniTest has 17 public methods. Consider refactoring FunctionsIniTest to keep number of public methods under 10. (undefined)
(TooManyPublicMethods)
🤖 Prompt for AI Agents
In `@tests/unit/htdocs/Frameworks/art/FunctionsIniTest.php` around lines 11 - 21,
Add a defensive tearDown() method to FunctionsIniTest that restores/cleans the
global state to prevent test pollution: implement tearDown() to reset
$GLOBALS['xoopsModule'], $GLOBALS['VAR_PREFIXU'], $GLOBALS['MOD_DB_PREFIX'],
$GLOBALS['xoopsModuleConfig'] and $_POST to their original or null/empty values
after each test; ensure it complements the existing setUpBeforeClass() behavior
(and any per-test setup) so tests that throw exceptions still have globals
restored.
Comment on lines
+288
to
+300
| private function createMockModule(string $dirname): object | ||
| { | ||
| return new class($dirname) { | ||
| private string $dirname; | ||
| public function __construct(string $dirname) { $this->dirname = $dirname; } | ||
| public function getVar(string $key, string $format = 'e') | ||
| { | ||
| if ($key === 'dirname') return $this->dirname; | ||
| if ($key === 'mid') return 1; | ||
| return ''; | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unused parameter in mock class.
The $format parameter on line 293 is never used in the getVar method. While this matches the signature of the real XoopsObject::getVar(), static analysis flags it. Consider prefixing with underscore to indicate intentional non-use.
♻️ Suppress warning with underscore prefix
- public function getVar(string $key, string $format = 'e')
+ public function getVar(string $key, string $_format = 'e')📝 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
| private function createMockModule(string $dirname): object | |
| { | |
| return new class($dirname) { | |
| private string $dirname; | |
| public function __construct(string $dirname) { $this->dirname = $dirname; } | |
| public function getVar(string $key, string $format = 'e') | |
| { | |
| if ($key === 'dirname') return $this->dirname; | |
| if ($key === 'mid') return 1; | |
| return ''; | |
| } | |
| }; | |
| } | |
| private function createMockModule(string $dirname): object | |
| { | |
| return new class($dirname) { | |
| private string $dirname; | |
| public function __construct(string $dirname) { $this->dirname = $dirname; } | |
| public function getVar(string $key, string $_format = 'e') | |
| { | |
| if ($key === 'dirname') return $this->dirname; | |
| if ($key === 'mid') return 1; | |
| return ''; | |
| } | |
| }; | |
| } |
🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 293-293: Avoid unused parameters such as '$format'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
In `@tests/unit/htdocs/Frameworks/art/FunctionsIniTest.php` around lines 288 -
300, The anonymous mock returned by createMockModule has an unused parameter
$format in its getVar method which triggers static analysis; to fix, rename the
parameter to _format (or prefix it with an underscore) in the getVar signature
of the anonymous class so the signature still matches XoopsObject::getVar while
indicating intentional non-use; update any internal references if present (none
here) and keep behavior of getVar unchanged.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 42 out of 134 changed files in this pull request and generated 12 comments.
Comments suppressed due to low confidence (1)
tests/unit/htdocs/class/auth/XoopsAuthFactoryTest.php:1
- This test codifies a
TypeErroras the expected outcome for empty config, which effectively locks in a production bug (passingnulltoin_array()). It would be more valuable to fix the production code to defaultldap_users_bypassto[](or cast to array) and update the test to assert a stable, non-exceptional behavior (e.g., returning a default auth connection). Otherwise, the suite will start failing the moment the underlying bug is fixed.
<?php
Comment on lines
+7
to
+10
| use PHPUnit\Framework\Attributes\CoversClass; | ||
| use PHPUnit\Framework\TestCase; | ||
| use ReflectionClass; | ||
| use XoopsFile; |
There was a problem hiding this comment.
These tests use PHP 8 attributes (e.g., #[CoversClass]). This repository targets PHP 7.4–8.5, and attributes are not supported on PHP 7.4, so the test suite will not run on the lower bound. To keep compatibility, replace attribute-based metadata with PHPUnit docblock annotations (e.g., @covers, @dataProvider, @test) or gate attribute usage behind a PHP version-specific test suite.
Comment on lines
+29
to
+30
| #[CoversClass(XoopsFile::class)] | ||
| class XoopsFileTest extends TestCase |
There was a problem hiding this comment.
These tests use PHP 8 attributes (e.g., #[CoversClass]). This repository targets PHP 7.4–8.5, and attributes are not supported on PHP 7.4, so the test suite will not run on the lower bound. To keep compatibility, replace attribute-based metadata with PHPUnit docblock annotations (e.g., @covers, @dataProvider, @test) or gate attribute usage behind a PHP version-specific test suite.
Comment on lines
65
to
68
| 'tplset', | ||
| 'tplsource', | ||
| 'xoopsnotifications', | ||
| 'banner', | ||
| 'bannerclient', | ||
| 'bannerfinish', | ||
| ]; |
There was a problem hiding this comment.
The reserved table list no longer includes the banner tables (banner, bannerclient, bannerfinish). If those tables are still part of the core schema, removing them from the reserved list can allow module uninstall flows to drop core banner tables (data loss). Re-add these tables to $reservedTables, or (if they were intentionally removed from core) add an explicit comment and/or add runtime guards to prevent dropping tables that are not owned by the module being uninstalled.
Comment on lines
117
to
121
| $msgs[] = '<p>' . sprintf(_AM_SYSTEM_MODULES_SQL_FOUND, "<strong>{$sql_file_path}</strong>") . '<br />' . _AM_SYSTEM_MODULES_CREATE_TABLES; | ||
| include_once XOOPS_ROOT_PATH . '/class/database/sqlutility.php'; | ||
| $sql_query = fread(fopen($sql_file_path, 'r'), filesize($sql_file_path)); | ||
| $sql_query = file_get_contents($sql_file_path); | ||
| $sql_query = trim($sql_query); | ||
| SqlUtility::splitMySqlFile($pieces, $sql_query); |
There was a problem hiding this comment.
file_get_contents() can return false on read failure; passing that into trim() will throw a TypeError on modern PHP. Handle the false case explicitly (e.g., add an error message and set $error = true / abort the install) before calling trim() / splitMySqlFile().
| $sql_query = trim($sql_query); | ||
| SqlUtility::splitMySqlFile($pieces, $sql_query); | ||
| $created_tables = []; | ||
| $db->query('SET FOREIGN_KEY_CHECKS = 0'); |
There was a problem hiding this comment.
Foreign key checks are toggled in multiple places, but there’s no single guaranteed restoration point if an unexpected error occurs between the SET ... = 0 and SET ... = 1 calls. Consider wrapping the SQL execution portions in a try { ... } finally { ... } style pattern (or a small helper) so FK checks are reliably re-enabled even when installation/uninstallation short-circuits due to an error.
| if ($error === false) { | ||
| if (!$module_handler->insert($module)) { | ||
| $errs[] = '<p>' . sprintf(_AM_SYSTEM_MODULES_INSERT_DATA_FAILD, '<strong>' . $module->getVar('name') . '</strong>'); | ||
| $db->query('SET FOREIGN_KEY_CHECKS = 0'); |
There was a problem hiding this comment.
Foreign key checks are toggled in multiple places, but there’s no single guaranteed restoration point if an unexpected error occurs between the SET ... = 0 and SET ... = 1 calls. Consider wrapping the SQL execution portions in a try { ... } finally { ... } style pattern (or a small helper) so FK checks are reliably re-enabled even when installation/uninstallation short-circuits due to an error.
| $db->exec('DROP TABLE ' . $db->prefix($ct)); | ||
| $db->exec('DROP TABLE IF EXISTS ' . $db->prefix($ct)); | ||
| } | ||
| $db->query('SET FOREIGN_KEY_CHECKS = 1'); |
There was a problem hiding this comment.
Foreign key checks are toggled in multiple places, but there’s no single guaranteed restoration point if an unexpected error occurs between the SET ... = 0 and SET ... = 1 calls. Consider wrapping the SQL execution portions in a try { ... } finally { ... } style pattern (or a small helper) so FK checks are reliably re-enabled even when installation/uninstallation short-circuits due to an error.
| $modtables = $module->getInfo('tables'); | ||
| if ($modtables !== false && \is_array($modtables)) { | ||
| $msgs[] = _AM_SYSTEM_MODULES_DELETE_MOD_TABLES; | ||
| $db->query('SET FOREIGN_KEY_CHECKS = 0'); |
There was a problem hiding this comment.
Foreign key checks are toggled in multiple places, but there’s no single guaranteed restoration point if an unexpected error occurs between the SET ... = 0 and SET ... = 1 calls. Consider wrapping the SQL execution portions in a try { ... } finally { ... } style pattern (or a small helper) so FK checks are reliably re-enabled even when installation/uninstallation short-circuits due to an error.
| $msgs[] = ' <span style="color:#ff0000;">' . sprintf(_AM_SYSTEM_MODULES_TABLE_DROPPED_FAILDED, '<strong>' . $db->prefix($table) . '</strong>') . '</span>'; | ||
| } | ||
| } | ||
| $db->query('SET FOREIGN_KEY_CHECKS = 1'); |
There was a problem hiding this comment.
Foreign key checks are toggled in multiple places, but there’s no single guaranteed restoration point if an unexpected error occurs between the SET ... = 0 and SET ... = 1 calls. Consider wrapping the SQL execution portions in a try { ... } finally { ... } style pattern (or a small helper) so FK checks are reliably re-enabled even when installation/uninstallation short-circuits due to an error.
Comment on lines
+81
to
+84
| $this->assertTrue( | ||
| function_exists('curl_init'), | ||
| 'Precondition: curl_init must be available on this system' | ||
| ); |
There was a problem hiding this comment.
This hard assertion makes the test fail on environments where the curl extension isn't installed/enabled. For CI portability, replace the precondition assertion with markTestSkipped() when curl_init is unavailable, and then run the curl-specific assertions only when curl is present.
Suggested change
| $this->assertTrue( | |
| function_exists('curl_init'), | |
| 'Precondition: curl_init must be available on this system' | |
| ); | |
| if (!function_exists('curl_init')) { | |
| $this->markTestSkipped('curl_init is not available; skipping curl-specific constructor test.'); | |
| } |
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
1 participant
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
#[CoversClass()],#[DataProvider()],#[Test])Test Results (PHP 8.4.16 + PHPUnit 11.5)
Tests: 5543, Assertions: 9093, Errors: 0, Failures: 0, Deprecations: 0, Risky: 0, Skipped: 174 (database-dependent)
Changes
New test files (131 files, ~66K lines)
tests/bootstrap.phpwith autoloading and graceful DB skipProduction code fixes (3 files)
htdocs/class/database/sqlutility.php: Fixed implicit nullable?stringparameterhtdocs/class/xml/saxparser.php: Fixed implicit nullable?stringparameterhtdocs/modules/system/admin/modulesadmin/modulesadmin.php: Fixed implicit nullable types + array type hintsRemoved files (3 files)
tests/bootstrap.phpConsolidation Note
This PR consolidates and supersedes mambax7/XoopsCore25 PRs #18, #19, #20, and is related to upstream PR #1312. All tests have been verified passing on a real WAMP installation before being committed.
Test plan
Summary by CodeRabbit
New Features
Bug Fixes
Tests