Add PHPUnit contract tests for Object and ObjectHandler#1700
Open
fiammybe wants to merge 12 commits into
Open
Conversation
Review Summary by QodoAdd PHPUnit contract tests for core Object and ObjectHandler classes
WalkthroughsDescription• Add comprehensive PHPUnit test suite for core ImpressCMS base classes • Create isolated test bootstrap with minimal CMS dependencies and mock database • Implement contract tests for icms_core_Object with data providers • Add tests for icms_core_ObjectHandler and icms_ipf_Object classes • Configure PHPUnit with separate test suites for core and IPF modules Diagramflowchart LR
A["Test Bootstrap<br/>tests/bootstrap.php"] -->|defines constants| B["PHPUnit Tests"]
C["Core Object Tests<br/>ObjectTest.php"] -->|tests| D["icms_core_Object"]
E["ObjectHandler Tests<br/>ObjectHandlerTest.php"] -->|tests| F["icms_core_ObjectHandler"]
G["IPF Object Tests<br/>IpfObjectTest.php"] -->|tests| H["icms_ipf_Object"]
B -->|runs| C
B -->|runs| E
B -->|runs| G
I["PHPUnit Config<br/>phpunit.xml.dist"] -->|configures| B
J["Composer Config<br/>composer.json"] -->|defines scripts| B
File Changes1. tests/bootstrap.php
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+92
to
+106
| #[DataProvider('scalarCastingProvider')] | ||
| public function testCleanVarsCastsScalarTypes(int $dataType, $input, int $expected): void | ||
| { | ||
| $object = new icms_core_Object(); | ||
| $object->initVar('value', $dataType, 0); | ||
| $object->setVar('value', $input); | ||
|
|
||
| $this->assertTrue($object->cleanVars()); | ||
| $this->assertSame($expected, $object->cleanVars['value']); | ||
| $this->assertFalse($object->isDirty()); | ||
| } | ||
|
|
||
| #[DataProvider('urlNormalizationProvider')] | ||
| public function testCleanVarsNormalizesUrls(string $input, string $expected): void | ||
| { |
There was a problem hiding this comment.
2. #[dataprovider] attribute used 📘 Rule violation ≡ Correctness
tests/icms/core/ObjectTest.php uses the #[DataProvider(...)] attribute syntax which requires PHP 8.0+, so the test code is not compatible with PHP 7.4. This breaks the stated requirement to keep PHP code runnable on PHP 7.4–8.4.
Agent Prompt
## Issue description
The test file uses PHP 8 attributes (`#[DataProvider(...)]`), which is not valid syntax on PHP 7.4.
## Issue Context
The project compliance requires PHP syntax compatibility with PHP 7.4 through 8.4.
## Fix Focus Areas
- tests/icms/core/ObjectTest.php[92-116]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+20
to
+22
| "require-dev": { | ||
| "phpunit/phpunit": "^11.5" | ||
| } |
There was a problem hiding this comment.
3. phpunit/phpunit requires php 8.2+ 📘 Rule violation ≡ Correctness
composer.json adds phpunit/phpunit ^11.5, which requires a higher minimum PHP version than 7.4. This prevents running the test suite under PHP 7.4 as required.
Agent Prompt
## Issue description
`composer.json` requires PHPUnit 11.x, which is not compatible with PHP 7.4.
## Issue Context
Compliance requires support for PHP 7.4–8.4. PHPUnit 11 requires PHP >= 8.2, so it cannot be installed/executed on PHP 7.4.
## Fix Focus Areas
- composer.json[20-22]
- phpunit.xml.dist[1-8]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
MekDrop
requested changes
May 21, 2026
| } | ||
| } | ||
|
|
||
| final class CoreObjectHandlerTestDouble extends icms_core_ObjectHandler |
Member
There was a problem hiding this comment.
multiple classes per file I think is bad idea; you can use fixture classes or anonymous classes if that really is required; or better use mocking - AI really can help with that
Member
Author
There was a problem hiding this comment.
that should be taken into account now.
Comment on lines
+14
to
+22
| if (!class_exists('icms_core_Filesystem', false)) { | ||
| class icms_core_Filesystem | ||
| { | ||
| public static function mkdir($target, $mode = 0777, $base = ICMS_ROOT_PATH, $metachars = []): bool | ||
| { | ||
| return true; | ||
| } | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
this can generate false positive results if runned with some other tests; use mocking for such things
| } | ||
|
|
||
| if (!defined('ICMS_UPLOAD_PATH')) { | ||
| define('ICMS_UPLOAD_PATH', sys_get_temp_dir()); |
Member
There was a problem hiding this comment.
use https://github.com/bovigo/vfsStream to not generate file spam after tests
Member
Author
There was a problem hiding this comment.
thanks for the suggestion, I'll look into integrating this package
Comment on lines
+113
to
+117
| function icms_loadLanguageFile($name, $module = null) | ||
| { | ||
| // No-op for tests | ||
| return true; | ||
| } |
Comment on lines
+121
to
+124
| function icms_getConfig($key) | ||
| { | ||
| return null; | ||
| } |
Comment on lines
+128
to
+131
| class icms | ||
| { | ||
| public static $config; | ||
| } |
Member
There was a problem hiding this comment.
another trait that helps to work with icms config
Comment on lines
+148
to
+151
| function icms_currency($value) | ||
| { | ||
| return (float) $value; | ||
| } |
Comment on lines
+17
to
+18
| "test:core": "phpunit --configuration phpunit.xml.dist --testsuite icms-core", | ||
| "test:ipf": "phpunit --configuration phpunit.xml.dist --testsuite icms-ipf" |
Member
Author
There was a problem hiding this comment.
the tested classes are in different subfolders of the icms library folder. In the future, a 'test:db' or 'test:view' could be added as well. I admit that for 4 classes, it is overkill
…for tests, externalise everything else
composer.json now includes the upstream package name mikey179/vfsstream in require-dev even though the source repo is bovigo/vfsStream. In tests\icms\ipf\HandlerTest.php, each test now gets a fresh vfsStream root, getImagePath() is pointed at that virtual upload path, and the test asserts the directory is actually created there. I also updated tests\fixtures\IcmsCoreFilesystemStub.php so the test helper performs a real recursive mkdir() in the virtual filesystem instead of returning success unconditionally. The full PHPUnit suite still passes.
…efixingDatabaseMock(). I removed the trait, had tests\icms\ipf\HandlerTest.php require the mock class directly, and put the small createMockDb() helper back on the test class. The focused handler test still passes, and the extra trait file is gone
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
2 participants
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.
Starting out with unit tests for the base IPF classes, on which everything is built. I'm looking for improvements and suggestions to this starting point. Once there is a solid base for these classes, I want to expand it to cover as much of IPF as possible. This will help us tremendously in PHP version updates and internal refactorings.