feat(support): Refactor codebase to improve performance. #18
Conversation
terabytesoftw
commented
Jan 19, 2026
terabytesoftw
commented
| Q | A |
|---|---|
| Is bugfix? | ❌ |
| New feature? | ✔️ |
| Breaks BC? | ❌ |
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughRestructures CI/workflows, updates repository metadata and coding standards, adds several public utility classes (ReflectionHelper, DirectoryCleaner, EnumDataProvider, LineEndingNormalizer, Message), converts TestSupport helpers to private, updates composer and tooling configs, and adds comprehensive tests while removing older community templates and one test file. Changes
Sequence Diagram(s)(Skipped — changes do not introduce a new multi-component sequential control flow that benefits from a sequence diagram.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-08-18T15:47:45.406ZApplied to files:
🧬 Code graph analysis (1)tests/ReflectionHelperTest.php (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
🔇 Additional comments (1)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @.github/workflows/ecs.yml:
- Line 21: Replace the upstream workflow tag reference that currently uses
"@main" in the uses entry (the line referencing
"yii2-framework/actions/.github/workflows/ecs.yml@main") with the stable
versioned tag "@v1" to match linter.yml and mutation.yml; update the uses string
to end with "@v1" so the workflow references the pinned release instead of the
main branch.
In `@README.md`:
- Around line 35-47: Replace the bolded labels currently used as section titles
with real Markdown headings to satisfy MD036: change "**Advanced Reflection
Utilities**", "**Cross-Platform String Assertions**", "**File System Test
Management**", and "**Enum Test Data Generation**" into appropriate heading
lines (e.g., "## Advanced Reflection Utilities", "## Cross-Platform String
Assertions", "## File System Test Management", "## Enum Test Data Generation")
so they become proper headings instead of emphasized text.
In `@tests/TestSupportTest.php`:
- Around line 125-129: The assertion message in TestSupportTest::assertSame
incorrectly references "inaccessibleParentMethod"; update the message to
reference "inaccessibleMethod" to match the actual call to
self::invokeMethod(new TestClass(), 'inaccessibleMethod'), e.g. change the
string mentioning inaccessibleParentMethod to inaccessibleMethod so the error
message correctly describes the invoked method on TestClass.
- Around line 73-81: The unlink calls for "{$dir}/.gitignore" and
"{$dir}/.gitkeep" are unconditional and will raise warnings under failOnWarning;
update the cleanup to check existence first (e.g., use file_exists() or
is_file()) before calling unlink, leaving the rest of the block (the $dir
variable, rmdir($dir), and mkdir($dir, 0777, true)) unchanged so deletes only
occur when those files actually exist.
🧹 Nitpick comments (7)
README.md (1)
52-54: Clarify installation guidance for consumers of test utilities.Line 52-54 uses
--dev. If this package is meant to be a required dependency for downstream test utilities, consider removing--devor explicitly noting when to use it.💡 Suggested tweak
-composer require php-forge/support:^0.2 --dev +composer require php-forge/support:^0.2Based on learnings, please keep install guidance aligned with testing-helper dependency placement expectations.
LICENSE (1)
5-6: Consider updating the copyright year range.Line 5 lists 2024; if 2026 contributions are included, a range like 2024–2026 keeps the notice current.
💡 Suggested tweak
-Copyright (c) 2024, Terabytesoftw (https://github.com/terabytesoftw/) +Copyright (c) 2024-2026, Terabytesoftw (https://github.com/terabytesoftw/).github/workflows/static.yml (1)
20-21: Same@maintag inconsistency.Similar to
ecs.yml, consider using a versioned tag (@v1) for consistency withlinter.ymland to prevent unexpected breaking changes from upstream updates.Suggested change
- uses: yii2-framework/actions/.github/workflows/phpstan.yml@main + uses: yii2-framework/actions/.github/workflows/phpstan.yml@v1.github/workflows/build.yml (1)
20-22: Same@maintag inconsistency.For consistency with
linter.ymland stability, consider using@v1instead of@main.Suggested change
- uses: yii2-framework/actions/.github/workflows/phpunit.yml@main + uses: yii2-framework/actions/.github/workflows/phpunit.yml@v1.editorconfig (1)
19-22: IDE-specific settings in.editorconfig.The
ij_php_*properties are JetBrains-specific (IntelliJ/PhpStorm) and won't be recognized by other editors or EditorConfig plugins. This is fine if the team primarily uses JetBrains IDEs, but be aware these settings won't apply universally..github/workflows/dependency-check.yml (1)
21-21: Same@maintag inconsistency.For consistency with
linter.ymland stability, consider using@v1instead of@main.Suggested change
- uses: yii2-framework/actions/.github/workflows/composer-require-checker.yml@main + uses: yii2-framework/actions/.github/workflows/composer-require-checker.yml@v1tests/EnumDataGeneratorTest.php (1)
34-37: Optional simplification: pass$asHtmldirectly.♻️ Suggested refactor
- $data = match ($asHtml) { - true => EnumDataGenerator::cases($enumClass, $attribute), - false => EnumDataGenerator::cases($enumClass, $attribute, false), - }; + $data = EnumDataGenerator::cases($enumClass, $attribute, $asHtml);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
docs/svgs/features-mobile.svgis excluded by!**/*.svgdocs/svgs/features.svgis excluded by!**/*.svg
📒 Files selected for processing (32)
.editorconfig.gitattributes.github/CODE_OF_CONDUCT.md.github/ISSUE_TEMPLATE.md.github/PULL_REQUEST_TEMPLATE.md.github/ecs.yml.github/workflows/build.yml.github/workflows/dependency-check.yml.github/workflows/ecs.yml.github/workflows/linter.yml.github/workflows/mutation.yml.github/workflows/static.yml.gitignoreCHANGELOG.mdLICENSELICENSE.mdREADME.mdcomposer.jsondocs/development.mddocs/testing.mdecs.phpinfection.json5phpunit.xml.distruntime/.gitignoresrc/EnumDataGenerator.phpsrc/TestSupport.phptests/EnumDataGeneratorTest.phptests/Stub/TestEnum.phptests/Support/Provider/EnumDataGeneratorProvider.phptests/Support/runtime/.gitignoretests/Support/runtime/.gitkeeptests/TestSupportTest.php
💤 Files with no reviewable changes (5)
- LICENSE.md
- .github/ISSUE_TEMPLATE.md
- .github/ecs.yml
- .github/CODE_OF_CONDUCT.md
- .github/PULL_REQUEST_TEMPLATE.md
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: terabytesoftw
Repo: php-forge/support PR: 12
File: src/TestSupport.php:0-0
Timestamp: 2025-08-18T20:13:33.518Z
Learning: User terabytesoftw prefers clean trait-based designs over maintaining backward compatibility wrappers when refactoring utility classes in the php-forge/support project.
Learnt from: terabytesoftw
Repo: php-forge/actions PR: 65
File: .github/workflows/composer-require-checker.yml:80-83
Timestamp: 2025-09-28T15:12:48.345Z
Learning: The user terabytesoftw prefers using floating tags like v1 for third-party actions in GitHub workflows instead of pinning to specific commit SHAs, even when it's a security best practice to pin to immutable commits.
📚 Learning: 2025-08-18T20:13:33.518Z
Learnt from: terabytesoftw
Repo: php-forge/support PR: 12
File: src/TestSupport.php:0-0
Timestamp: 2025-08-18T20:13:33.518Z
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
📚 Learning: 2025-08-18T15:47:45.406Z
Learnt from: terabytesoftw
Repo: php-forge/support PR: 11
File: tests/AssertTest.php:82-89
Timestamp: 2025-08-18T15:47:45.406Z
Learning: In PHP 8.1+, the RFC "Make Reflection setAccessible() a no-op" was implemented. This means ReflectionProperty and ReflectionMethod can access private and protected members automatically without requiring setAccessible(true) calls. The setAccessible() method still exists for backward compatibility but has no effect.
Applied to files:
README.mdsrc/TestSupport.php
📚 Learning: 2025-08-18T15:43:30.996Z
Learnt from: terabytesoftw
Repo: php-forge/support PR: 11
File: composer.json:13-16
Timestamp: 2025-08-18T15:43:30.996Z
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.mdphpunit.xml.distdocs/testing.mdcomposer.json
📚 Learning: 2025-09-12T13:21:35.231Z
Learnt from: terabytesoftw
Repo: php-forge/actions PR: 56
File: .github/workflows/linter.yml:19-21
Timestamp: 2025-09-12T13:21:35.231Z
Learning: When using GitHub Actions reusable workflows, if the reusable workflow defines its own permissions block at the job level, those permissions are sufficient for the actions within that reusable workflow. The caller workflow does not need to duplicate or grant additional permissions in such cases.
Applied to files:
.github/workflows/build.yml
📚 Learning: 2025-09-12T13:21:35.231Z
Learnt from: terabytesoftw
Repo: php-forge/actions PR: 56
File: .github/workflows/linter.yml:19-21
Timestamp: 2025-09-12T13:21:35.231Z
Learning: In GitHub Actions, when a reusable workflow defines its own permissions block, those permissions are sufficient for the actions within that reusable workflow. The caller workflow does not need to grant additional permissions if the reusable workflow already has the necessary permissions defined.
Applied to files:
.github/workflows/build.yml
📚 Learning: 2025-09-29T14:58:04.095Z
Learnt from: terabytesoftw
Repo: php-forge/actions PR: 67
File: .github/workflows/phpstan.yml:13-16
Timestamp: 2025-09-29T14:58:04.095Z
Learning: In the php-forge/actions repository, the maintainer prefers using "composer update" as the default command in CI workflows to ensure the latest dependencies are always installed, rather than using "composer install" for reproducibility.
Applied to files:
.github/workflows/dependency-check.ymldocs/development.md
📚 Learning: 2025-09-12T10:52:52.955Z
Learnt from: terabytesoftw
Repo: php-forge/actions PR: 55
File: .github/workflows/reusable/linter.yml:65-79
Timestamp: 2025-09-12T10:52:52.955Z
Learning: For Super Linter workflows, always recommend using the official super-linter/super-linter action instead of docker run commands, as the action properly handles GitHub context variables and annotations automatically.
Applied to files:
.github/workflows/linter.yml
📚 Learning: 2025-08-18T16:13:31.606Z
Learnt from: terabytesoftw
Repo: php-forge/support PR: 11
File: phpstan.neon:1-3
Timestamp: 2025-08-18T16:13:31.606Z
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:
phpunit.xml.distcomposer.json
🧬 Code graph analysis (2)
tests/TestSupportTest.php (3)
src/TestSupport.php (4)
normalizeLineEndings(156-159)removeFilesFromDirectory(173-202)invokeMethod(102-112)invokeParentMethod(131-145)tests/Stub/TestClass.php (1)
TestClass(15-23)tests/Stub/TestBaseClass.php (1)
TestBaseClass(15-23)
tests/EnumDataGeneratorTest.php (2)
src/EnumDataGenerator.php (3)
EnumDataGenerator(23-90)cases(41-64)tagCases(79-89)tests/Support/Provider/EnumDataGeneratorProvider.php (1)
EnumDataGeneratorProvider(17-55)
🪛 markdownlint-cli2 (0.18.1)
README.md
35-35: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
39-39: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
43-43: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
47-47: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: phpunit / PHP 8.4-ubuntu-latest
- GitHub Check: phpstan / PHP 8.4-ubuntu-latest
- GitHub Check: phpunit / PHP 8.3-windows-2022
- GitHub Check: linter / Super Linter
- GitHub Check: mutation / PHP 8.4-ubuntu-latest
- GitHub Check: linter / Super Linter
🔇 Additional comments (25)
tests/Stub/TestEnum.php (1)
1-17: LGTM — clean and focused enum stub for tests.Line 1-17 looks solid and well-documented.
runtime/.gitignore (1)
1-2: LGTM — restores .gitignore tracking.Line 1-2 is straightforward and correct.
CHANGELOG.md (1)
1-6: LGTM — clear changelog update.Line 1-6 reads well and matches the PR intent.
infection.json5 (1)
1-11: LGTM — clear Infection config.Line 1-11 looks correct and minimal.
.github/workflows/linter.yml (1)
1-17: LGTM!The linter workflow is well-configured with appropriate permissions for super-linter (checks/statuses write for annotations). Using
@v1floating tag aligns with your preference. The absence ofpaths-ignoreis appropriate here since linting should run on all file changes..github/workflows/static.yml (1)
1-21: LGTM on the YAML anchor pattern and permissions.The
&ignore-pathsanchor with*ignore-pathsalias is a clean DRY approach for sharing path-ignore configuration between triggers. Permissions are appropriately scoped..editorconfig (1)
13-34: LGTM on the per-filetype overrides.The added configurations follow common conventions: 2-space indentation for YAML/JS/XML, disabled trailing whitespace trimming for Markdown, and
unsetfor LICENSE files to preserve original formatting..github/workflows/dependency-check.yml (1)
1-21: LGTM on the workflow structure.The YAML anchor pattern, permissions, and workflow naming are all well-configured. Based on learnings, the caller workflow permissions are sufficient since the reusable workflow defines its own permissions.
.gitattributes (1)
27-44: LGTM!The export-ignore patterns are well-organized and appropriately use wildcards for files with variants (
infection.json*,phpstan*.neon*). This ensures development/build artifacts are excluded from distribution archives while keeping the configuration flexible.phpunit.xml.dist (1)
2-23: LGTM!The PHPUnit configuration is clean and well-structured:
- Explicit
./path prefixes provide consistency- Cache directory appropriately placed in
./runtime/(which is export-ignored)- Sensible defaults with
failOnRisky,failOnWarning, andexecutionOrdersettingsdocs/development.md (1)
1-43: LGTM!Well-structured documentation with clear guidance on the sync-metadata workflow. The table format for updated files is helpful, and the important notes about overwrite behavior provide appropriate warnings for developers.
tests/Support/Provider/EnumDataGeneratorProvider.php (1)
17-55: LGTM!Clean data provider implementation following PHPUnit conventions:
- Final class with static methods is appropriate for data providers
- PHPDoc return types accurately describe the array structures
- Test case labels are descriptive and meaningful
.github/workflows/mutation.yml (3)
3-11: Nice use of YAML anchors for DRY.The anchor (
&ignore-paths) and alias (*ignore-paths) pattern effectively avoids duplicating the paths-ignore configuration betweenpull_requestandpushtriggers.
15-17: Good security practice with explicit permissions.Adding explicit
contents: readandpull-requests: writepermissions follows the principle of least privilege.
21-25: Workflow source change verified. Theyii2-framework/actionsrepository is accessible and contains the expectedinfection.ymlworkflow. The floating tag@v1aligns with your tag preferences..gitignore (1)
1-51: LGTM!Well-organized
.gitignorewith clear categorization:
- Appropriate for a library package (ignoring
composer.lockallows consumers to resolve dependencies)- Comprehensive coverage of IDE artifacts, build outputs, and tooling caches
- The
phpunit.xml*pattern appropriately allows local configuration overrides while keepingphpunit.xml.disttrackeddocs/testing.md (1)
3-73: Docs update looks consistent and clear.ecs.php (1)
5-86: ECS config updates look good.composer.json (1)
14-65: Composer dependency and script updates look OK.tests/EnumDataGeneratorTest.php (1)
39-148: Test assertions are thorough and well-structured.src/TestSupport.php (1)
51-75: No BC concern. TestSupport is an internal test utility trait where these methods are appropriately private static. All usage occurs within the package (tests and README examples), and the private visibility correctly enforces that these are implementation details of the trait, not public API. This trait-based design with private methods aligns with clean architecture patterns.Likely an incorrect or invalid review comment.
src/EnumDataGenerator.php (1)
46-50: Guard against potential normalized-key collisions in enum test datasets.If
Enum::normalizeValue()can produce identical results for distinct enum cases, later entries would silently overwrite earlier ones in the generated datasets. While the current tests use simple unit enums where case names are unique, consider making keys more robust by including the enum case name (e.g.,'enum: Foo (normalizedValue)') to guarantee uniqueness, or add validation to detect collisions. This is especially important if the methods are used with backed enums that could normalize unexpectedly.Applies to lines 46-50 and 83-86.
tests/TestSupportTest.php (3)
53-69: Line-ending normalization tests look solid.The no-op case plus CRLF→LF expectation align with
normalizeLineEndingsbehavior.
82-105: Assertions now cover preserved.gitignore/.gitkeep.Nice to explicitly validate the keep-files remain while other files are removed.
138-147:self::assertSamerefactor is consistent.Static assertions and message formatting read cleanly across the updated tests.
Also applies to: 159-164, 176-180
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/linters/actionlint.yml:
- Around line 4-7: In the actionlint ignore list under the ignore key, remove
the generic entry "section is alias node but mapping node is expected" and keep
only the specific ignores for '"pull_request" section is alias node but mapping
node is expected' and '"push" section is alias node but mapping node is
expected' so actionlint won't be silenced for other sections; update the ignore
array to contain only the two specific strings and save the
.github/linters/actionlint.yml change.
🧹 Nitpick comments (1)
README.md (1)
144-158: Consider more complete cleanup in the test example.The finally block only removes
.gitignoreand the directory itself, but if the test fails beforeremoveFilesFromDirectory()is called,artifact.txtwould remain in the temp directory. For a best-practice example, consider cleaning all created files in the finally block.♻️ Optional improvement for more robust cleanup
- try { - file_put_contents($testDir . '/artifact.txt', 'test'); - file_put_contents($testDir . '/.gitignore', "*\n"); - - self::removeFilesFromDirectory($testDir); - - self::assertFileDoesNotExist($testDir . '/artifact.txt'); - self::assertFileExists($testDir . '/.gitignore'); - } finally { - `@unlink`($testDir . '/.gitignore'); - `@rmdir`($testDir); - } + try { + file_put_contents($testDir . '/artifact.txt', 'test'); + file_put_contents($testDir . '/.gitignore', "*\n"); + + self::removeFilesFromDirectory($testDir); + + self::assertFileDoesNotExist($testDir . '/artifact.txt'); + self::assertFileExists($testDir . '/.gitignore'); + } finally { + `@unlink`($testDir . '/artifact.txt'); + `@unlink`($testDir . '/.gitignore'); + `@rmdir`($testDir); + }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/linters/actionlint.ymlREADME.md
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: terabytesoftw
Repo: php-forge/support PR: 12
File: src/TestSupport.php:0-0
Timestamp: 2025-08-18T20:13:33.518Z
Learning: User terabytesoftw prefers clean trait-based designs over maintaining backward compatibility wrappers when refactoring utility classes in the php-forge/support project.
Learnt from: terabytesoftw
Repo: php-forge/actions PR: 65
File: .github/workflows/composer-require-checker.yml:80-83
Timestamp: 2025-09-28T15:12:48.345Z
Learning: The user terabytesoftw prefers using floating tags like v1 for third-party actions in GitHub workflows instead of pinning to specific commit SHAs, even when it's a security best practice to pin to immutable commits.
📚 Learning: 2025-08-18T15:43:30.996Z
Learnt from: terabytesoftw
Repo: php-forge/support PR: 11
File: composer.json:13-16
Timestamp: 2025-08-18T15:43:30.996Z
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.606Z
Learnt from: terabytesoftw
Repo: php-forge/support PR: 11
File: phpstan.neon:1-3
Timestamp: 2025-08-18T16:13:31.606Z
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:47:45.406Z
Learnt from: terabytesoftw
Repo: php-forge/support PR: 11
File: tests/AssertTest.php:82-89
Timestamp: 2025-08-18T15:47:45.406Z
Learning: In PHP 8.1+, the RFC "Make Reflection setAccessible() a no-op" was implemented. This means ReflectionProperty and ReflectionMethod can access private and protected members automatically without requiring setAccessible(true) calls. The setAccessible() method still exists for backward compatibility but has no effect.
Applied to files:
README.md
📚 Learning: 2025-08-18T20:13:33.518Z
Learnt from: terabytesoftw
Repo: php-forge/support PR: 12
File: src/TestSupport.php:0-0
Timestamp: 2025-08-18T20:13:33.518Z
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
🔇 Additional comments (1)
README.md (1)
192-303: > Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/TestSupportTest.php`:
- Around line 159-163: The assertion message is incorrect: it mentions invoking
an inaccessible method but the assertion is checking an inaccessible parent
property via inaccessibleParentProperty on TestBaseClass::class for
'propertyParent'; update the message passed to self::assertSame to accurately
describe that the test expects to retrieve the parent property 'propertyParent'
(e.g., "Should return 'foo' when accessing the inaccessible parent property
'propertyParent' on TestBaseClass via inaccessibleParentProperty"). Ensure the
message references inaccessibleParentProperty, TestBaseClass::class and
'propertyParent' so it matches the assertion intent.
♻️ Duplicate comments (1)
tests/TestSupportTest.php (1)
125-129: Assertion message names the wrong method.The test invokes
inaccessibleMethod, but the message still referencesinaccessibleParentMethod.✏️ Suggested fix
- "Should return 'value' when invoking the inaccessible method 'inaccessibleParentMethod' on 'TestClass' " + "Should return 'value' when invoking the inaccessible method 'inaccessibleMethod' on 'TestClass' " . 'via reflection.',
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/linters/actionlint.yml.github/workflows/build.yml.github/workflows/dependency-check.yml.github/workflows/ecs.yml.github/workflows/static.yml.styleci.ymltests/TestSupportTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/linters/actionlint.yml
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: terabytesoftw
Repo: php-forge/support PR: 12
File: src/TestSupport.php:0-0
Timestamp: 2025-08-18T20:13:33.518Z
Learning: User terabytesoftw prefers clean trait-based designs over maintaining backward compatibility wrappers when refactoring utility classes in the php-forge/support project.
Learnt from: terabytesoftw
Repo: php-forge/actions PR: 65
File: .github/workflows/composer-require-checker.yml:80-83
Timestamp: 2025-09-28T15:12:48.345Z
Learning: The user terabytesoftw prefers using floating tags like v1 for third-party actions in GitHub workflows instead of pinning to specific commit SHAs, even when it's a security best practice to pin to immutable commits.
📚 Learning: 2025-09-28T15:12:48.345Z
Learnt from: terabytesoftw
Repo: php-forge/actions PR: 65
File: .github/workflows/composer-require-checker.yml:80-83
Timestamp: 2025-09-28T15:12:48.345Z
Learning: The user terabytesoftw prefers using floating tags like v1 for third-party actions in GitHub workflows instead of pinning to specific commit SHAs, even when it's a security best practice to pin to immutable commits.
Applied to files:
.github/workflows/ecs.yml
📚 Learning: 2025-09-12T10:52:52.955Z
Learnt from: terabytesoftw
Repo: php-forge/actions PR: 55
File: .github/workflows/reusable/linter.yml:65-79
Timestamp: 2025-09-12T10:52:52.955Z
Learning: For Super Linter workflows, always recommend using the official super-linter/super-linter action instead of docker run commands, as the action properly handles GitHub context variables and annotations automatically.
Applied to files:
.github/workflows/ecs.yml
📚 Learning: 2025-09-29T14:58:04.095Z
Learnt from: terabytesoftw
Repo: php-forge/actions PR: 67
File: .github/workflows/phpstan.yml:13-16
Timestamp: 2025-09-29T14:58:04.095Z
Learning: In the php-forge/actions repository, the maintainer prefers using "composer update" as the default command in CI workflows to ensure the latest dependencies are always installed, rather than using "composer install" for reproducibility.
Applied to files:
.github/workflows/dependency-check.yml
📚 Learning: 2025-08-18T20:13:33.518Z
Learnt from: terabytesoftw
Repo: php-forge/support PR: 12
File: src/TestSupport.php:0-0
Timestamp: 2025-08-18T20:13:33.518Z
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:
tests/TestSupportTest.php
🧬 Code graph analysis (1)
tests/TestSupportTest.php (3)
src/TestSupport.php (2)
normalizeLineEndings(156-159)removeFilesFromDirectory(173-202)tests/Stub/TestClass.php (1)
TestClass(15-23)tests/Stub/TestBaseClass.php (1)
TestBaseClass(15-23)
🔇 Additional comments (17)
.github/workflows/ecs.yml (1)
1-21: Looks good: clean reusable workflow setup.Path ignores, permissions, and the reusable workflow reference are consistent and clear.
.styleci.yml (4)
6-9: LGTM!Excluding
docsandvendordirectories from style checks is appropriate. These directories contain either documentation or third-party code that shouldn't be reformatted.
11-81: LGTM!The enabled rules provide a comprehensive and modern PHP style configuration. Key highlights:
declare_strict_typesenforces strict typing- PHPUnit rules ensure consistent test assertions
- PHPDoc rules maintain documentation quality
- Various
no_*rules eliminate redundant code constructs
83-87: Verify intentional PSR-12 deviations.Disabling
psr12_bracesandpsr12_class_definitionwhile using thepsr12preset creates intentional deviations from PSR-12 compliance. This is acceptable if the project has specific style preferences, but ensure this is documented or well-understood by contributors.
4-4: Version update is appropriate.The version bump to 8.1 aligns with the project's
composer.jsonrequirement of^8.1, enabling PHP 8.1-specific style rules.tests/TestSupportTest.php (5)
53-60: Nice coverage for no-op normalization case.
64-68: Updated CRLF normalization expectation looks correct.
73-105: Refactored temp-dir test setup is clean and robust.
138-147: Updated parent-method assertion message reads correctly.
176-180: LGTM..github/workflows/build.yml (3)
3-11: LGTM - YAML anchor pattern for shared paths-ignore.The
&ignore-pathsanchor onpull_requestwith*ignore-pathsalias onpushis a clean DRY approach that improves maintainability across triggers.
15-16: Good addition of explicit permissions.Adding
contents: readfollows the principle of least privilege and is the minimum required for checkout operations.
20-20: External action reference is properly configured.The repository
yii2-framework/actionsexists, thev1tag is available, and thephpunit.ymlworkflow file is accessible at that tag. The action reference change is valid..github/workflows/static.yml (3)
3-11: Consistent ignore-paths anchor pattern.Matches the pattern established in
build.yml, ensuring consistency across workflows.
15-17: Appropriate permissions for static analysis.The
pull-requests: writepermission enables the static analysis workflow to post comments or annotations directly on PRs, which is useful for inline feedback.
20-21: The workflow reference is valid. Theyii2-framework/actionsrepository provides thephpstan.ymlreusable workflow at thev1tag (currently v1.0.2), confirming that the job rename frompsalmtophpstanis properly aligned with available workflows..github/workflows/dependency-check.yml (1)
1-21: LGTM - Consistent with other workflow refactoring.The changes follow the same pattern as the other workflow files:
- YAML anchor for shared paths-ignore configuration
- Explicit permissions block (contents: read, pull-requests: write) matching the ecs.yml pattern
- Updated action reference to
yii2-framework/actions
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/DirectoryCleaner.php`:
- Around line 10-16: The recursive delete currently uses is_dir() and can
traverse into symlinked directories; modify the logic in DirectoryCleaner where
it decides to recurse (the block that uses is_dir, opendir, readdir, unlink,
rmdir) to treat symlinks as files by adding an explicit is_link() check: only
recurse when is_dir($path) && !is_link($path), and for entries where
is_link($path) is true, call the file-removal path (unlink) instead of
rmdir/recursing; apply the same change for both occurrences noted in the diff
(the initial loop and the second block around lines 64–71).
🧹 Nitpick comments (8)
tests/Support/Provider/EnumDataProviderProvider.php (1)
52-63: Consider adding more variety to tagCasesParameters.The tag cases provider only includes one test case. Consider adding a test with a different category value to improve coverage.
💡 Suggested enhancement
public static function tagCasesParameters(): array { return [ 'default category' => [ TestEnum::class, 'element', ], + 'custom category' => [ + TestEnum::class, + 'widget', + ], ]; }tests/EnumDataProviderTest.php (2)
45-48: Simplify the match expression.The match statement can be replaced with a direct parameter pass-through.
♻️ Proposed simplification
- $data = match ($asHtml) { - true => EnumDataProvider::attributeCases($enumClass, $attribute), - false => EnumDataProvider::attributeCases($enumClass, $attribute, false), - }; + $data = EnumDataProvider::attributeCases($enumClass, $attribute, $asHtml);
80-102: Consider using the data provider for tag cases.
testTagCasesGenerateExpectedStructurehardcodesTestEnum::classand'element'instead of usingEnumDataProviderProvider::tagCasesParameters(). The data provider method exists but is unused. Either wire the test to use#[DataProviderExternal(EnumDataProviderProvider::class, 'tagCasesParameters')]to match the pattern oftestCasesGenerateExpectedStructure, or remove the unusedtagCasesParameters()method.composer.json (1)
55-64: Network dependency for sync-metadata script.The script fetches configuration files from an external repository. A few considerations:
- Uses
-fflag which fails fast on HTTP errors (good)- Uses
-sfor silent mode and-Swould show errors on failure- Consider using
-fsSLfor consistent behavior with redirectsOtherwise, this is a clean approach for maintaining shared configuration.
💡 Minor improvement for curl flags
"sync-metadata": [ - "curl -fsSL -o .editorconfig https://raw.githubusercontent.com/yii2-extensions/template/main/.editorconfig", + "curl -fsSL --retry 3 -o .editorconfig https://raw.githubusercontent.com/yii2-extensions/template/main/.editorconfig",Adding
--retry 3could help with transient network failures, though this is optional.src/ReflectionHelper.php (3)
49-57: Inconsistent handling of empty property name.
inaccessibleParentPropertywill throw aReflectionExceptionif$propertyNameis empty, whereasinaccessibleProperty,setInaccessibleProperty, andsetInaccessibleParentPropertysilently returnnullor no-op. Consider adding a guard for consistency:💡 Suggested change
public static function inaccessibleParentProperty( object $object, string|object $className, string $propertyName, ): mixed { + if ($propertyName === '') { + return null; + } + $class = new ReflectionClass($className); return $class->getProperty($propertyName)->getValue($object); }
174-174: Unnecessaryunset()call.Local variables
$classand$propertyare automatically garbage collected when the method returns. The explicitunset()adds no value and can be removed for cleaner code.💡 Suggested change
if ($propertyName !== '') { $property = $class->getProperty($propertyName); $property->setValue($object, $value); } - - unset($class, $property); }
202-202: Same unnecessaryunset()as above.💡 Suggested change
if ($propertyName !== '') { $property = $class->getProperty($propertyName); $property->setValue($object, $value); } - - unset($class, $property); }tests/ReflectionHelperTest.php (1)
29-120: Consider adding edge case tests.The current tests cover the happy path well. For completeness, consider adding tests for:
- Empty string property/method names (verifies no-op behavior)
- Non-existent property/method names (verifies
ReflectionExceptionis thrown)💡 Example edge case test
public function testInaccessiblePropertyReturnsNullForEmptyPropertyName(): void { self::assertNull( ReflectionHelper::inaccessibleProperty(new TestClass(), ''), 'Should return null when property name is empty.', ); } public function testInaccessiblePropertyThrowsForNonExistentProperty(): void { $this->expectException(ReflectionException::class); ReflectionHelper::inaccessibleProperty(new TestClass(), 'nonExistent'); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
docs/svgs/features-mobile.svgis excluded by!**/*.svgdocs/svgs/features.svgis excluded by!**/*.svg
📒 Files selected for processing (16)
README.mdcomposer.jsonsrc/DirectoryCleaner.phpsrc/EnumDataProvider.phpsrc/Exception/Message.phpsrc/LineEndingNormalizer.phpsrc/ReflectionHelper.phptests/DirectoryCleanerTest.phptests/EnumDataProviderTest.phptests/LineEndingNormalizerTest.phptests/ReflectionHelperTest.phptests/Stub/TestBackedEnum.phptests/Stub/TestEnum.phptests/Stub/TestIntBackedEnum.phptests/Support/Provider/EnumDataProviderProvider.phptests/TestSupportTest.php
💤 Files with no reviewable changes (1)
- tests/TestSupportTest.php
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: terabytesoftw
Repo: php-forge/support PR: 12
File: src/TestSupport.php:0-0
Timestamp: 2025-08-18T20:13:33.518Z
Learning: User terabytesoftw prefers clean trait-based designs over maintaining backward compatibility wrappers when refactoring utility classes in the php-forge/support project.
Learnt from: terabytesoftw
Repo: php-forge/actions PR: 65
File: .github/workflows/composer-require-checker.yml:80-83
Timestamp: 2025-09-28T15:12:48.345Z
Learning: The user terabytesoftw prefers using floating tags like v1 for third-party actions in GitHub workflows instead of pinning to specific commit SHAs, even when it's a security best practice to pin to immutable commits.
📚 Learning: 2025-08-18T15:43:30.996Z
Learnt from: terabytesoftw
Repo: php-forge/support PR: 11
File: composer.json:13-16
Timestamp: 2025-08-18T15:43:30.996Z
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.jsonREADME.md
📚 Learning: 2025-08-18T16:13:31.606Z
Learnt from: terabytesoftw
Repo: php-forge/support PR: 11
File: phpstan.neon:1-3
Timestamp: 2025-08-18T16:13:31.606Z
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:
composer.jsonREADME.md
📚 Learning: 2025-08-18T15:47:45.406Z
Learnt from: terabytesoftw
Repo: php-forge/support PR: 11
File: tests/AssertTest.php:82-89
Timestamp: 2025-08-18T15:47:45.406Z
Learning: In PHP 8.1+, the RFC "Make Reflection setAccessible() a no-op" was implemented. This means ReflectionProperty and ReflectionMethod can access private and protected members automatically without requiring setAccessible(true) calls. The setAccessible() method still exists for backward compatibility but has no effect.
Applied to files:
tests/ReflectionHelperTest.phpREADME.mdsrc/ReflectionHelper.php
🧬 Code graph analysis (5)
src/DirectoryCleaner.php (1)
src/Exception/Message.php (1)
getMessage(55-58)
tests/LineEndingNormalizerTest.php (1)
src/LineEndingNormalizer.php (2)
LineEndingNormalizer(21-43)normalize(37-42)
tests/DirectoryCleanerTest.php (1)
src/DirectoryCleaner.php (2)
DirectoryCleaner(30-76)clean(44-75)
tests/ReflectionHelperTest.php (3)
src/ReflectionHelper.php (7)
ReflectionHelper(30-204)inaccessibleParentProperty(49-57)inaccessibleProperty(73-83)invokeMethod(100-110)invokeParentMethod(129-143)setInaccessibleParentProperty(161-175)setInaccessibleProperty(190-203)tests/Stub/TestBaseClass.php (1)
TestBaseClass(15-23)tests/Stub/TestClass.php (1)
TestClass(15-23)
tests/EnumDataProviderTest.php (2)
src/EnumDataProvider.php (3)
EnumDataProvider(24-100)attributeCases(43-66)tagCases(81-91)tests/Support/Provider/EnumDataProviderProvider.php (1)
EnumDataProviderProvider(17-64)
🪛 LanguageTool
README.md
[style] ~313-~313: Using many exclamation marks might seem excessive (in this case: 3 exclamation marks for a text that’s 1685 characters long)
Context: ...://codecov.io/github/php-forge/support) 
(EN_EXCESSIVE_EXCLAMATION)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: linter / Super Linter
🔇 Additional comments (21)
src/LineEndingNormalizer.php (1)
37-41: LGTM — normalization order is sound.src/Exception/Message.php (2)
32-39: Clean, type-safe message template.
55-57: Formatting helper looks correct.tests/LineEndingNormalizerTest.php (3)
27-33: Good coverage for literal\\nnormalization.
36-42: No-op behavior test is solid.
45-51: CRLF normalization test looks correct.src/DirectoryCleaner.php (1)
46-52: Exception message wiring is clear and consistent.tests/DirectoryCleanerTest.php (2)
27-57: Good end-to-end coverage for cleanup behavior.
64-69: Exception case test is precise.src/EnumDataProvider.php (3)
43-65: attributeCases() logic is clear and consistent.
81-88: tagCases() output structure looks correct.
93-98: normalizeValue() handles backed vs. unit enums well.tests/Stub/TestIntBackedEnum.php (1)
1-19: LGTM!Clean int-backed enum stub providing deterministic test values. The structure is consistent with the other test enums in this PR.
tests/Stub/TestEnum.php (1)
13-18: LGTM with minor observation.The enum case naming (uppercase
BAR,FOO) differs from the other test stubs (Bar,Foo). This appears intentional to provide test variety for thenormalizeValuelogic, which uses$enum->namefor unit enums.tests/Stub/TestBackedEnum.php (1)
1-19: LGTM!Clean string-backed enum stub that complements
TestIntBackedEnumfor comprehensive enum normalization testing.tests/Support/Provider/EnumDataProviderProvider.php (1)
17-50: LGTM!The data provider correctly supplies representative test cases covering:
- Unit enum instances (asHtml=false)
- HTML attribute generation (asHtml=true)
- Enum instances as attribute names
composer.json (2)
51-52: LGTM on mutation script updates.The simplified mutation flags (
--min-msi=100 --min-covered-msi=100) and static analysis integration look good for enforcing strict mutation testing standards.
17-18: Verify the version constraints for these dependencies with git history.
infection/infection: Range skips^0.31(was^0.27|^0.31, now^0.27|^0.32)maglnet/composer-require-checker: Changed from^4.7to^4.1— if confirmed, this is a downgrade that allows older minor versions (4.1–4.6)The current constraints are technically valid (maglnet is in 4.x versioning, and
^4.1will correctly resolve to the latest 4.20.0), but the version changes appear intentional. Please confirm these changes are intentional using git history.README.md (1)
1-324: Documentation is comprehensive and well-structured.The README provides clear examples for all new utilities (ReflectionHelper, LineEndingNormalizer, DirectoryCleaner, EnumDataProvider) with appropriate code samples demonstrating PHPUnit integration.
src/ReflectionHelper.php (1)
30-32: Good use of private constructor for utility class.The private constructor correctly prevents instantiation of this static-only helper class.
Based on learnings, no
setAccessible()calls are needed since PHP 8.1+ made them a no-op, and this implementation correctly omits them.tests/ReflectionHelperTest.php (1)
29-120: Well-structured tests with good coverage of core functionality.The test suite covers all six public methods of
ReflectionHelperwith clear assertions and descriptive failure messages. The use of stub classes (TestClass,TestBaseClass) for testing inheritance scenarios is appropriate.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
