feat(tests): Add TestSupport trait and corresponding test suite for inaccessible properties and methods.
#12
Conversation
… inaccessible properties and methods.
|
Warning Rate limit exceeded@terabytesoftw has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 40 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughReplaces the final Changes
Sequence Diagram(s)sequenceDiagram
participant Test as PHPUnit TestCase (uses TestSupport)
participant Trait as TestSupport
participant Subject as SubjectUnderTest
Note over Test: Tests call helpers via self:: provided by the trait
Test->>Trait: self::normalizeLineEndings(rawString)
Trait-->>Test: normalizedString
Test->>Trait: self::invokeMethod(subjectInstance, methodName, args)
Trait->>Subject: reflection invoke(methodName, args)
Subject-->>Trait: result
Trait-->>Test: result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
…r installation instructions.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (11)
README.md (5)
105-121: Update example to trait-based API (replaces Assert::inaccessibleProperty).The current example uses the removed Assert class. Suggested replacement:
- use PHPForge\Support\Assert; + use PHPForge\Support\TestSupport; + use PHPUnit\Framework\TestCase; -$object = new class () { - private string $secretValue = 'hidden'; -}; - -// access private properties for testing -$value = Assert::inaccessibleProperty($object, 'secretValue'); - -self::assertSame('hidden', $value); +final class MyTest extends TestCase +{ + use TestSupport; + + public function testAccessPrivateProperty(): void + { + $object = new class () { + private string $secretValue = 'hidden'; + }; + + $value = self::inaccessibleProperty($object, 'secretValue'); + self::assertSame('hidden', $value); + } +}
122-137: Replace removed equalsWithoutLE with normalizeLineEndings usage.equalsWithoutLE no longer exists. Recommend renaming the section and using the trait:
-### Equals without line ending +### Normalizing line endings ```php <?php declare(strict_types=1); -use PHPForge\Support\Assert; +use PHPForge\Support\TestSupport; +use PHPUnit\Framework\TestCase; -// normalize line endings for consistent comparisons -Assert::equalsWithoutLE( - "Foo\r\nBar", - "Foo\nBar", - "Should match regardless of line ending style" -); +final class MyTest extends TestCase +{ + use TestSupport; + + public function testNormalizeLineEndings(): void + { + $expected = self::normalizeLineEndings("Foo\nBar"); + $actual = self::normalizeLineEndings("Foo\r\nBar"); + + self::assertSame($expected, $actual, 'Should match regardless of line-ending style.'); + } +}--- `146-159`: **Update invoking protected method example to trait-based API.** ```diff -use PHPForge\Support\Assert; +use PHPForge\Support\TestSupport; +use PHPUnit\Framework\TestCase; -$object = new class () { - protected function calculate(int $a, int $b): int - { - return $a + $b; - } -}; - -// test protected method behavior -$result = Assert::invokeMethod($object, 'calculate', [5, 3]); - -self::assertSame(8, $result); +final class MyTest extends TestCase +{ + use TestSupport; + + public function testInvokeProtectedMethod(): void + { + $object = new class () { + protected function calculate(int $a, int $b): int + { + return $a + $b; + } + }; + + $result = self::invokeMethod($object, 'calculate', [5, 3]); + self::assertSame(8, $result); + } +}
168-174: Update filesystem cleanup example to trait-based API.-use PHPForge\Support\Assert; +use PHPForge\Support\TestSupport; +use PHPUnit\Framework\TestCase; -$testDir = dirname(__DIR__) . '/runtime'; - -// clean up test artifacts (preserves '.gitignore' and '.gitkeep') -Assert::removeFilesFromDirectory($testDir); +final class MyTest extends TestCase +{ + use TestSupport; + + public function testRemoveFilesFromDirectory(): void + { + $testDir = dirname(__DIR__) . '/runtime'; + // clean up test artifacts (preserves '.gitignore' and '.gitkeep') + self::removeFilesFromDirectory($testDir); + } +}
183-195: Update property set/get example to trait-based API.-use PHPForge\Support\Assert; +use PHPForge\Support\TestSupport; +use PHPUnit\Framework\TestCase; -$object = new class () { - private string $config = 'default'; -}; - -// set private property for testing scenarios -Assert::setInaccessibleProperty($object, 'config', 'test-mode'); - -$newValue = Assert::inaccessibleProperty($object, 'config'); - -self::assertSame('test-mode', $newValue); +final class MyTest extends TestCase +{ + use TestSupport; + + public function testSetPrivateProperty(): void + { + $object = new class () { + private string $config = 'default'; + }; + + self::setInaccessibleProperty($object, 'config', 'test-mode'); + $newValue = self::inaccessibleProperty($object, 'config'); + + self::assertSame('test-mode', $newValue); + } +}src/TestSupport.php (6)
56-64: Reflection access to non-public parent property will fail without setAccessible(true).Accessing private/protected properties requires setting accessibility. Otherwise, Reflection will throw on getValue.
Apply this diff:
): mixed { $class = new ReflectionClass($className); - return $class->getProperty($propertyName)->getValue($object); + $property = $class->getProperty($propertyName); + $property->setAccessible(true); + return $property->getValue($object); }
84-94: Reflection access to non-public property is missing setAccessible(true) and can throw.Also simplify the flow and avoid using an undefined $result when $propertyName is empty.
- public static function inaccessibleProperty(string|object $object, string $propertyName): mixed - { - $class = new ReflectionClass($object); - - if ($propertyName !== '') { - $property = $class->getProperty($propertyName); - $result = is_string($object) ? $property->getValue() : $property->getValue($object); - } - - return $result ?? null; - } + public static function inaccessibleProperty(string|object $object, string $propertyName): mixed + { + if ($propertyName === '') { + return null; + } + + $class = new ReflectionClass($object); + $property = $class->getProperty($propertyName); + $property->setAccessible(true); + + return is_string($object) + ? $property->getValue() // static property + : $property->getValue($object); + }
114-124: Invoking non-public methods requires setAccessible(true). Also avoid variable shadowing.Shadowing $method with a ReflectionMethod hurts readability.
- public static function invokeMethod(object $object, string $method, array $args = []): mixed - { - $reflection = new ReflectionObject($object); - - if ($method !== '') { - $method = $reflection->getMethod($method); - $result = $method->invokeArgs($object, $args); - } - - return $result ?? null; - } + public static function invokeMethod(object $object, string $method, array $args = []): mixed + { + if ($method === '') { + return null; + } + + $reflection = new ReflectionObject($object); + $refMethod = $reflection->getMethod($method); + $refMethod->setAccessible(true); + + return $refMethod->invokeArgs($object, $args); + }
146-160: Parent method invocation also needs setAccessible(true) and can mirror the above cleanup.- ): mixed { - $reflection = new ReflectionClass($parentClass); - - if ($method !== '') { - $method = $reflection->getMethod($method); - $result = $method->invokeArgs($object, $args); - } - - return $result ?? null; - } + ): mixed { + if ($method === '') { + return null; + } + + $reflection = new ReflectionClass($parentClass); + $refMethod = $reflection->getMethod($method); + $refMethod->setAccessible(true); + + return $refMethod->invokeArgs($object, $args); + }
237-251: Setting non-public parent property is missing setAccessible(true); also remove unnecessary unset.): void { $class = new ReflectionClass($parentClass); if ($propertyName !== '') { $property = $class->getProperty($propertyName); - $property->setValue($object, $value); + $property->setAccessible(true); + $property->setValue($object, $value); } - - unset($class, $property); }
267-280: Setting non-public property is missing setAccessible(true); also remove unnecessary unset.): void { - $class = new ReflectionClass($object); - - if ($propertyName !== '') { - $property = $class->getProperty($propertyName); - $property->setValue($object, $value); - } - - unset($class, $property); + if ($propertyName === '') { + return; + } + + $class = new ReflectionClass($object); + $property = $class->getProperty($propertyName); + $property->setAccessible(true); + $property->setValue($object, $value); }
🧹 Nitpick comments (5)
composer.json (1)
14-24: Reconsider moving phpunit to require-dev (consumer experience).Based on previously captured preferences (see retrieved learnings), this library is a testing helper and consumers rely on it in their test suites. To ensure consumers get a working setup out of the box, PHPUnit should be in the main "require" so it’s installed transitively when this package is required as a dev dependency in downstream projects.
Apply this diff to move phpunit/phpunit back to require:
"require": { - "php": "^8.1" + "php": "^8.1", + "phpunit/phpunit": "^10.5" }, "require-dev": { "infection/infection": "^0.27|^0.31", "maglnet/composer-require-checker": "^4.7", "phpstan/phpstan": "^2.1", - "phpstan/phpstan-strict-rules": "^2.0.3", - "phpunit/phpunit": "^10.5", + "phpstan/phpstan-strict-rules": "^2.0.3", "rector/rector": "^2.1", "symplify/easy-coding-standard": "^12.5" },Note: This follows your org’s prior guidance for testing helper packages so downstream consumers don’t need to add an extra explicit phpunit requirement.
README.md (1)
31-31: Capitalize proper nouns (“Windows”, “Unix”).Minor editorial polish.
- Eliminate false positives/negatives caused by windows vs. unix line ending differences. + Eliminate false positives/negatives caused by Windows vs. Unix line-ending differences.src/TestSupport.php (2)
174-177: Normalize classic Mac line endings too.Covers both CRLF and lone CR.
- return str_replace("\r\n", "\n", $line); + return str_replace(["\r\n", "\r"], "\n", $line);
192-217: Avoid error suppression and improve robustness in filesystem cleanup (optional).The @ operator hides issues (permissions, transient FS errors). Consider explicit checks and handling, or at least guard against non-directories before opendir.
Example refinement:
- $handle = @opendir($basePath); + if (!is_dir($basePath)) { + $dirname = basename($basePath); + throw new RuntimeException("Unable to open directory: $dirname"); + } + $handle = opendir($basePath);And replace
@rmdir/@unlinkwith conditional checks or exceptions depending on your desired strictness.tests/TestSupportTest.php (1)
99-104: Fix assertion message: referencing the wrong method name.The test invokes 'inaccessibleMethod' but the message mentions 'inaccessibleParentMethod'.
- "Should return 'value' when invoking the inaccessible method 'inaccessibleParentMethod' on 'TestClass' " . + "Should return 'value' when invoking the inaccessible method 'inaccessibleMethod' on 'TestClass' " .
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
README.md(2 hunks)composer.json(2 hunks)src/TestSupport.php(2 hunks)tests/TestSupportTest.php(10 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: terabytesoftw
PR: php-forge/support#11
File: composer.json:13-16
Timestamp: 2025-08-18T15:43:30.951Z
Learning: Testing helper libraries (packages with "testing" or "support" in keywords/description that provide test utilities) should include PHPUnit and other testing frameworks in the main "require" section rather than "require-dev", as consuming packages need these dependencies to use the testing utilities.
📚 Learning: 2025-08-18T15:43:30.951Z
Learnt from: terabytesoftw
PR: php-forge/support#11
File: composer.json:13-16
Timestamp: 2025-08-18T15:43:30.951Z
Learning: Testing helper libraries (packages with "testing" or "support" in keywords/description that provide test utilities) should include PHPUnit and other testing frameworks in the main "require" section rather than "require-dev", as consuming packages need these dependencies to use the testing utilities.
Applied to files:
composer.json
🧬 Code Graph Analysis (1)
tests/TestSupportTest.php (3)
src/TestSupport.php (8)
inaccessibleParentProperty(56-64)normalizeLineEndings(174-177)removeFilesFromDirectory(192-217)inaccessibleProperty(84-94)invokeMethod(114-124)invokeParentMethod(146-160)setInaccessibleParentProperty(237-251)setInaccessibleProperty(267-280)tests/Stub/TestClass.php (1)
TestClass(15-23)tests/Stub/TestBaseClass.php (1)
TestBaseClass(15-23)
🪛 LanguageTool
README.md
[grammar] ~31-~31: There might be a mistake here.
Context: ...indows vs. unix line ending differences. - Normalize line endings for consistent st...
(QB_NEW_EN)
🔇 Additional comments (2)
composer.json (1)
4-4: Description update reads well.Concise and aligned with the new trait-based testing utilities.
tests/TestSupportTest.php (1)
70-71: LGTM on using the trait in tests.Using the trait statically (self::...) is consistent with the static methods in TestSupport.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
composer.json (1)
16-24: Reconsider moving phpunit to require-dev; consumers of TestSupport may need it at runtime.Per our prior learning for this repository, testing helper libraries should declare PHPUnit in "require" so consumers using these helpers in their tests don’t need to redundantly manage transitive dev dependencies. Your change moves phpunit/phpunit to require-dev, which contradicts that convention and may cause friction for adopters expecting the utility to bring in PHPUnit.
If you agree, move phpunit/phpunit back to "require" and remove it from "require-dev".
Apply this diff:
"require": { - "php": "^8.1" + "php": "^8.1", + "phpunit/phpunit": "^10.5" }, "require-dev": { @@ - "phpstan/phpstan-strict-rules": "^2.0.3", - "phpunit/phpunit": "^10.5", + "phpstan/phpstan-strict-rules": "^2.0.3", "rector/rector": "^2.1", "symplify/easy-coding-standard": "^12.5" },Note: If the intent is to keep PHPUnit only as a dev tool of this package (and require consumers to bring their own), let’s document that explicitly in README’s installation section.
README.md (2)
31-31: Capitalize proper nouns: Windows/Unix.Minor grammar tweak for clarity and consistency.
Apply this diff:
- - Eliminate false positives/negatives caused by windows vs. unix line ending differences. + - Eliminate false positives/negatives caused by Windows vs. Unix line-ending differences.
120-120: Rename section to reflect the new helper.The header still says “Equals without line ending”. Since the helper is normalizeLineEndings, make the header reflect normalization.
Apply this diff:
-### Equals without line ending +### Normalize line endings before comparing
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
README.md(8 hunks)composer.json(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-18T16:13:31.551Z
Learnt from: terabytesoftw
PR: php-forge/support#11
File: phpstan.neon:1-3
Timestamp: 2025-08-18T16:13:31.551Z
Learning: In the php-forge/support project, PHPStan is intended to be used as a PHAR installation rather than as a Composer dependency, even though the composer.json was updated to include phpstan/phpstan as a dependency.
Applied to files:
README.md
📚 Learning: 2025-08-18T15:43:30.951Z
Learnt from: terabytesoftw
PR: php-forge/support#11
File: composer.json:13-16
Timestamp: 2025-08-18T15:43:30.951Z
Learning: Testing helper libraries (packages with "testing" or "support" in keywords/description that provide test utilities) should include PHPUnit and other testing frameworks in the main "require" section rather than "require-dev", as consuming packages need these dependencies to use the testing utilities.
Applied to files:
README.mdcomposer.json
🪛 LanguageTool
README.md
[grammar] ~31-~31: There might be a mistake here.
Context: ...indows vs. unix line ending differences. - Normalize line endings for consistent st...
(QB_NEW_EN)
🔇 Additional comments (5)
composer.json (3)
4-4: Description update reads well.The new description is concise and accurate for the package scope.
20-23: Dev tooling additions look good.Adding phpstan-strict-rules, rector, and ECS under require-dev is appropriate and aligns with the provided scripts.
37-37: Confirm versioning narrative: branch-alias 0.3.x-dev vs. README ^0.2.Branch alias targets the next minor (0.3.x-dev) while README installation suggests ^0.2. This is often fine (README pointing to latest stable). Just confirm that 0.2 will be tagged/published before merging and that Packagist reflects it.
README.md (2)
53-53: Verify Packagist availability of ^0.2 before merge.Installation snippets reference ^0.2. Ensure the 0.2 tag is (or will be) published to avoid 404/confusion immediately after merge.
Also applies to: 63-63
76-101: Trait-based usage example looks correct.Good, realistic example showing the trait mixed into a PHPUnit TestCase and using self::-style helpers.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
CHANGELOG.md (2)
4-5: Use a consistent heading style (setext vs atx) to satisfy markdownlint MD003.Top-level uses setext (“Change Log” with underlines), while this section uses atx “##”. Align to setext for consistency and to satisfy MD003.
Apply:
-## 0.2.0 August 18, 2025 + +0.2.0 August 18, 2025 +---------------------
7-7: Prefer “Enhancement” over the abbreviated “Enh” for clarity.Minor wording polish and consistency with common changelog conventions.
-- Enh #12: Add `TestSupport` trait and corresponding test suite for inaccessible properties and methods (@terabytesoftw) +- Enhancement #12: Add `TestSupport` trait and corresponding test suite for inaccessible properties and methods (@terabytesoftw)README.md (3)
31-32: Capitalize proper nouns and tighten phrasing.“windows/unix” should be “Windows/Unix”. Also, “line ending differences” reads better than “line ending differences.” Hyphenating “line-ending” is optional; below keeps it simple.
-- Eliminate false positives/negatives caused by windows vs. unix line ending differences. +- Eliminate false positives/negatives caused by Windows vs. Unix line ending differences.
82-99: Fix method name typo and PSR-12 spacing in return type.“Inaccesible” → “Inaccessible”; add space before return type per PSR-12.
- public function testInaccesibleProperty():void + public function testInaccessibleProperty(): void
203-205: Remove double space after comma in assert for consistent formatting.Minor style cleanup.
- self::assertSame('test-mode', $newValue, "Should set the inaccessible property to 'test-mode'."); + self::assertSame('test-mode', $newValue, "Should set the inaccessible property to 'test-mode'.");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)README.md(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-18T16:13:31.551Z
Learnt from: terabytesoftw
PR: php-forge/support#11
File: phpstan.neon:1-3
Timestamp: 2025-08-18T16:13:31.551Z
Learning: In the php-forge/support project, PHPStan is intended to be used as a PHAR installation rather than as a Composer dependency, even though the composer.json was updated to include phpstan/phpstan as a dependency.
Applied to files:
README.md
📚 Learning: 2025-08-18T15:43:30.951Z
Learnt from: terabytesoftw
PR: php-forge/support#11
File: composer.json:13-16
Timestamp: 2025-08-18T15:43:30.951Z
Learning: Testing helper libraries (packages with "testing" or "support" in keywords/description that provide test utilities) should include PHPUnit and other testing frameworks in the main "require" section rather than "require-dev", as consuming packages need these dependencies to use the testing utilities.
Applied to files:
README.md
🪛 LanguageTool
CHANGELOG.md
[grammar] ~6-~6: There might be a mistake here.
Context: ...and update dependencies (@terabytesoftw) - Enh #12: Add TestSupport trait and cor...
(QB_NEW_EN)
README.md
[grammar] ~31-~31: There might be a mistake here.
Context: ...indows vs. unix line ending differences. - Normalize line endings for consistent st...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
CHANGELOG.md
4-4: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (6)
src/TestSupport.php (6)
56-64: Private/protected parent property access will fail without setAccessible(true).Reflection must be made accessible to read non-public properties. As written, this will throw on private/protected members.
Apply:
$class = new ReflectionClass($className); - return $class->getProperty($propertyName)->getValue($object); + $property = $class->getProperty($propertyName); + $property->setAccessible(true); + return $property->getValue($object);
84-94: Inaccessible property getter: missing setAccessible(true) and static-property guard.
- Non-public properties require
$property->setAccessible(true).- If
$objectis a class name (string), only static properties can be read; otherwisegetValue()without an instance will error.public static function inaccessibleProperty(string|object $object, string $propertyName): mixed { $class = new ReflectionClass($object); - if ($propertyName !== '') { - $property = $class->getProperty($propertyName); - $result = is_string($object) ? $property->getValue() : $property->getValue($object); - } - - return $result ?? null; + if ($propertyName === '') { + return null; + } + + $property = $class->getProperty($propertyName); + $property->setAccessible(true); + + if (is_string($object)) { + if (!$property->isStatic()) { + throw new ReflectionException(sprintf('Property %s::%s is not static', $class->getName(), $propertyName)); + } + return $property->getValue(); + } + + return $property->getValue($object);
114-124: Invoking non-public methods requires setAccessible(true) and avoid name shadowing.
- Protected/private methods will fail to invoke without making them accessible.
- Reusing
$methodfor the ReflectionMethod shadows the string parameter and hurts readability.public static function invokeMethod(object $object, string $method, array $args = []): mixed { $reflection = new ReflectionObject($object); - if ($method !== '') { - $method = $reflection->getMethod($method); - $result = $method->invokeArgs($object, $args); - } - - return $result ?? null; + if ($method === '') { + return null; + } + + $refMethod = $reflection->getMethod($method); + $refMethod->setAccessible(true); + return $refMethod->invokeArgs($object, $args);
146-160: Parent method invocation: same accessibility gap and variable shadowing.Make the reflected method accessible before invocation and avoid parameter shadowing.
): mixed { $reflection = new ReflectionClass($parentClass); - if ($method !== '') { - $method = $reflection->getMethod($method); - $result = $method->invokeArgs($object, $args); - } - - return $result ?? null; + if ($method === '') { + return null; + } + + $refMethod = $reflection->getMethod($method); + $refMethod->setAccessible(true); + return $refMethod->invokeArgs($object, $args);
237-251: Setting parent non-public properties requires setAccessible(true); avoid unset.
- Non-public properties won’t be writable without
setAccessible(true).unset($class, $property)is unnecessary and can hide undefined-variable mistakes.$class = new ReflectionClass($parentClass); - if ($propertyName !== '') { - $property = $class->getProperty($propertyName); - $property->setValue($object, $value); - } - - unset($class, $property); + if ($propertyName === '') { + return; + } + + $property = $class->getProperty($propertyName); + $property->setAccessible(true); + $property->setValue($object, $value);
267-280: Setting object non-public properties requires setAccessible(true); avoid unset.Same issue as above for non-public property writes.
$class = new ReflectionClass($object); - if ($propertyName !== '') { - $property = $class->getProperty($propertyName); - $property->setValue($object, $value); - } - - unset($class, $property); + if ($propertyName === '') { + return; + } + + $property = $class->getProperty($propertyName); + $property->setAccessible(true); + $property->setValue($object, $value);
♻️ Duplicate comments (1)
README.md (1)
52-54: Move PHPUnit to require in composer.jsonThe package currently lists phpunit/phpunit under
require-dev, which contradicts our policy for testing‐helper libraries (they should ship PHPUnit inrequireso consumers get it automatically).• composer.json
• Remove"phpunit/phpunit": "^10.5"fromrequire-dev
• Add"phpunit/phpunit": "^10.5"torequire"require": { + "phpunit/phpunit": "^10.5", … }, "require-dev": { - "phpunit/phpunit": "^10.5", … }The README’s
composer require --dev php-forge/support:^0.2remains valid for installing this package as a dev dependency.
🧹 Nitpick comments (5)
src/TestSupport.php (2)
162-177: Normalize lone '\r' as well for full cross-platform coverage.On some systems/files, CR can appear without LF. Normalize both CRLF and CR to LF.
public static function normalizeLineEndings(string $line): string { - return str_replace("\r\n", "\n", $line); + return str_replace(["\r\n", "\r"], "\n", $line); }
192-199: Gracefully no-op when basePath isn’t a directory.Early-return avoids emitting warnings when a non-directory path is passed.
- $handle = @opendir($basePath); - - if ($handle === false) { + if (!is_dir($basePath)) { + return; + } + + $handle = @opendir($basePath); + if ($handle === false) { $dirname = basename($basePath); throw new RuntimeException("Unable to open directory: $dirname"); }README.md (3)
31-31: Grammar: clarify phrasing and hyphenation.Replace the slash and add hyphenation to improve readability.
-- Eliminate false positives/negatives caused by Windows vs. Unix line ending differences. +- Eliminate false positives and negatives caused by Windows vs. Unix line-ending differences.
85-99: Fix method name typo and spacing in example.“Inaccesible” → “Inaccessible”; add missing space before return type.
- public function testInaccesibleProperty():void + public function testInaccessibleProperty(): void
193-206: Consistent return type declaration in examples.Other examples use
: void; align for consistency.- public function testSetProperty() + public function testSetProperty(): void
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
README.md(4 hunks)src/TestSupport.php(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-18T20:13:33.486Z
Learnt from: terabytesoftw
PR: php-forge/support#12
File: src/TestSupport.php:0-0
Timestamp: 2025-08-18T20:13:33.486Z
Learning: User terabytesoftw prefers clean trait-based designs over maintaining backward compatibility wrappers when refactoring utility classes in the php-forge/support project.
Applied to files:
src/TestSupport.php
📚 Learning: 2025-08-18T15:43:30.951Z
Learnt from: terabytesoftw
PR: php-forge/support#11
File: composer.json:13-16
Timestamp: 2025-08-18T15:43:30.951Z
Learning: Testing helper libraries (packages with "testing" or "support" in keywords/description that provide test utilities) should include PHPUnit and other testing frameworks in the main "require" section rather than "require-dev", as consuming packages need these dependencies to use the testing utilities.
Applied to files:
README.md
📚 Learning: 2025-08-18T16:13:31.551Z
Learnt from: terabytesoftw
PR: php-forge/support#11
File: phpstan.neon:1-3
Timestamp: 2025-08-18T16:13:31.551Z
Learning: In the php-forge/support project, PHPStan is intended to be used as a PHAR installation rather than as a Composer dependency, even though the composer.json was updated to include phpstan/phpstan as a dependency.
Applied to files:
README.md
🪛 LanguageTool
README.md
[grammar] ~31-~31: There might be a mistake here.
Context: ...indows vs. Unix line ending differences. - Normalize line endings for consistent st...
(QB_NEW_EN)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
README.md (3)
31-32: Polish phrasing for clarity (minor grammar nit).“false positives/negatives” reads awkwardly. Use plain wording.
- - Eliminate false positives/negatives caused by Windows vs. Unix line ending differences. + - Avoid false positives and negatives caused by Windows vs Unix line-ending differences.
137-153: Optional: drop trailing comma in assert for slightly cleaner snippet.PHP 8.1 allows trailing commas, so this is fine. If you want the example to look a bit tidier, you can remove it.
- self::assertSame( - self::normalizeLineEndings("Foo\r\nBar"), - self::normalizeLineEndings("Foo\nBar"), - "Should match regardless of line ending style", - ); + self::assertSame( + self::normalizeLineEndings("Foo\r\nBar"), + self::normalizeLineEndings("Foo\nBar"), + "Should match regardless of line ending style" + );
189-206: Align method signature with other examples (add : void).Minor consistency tweak: other examples declare : void on test methods; add it here too.
- public function testSetProperty() + public function testSetProperty(): void
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
README.md(4 hunks)src/TestSupport.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/TestSupport.php
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-18T15:43:30.951Z
Learnt from: terabytesoftw
PR: php-forge/support#11
File: composer.json:13-16
Timestamp: 2025-08-18T15:43:30.951Z
Learning: Testing helper libraries (packages with "testing" or "support" in keywords/description that provide test utilities) should include PHPUnit and other testing frameworks in the main "require" section rather than "require-dev", as consuming packages need these dependencies to use the testing utilities.
Applied to files:
README.md
📚 Learning: 2025-08-18T16:13:31.551Z
Learnt from: terabytesoftw
PR: php-forge/support#11
File: phpstan.neon:1-3
Timestamp: 2025-08-18T16:13:31.551Z
Learning: In the php-forge/support project, PHPStan is intended to be used as a PHAR installation rather than as a Composer dependency, even though the composer.json was updated to include phpstan/phpstan as a dependency.
Applied to files:
README.md
📚 Learning: 2025-08-18T20:13:33.486Z
Learnt from: terabytesoftw
PR: php-forge/support#12
File: src/TestSupport.php:0-0
Timestamp: 2025-08-18T20:13:33.486Z
Learning: User terabytesoftw prefers clean trait-based designs over maintaining backward compatibility wrappers when refactoring utility classes in the php-forge/support project.
Applied to files:
README.md
🪛 LanguageTool
README.md
[grammar] ~31-~31: There might be a mistake here.
Context: ...indows vs. Unix line ending differences. - Normalize line endings for consistent st...
(QB_NEW_EN)
🔇 Additional comments (5)
README.md (5)
53-54: Install command matches dev-dependency setup. LGTM.The Composer install snippet uses --dev consistently with the manual require-dev example below.
61-64: Manual composer.json snippet is consistent.Using "require-dev" aligns with the installation guidance above. Looks good.
82-99: Example correctly wraps self:: calls in a TestCase using the trait.Valid PHP context, clean and minimal. Good demonstration of inaccessibleProperty.
102-129: Protected method invocation example is clear and correct.The invokeMethod usage and assertion are spot on.
161-177: Directory cleanup example is concise and correct.Demonstrates removeFilesFromDirectory with a minimal assertion. All good.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
README.md (3)
31-31: Polish wording: add period to “vs.” and drop unnecessary hyphen.Improves readability and grammar.
Apply this diff:
-- Avoid false positives and negatives caused by Windows vs Unix line-ending differences. +- Avoid false positives and negatives caused by Windows vs. Unix line ending differences.
146-150: Optional: remove trailing comma in function call for cleaner style.Valid on PHP ≥7.3, but unnecessary here; removing it reads cleaner in docs.
Apply this diff:
self::assertSame( self::normalizeLineEndings("Foo\r\nBar"), self::normalizeLineEndings("Foo\nBar"), - "Should match regardless of line ending style", + "Should match regardless of line ending style" );
168-175: Safer example: use a temporary directory to avoid accidental deletion.Using a project-relative path in docs can be risky if readers copy-paste. Prefer sys_get_temp_dir() in examples.
Apply this diff:
- $testDir = dirname(__DIR__) . '/runtime'; - // clean up test artifacts (preserves '.gitignore' and '.gitkeep') + // Use a temp dir to avoid accidental deletions in your project tree. + $testDir = sys_get_temp_dir() . '/support-runtime'; + // clean up test artifacts (preserves '.gitignore' and '.gitkeep')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
README.md(4 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-18T15:43:30.951Z
Learnt from: terabytesoftw
PR: php-forge/support#11
File: composer.json:13-16
Timestamp: 2025-08-18T15:43:30.951Z
Learning: Testing helper libraries (packages with "testing" or "support" in keywords/description that provide test utilities) should include PHPUnit and other testing frameworks in the main "require" section rather than "require-dev", as consuming packages need these dependencies to use the testing utilities.
Applied to files:
README.md
📚 Learning: 2025-08-18T16:13:31.551Z
Learnt from: terabytesoftw
PR: php-forge/support#11
File: phpstan.neon:1-3
Timestamp: 2025-08-18T16:13:31.551Z
Learning: In the php-forge/support project, PHPStan is intended to be used as a PHAR installation rather than as a Composer dependency, even though the composer.json was updated to include phpstan/phpstan as a dependency.
Applied to files:
README.md
📚 Learning: 2025-08-18T20:13:33.486Z
Learnt from: terabytesoftw
PR: php-forge/support#12
File: src/TestSupport.php:0-0
Timestamp: 2025-08-18T20:13:33.486Z
Learning: User terabytesoftw prefers clean trait-based designs over maintaining backward compatibility wrappers when refactoring utility classes in the php-forge/support project.
Applied to files:
README.md
🪛 LanguageTool
README.md
[grammar] ~31-~31: There might be a mistake here.
Context: ...Windows vs Unix line-ending differences. - Normalize line endings for consistent st...
(QB_NEW_EN)
🔇 Additional comments (5)
README.md (5)
52-54: Install command looks good and aligned with dev-only usage.
61-64: Composer snippet correctly mirrors the install command (dev dependency).
82-99: Great: examples now live inside a PHPUnit TestCase using the trait.This fixes prior context issues with self:: and makes the snippet copy-pasteable.
102-129: LGTM: protected method invocation example is correct and concise.
186-206: LGTM: setting and reading back an inaccessible property is demonstrated well.
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests
Chores