fix(tests): Raise PHPStan level to 5.
#117
Conversation
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughRaises PHPStan from level 2 to 5, removes the phpstan baseline ignore rules, and updates code and tests to satisfy stricter static analysis: tightened boolean/string checks, refined regexes, small API-safe refactors, and added ComposerFallback tests. No public API signature regressions. Changes
Sequence Diagram(s)(Skipped — changes are configuration, validation tightening, and test updates rather than a new multi-component sequential feature.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-09-29T14:58:04.095ZApplied to files:
📚 Learning: 2026-01-23T11:09:08.789ZApplied to files:
🧬 Code graph analysis (1)src/Fallback/ComposerFallback.php (3)
🪛 PHPMD (2.15.0)src/Fallback/ComposerFallback.php143-143: Avoid using static access to class '\Foxy\Util\ConsoleUtil' in method 'restorePreviousLockFile'. (undefined) (StaticAccess) 176-182: The method restorePreviousLockFile uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them. (undefined) (ElseExpression) 184-184: Avoid using static access to class '\Composer\Filter\PlatformRequirementFilter\PlatformRequirementFilterFactory' in method 'restorePreviousLockFile'. (undefined) (StaticAccess) ⏰ 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). (2)
🔇 Additional comments (6)
✏️ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #117 +/- ##
===========================================
Coverage 100.00% 100.00%
- Complexity 347 359 +12
===========================================
Files 26 26
Lines 799 821 +22
===========================================
+ Hits 799 821 +22 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Config/Config.php (1)
127-137: Return type annotation is incorrect for all possible json_decode values.The error handling with
json_last_error()correctly detects invalid JSON, but the method's return type is declared asarraywhenjson_decode($value, true)can returnmixed—including strings, integers, floats, booleans, ornullfor valid JSON that isn't an object or array. For example, valid JSON like"text",123,true, ornullwill pass the error check but violate thearrayreturn type.Either change the return type to
mixedor add explicit validation to ensure the decoded value is an array before returning:if (!is_array($value)) { throw new RuntimeException(sprintf('The "%s" environment variable must be a JSON object or array', $environmentVariable)); }
🤖 Fix all issues with AI agents
In `@src/Fallback/ComposerFallback.php`:
- Around line 180-184: The current code always disables scripts by calling
$dispatcher->setRunScripts(false) before $installer->run(); change this to
respect the user's option by computing $runScripts and setting
$dispatcher->setRunScripts($runScripts) before calling $installer->run(); if the
EventDispatcher API exposes a previous state getter (e.g. isRunScripts() or
getRunScripts()), capture the original state, set it to $runScripts for the run,
then restore the original state after $installer->run() to avoid side effects.
In `@src/Json/JsonFormatter.php`:
- Around line 112-114: The anonymous callback currently declares a string return
type but calls mb_convert_encoding(pack('H*', $match[1]), 'UTF-8', 'UCS-2BE')
which can return false; update the closure used in JsonFormatter.php to either
(a) remove the explicit ": string" return type, or (b) handle the false case
defensively by checking the mb_convert_encoding() result and returning a safe
fallback (e.g., empty string or throw) so the closure always yields a string;
reference the anonymous static function, mb_convert_encoding, and pack('H*',
$match[1]) when making the change.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (23)
CHANGELOG.mdREADME.mdphpstan-baseline.neonphpstan.neonsrc/Asset/AbstractAssetManager.phpsrc/Asset/AssetManagerFinder.phpsrc/Config/Config.phpsrc/Converter/SemverUtil.phpsrc/Fallback/AssetFallback.phpsrc/Fallback/ComposerFallback.phpsrc/Foxy.phpsrc/Json/JsonFormatter.phpsrc/Solver/Solver.phpsrc/Util/AssetUtil.phpsrc/Util/ConsoleUtil.phptests/Config/ConfigTest.phptests/Fixtures/Asset/StubAssetManager.phptests/Fixtures/Util/AbstractProcessExecutorMock.phptests/FoxyTest.phptests/Solver/SolverTest.phptests/Util/AssetUtilTest.phptests/Util/ComposerUtilTest.phptests/Util/ConsoleUtilTest.php
💤 Files with no reviewable changes (3)
- phpstan-baseline.neon
- tests/Util/AssetUtilTest.php
- tests/Util/ConsoleUtilTest.php
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: terabytesoftw
Repo: php-forge/foxy PR: 115
File: tests/Fallback/AssetFallbackTest.php:158-189
Timestamp: 2026-01-23T11:22:00.118Z
Learning: User terabytesoftw prefers not to use aliases when importing classes in PHP. They are comfortable with either fully-qualified class names or direct imports without aliases.
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: 2026-01-22T11:42:32.878Z
Learnt from: terabytesoftw
Repo: php-forge/foxy PR: 111
File: tests/Fallback/AssetFallbackTest.php:98-98
Timestamp: 2026-01-22T11:42:32.878Z
Learning: When using xepozz/internal-mocker in PHP tests, the library's namespaced wrapper records and matches function calls with all default parameters filled in. To ensure proper matching, always specify every parameter in MockerState::addCondition(), including defaults. For example, a call to file_get_contents($path) is observed as file_get_contents($path, false, null, 0, null). Use the same complete argument list (including default values) when adding conditions for mocks in tests like tests/Fallback/AssetFallbackTest.php.
Applied to files:
tests/Config/ConfigTest.phptests/Fixtures/Asset/StubAssetManager.phptests/FoxyTest.phptests/Fixtures/Util/AbstractProcessExecutorMock.phptests/Util/ComposerUtilTest.phptests/Solver/SolverTest.php
📚 Learning: 2026-01-23T11:22:00.118Z
Learnt from: terabytesoftw
Repo: php-forge/foxy PR: 115
File: tests/Fallback/AssetFallbackTest.php:158-189
Timestamp: 2026-01-23T11:22:00.118Z
Learning: In PHP tests (e.g., tests/Fallback/AssetFallbackTest.php) avoid using import aliases. Prefer either importing the full namespace without an alias or using fully-qualified class names directly. For example, avoid statements like 'use Some\Long\Namespace as Alias;' and either 'use Some\\Long\\Namespace;' or reference 'Some\\Long\\Namespace' in code. This guideline applies to PHP files under tests and improves readability and refactor safety.
Applied to files:
tests/Config/ConfigTest.phptests/Fixtures/Asset/StubAssetManager.phptests/FoxyTest.phptests/Fixtures/Util/AbstractProcessExecutorMock.phptests/Util/ComposerUtilTest.phptests/Solver/SolverTest.php
📚 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:
phpstan.neon
🧬 Code graph analysis (6)
src/Solver/Solver.php (3)
src/Config/Config.php (1)
get(37-55)src/Asset/AssetManagerInterface.php (1)
AssetManagerInterface(11-87)src/Solver/SolverInterface.php (1)
SolverInterface(10-26)
tests/Config/ConfigTest.php (1)
src/Exception/RuntimeException.php (1)
RuntimeException(7-7)
src/Foxy.php (1)
src/Config/Config.php (1)
get(37-55)
src/Config/Config.php (1)
src/Exception/RuntimeException.php (1)
RuntimeException(7-7)
src/Fallback/AssetFallback.php (3)
src/Fallback/ComposerFallback.php (1)
restore(47-66)src/Fallback/FallbackInterface.php (1)
restore(12-12)src/Config/Config.php (1)
get(37-55)
src/Fallback/ComposerFallback.php (3)
src/Fallback/AssetFallback.php (1)
restore(28-55)src/Config/Config.php (1)
get(37-55)src/Util/ConsoleUtil.php (2)
ConsoleUtil(17-79)getPreferredInstallOptions(48-78)
🪛 PHPMD (2.15.0)
tests/Fixtures/Asset/StubAssetManager.php
21-21: Avoid variables with short names like $io. Configured minimum length is 3. (undefined)
(ShortVariable)
33-33: Avoid variables with short names like $fs. Configured minimum length is 3. (undefined)
(ShortVariable)
tests/FoxyTest.php
113-113: Avoid excessively long variable names like $assetManagersProperty. Keep variable name length under 20. (undefined)
(LongVariable)
114-114: Avoid excessively long variable names like $originalAssetManagers. Keep variable name length under 20. (undefined)
(LongVariable)
src/Fallback/ComposerFallback.php
138-138: Avoid using static access to class '\Foxy\Util\ConsoleUtil' in method 'restorePreviousLockFile'. (undefined)
(StaticAccess)
171-177: The method restorePreviousLockFile uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them. (undefined)
(ElseExpression)
179-179: Avoid using static access to class '\Composer\Filter\PlatformRequirementFilter\PlatformRequirementFilterFactory' in method 'restorePreviousLockFile'. (undefined)
(StaticAccess)
tests/Solver/SolverTest.php
138-138: Avoid variables with short names like $rm. Configured minimum length is 3. (undefined)
(ShortVariable)
⏰ 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). (11)
- GitHub Check: linter / Super Linter
- GitHub Check: phpunit / PHP 8.3-ubuntu-latest
- GitHub Check: phpunit / PHP 8.2-windows-2022
- GitHub Check: mutation / PHP 8.5-ubuntu-latest
- GitHub Check: phpunit / PHP 8.5-windows-2022
- GitHub Check: phpunit / PHP 8.3-windows-2022
- GitHub Check: phpunit / PHP 8.4-ubuntu-latest
- GitHub Check: phpunit / PHP 8.4-windows-2022
- GitHub Check: phpunit / PHP 8.5-ubuntu-latest
- GitHub Check: linter / Super Linter
- GitHub Check: mutation / PHP 8.5-ubuntu-latest
🔇 Additional comments (33)
src/Json/JsonFormatter.php (5)
27-27: LGTM!Good cleanup —
\walready includes[a-zA-Z0-9_], so removing the redundant\d_is correct. The unescaped]is also valid outside a character class.
29-29: LGTM!Simplified regex syntax —
( +)is equivalent to([ ]+)for matching one or more spaces.
67-72: LGTM!Safe simplification —
preg_match_allalways populates$matches[1]as an array (empty when no matches), so the direct return is correct.
79-89: LGTM!More precise condition — checking
isset($matches[1])directly validates the capture group we use on line 85, which is cleaner than checking if the entire matches array is non-empty.
146-146: LGTM!Consistent regex cleanup — the unescaped
]is valid outside a character class, matching the pattern style used in the constants above.tests/Fixtures/Util/AbstractProcessExecutorMock.php (1)
19-23: Docblock now reflects nullable output parameter.Good alignment with the method signature and expected values.
README.md (1)
123-127: Badge update looks good.Quality badge now matches PHPStan level 5.
CHANGELOG.md (1)
3-19: Changelog entry is clear and consistent.Thanks for recording the PHPStan level bump.
src/Asset/AssetManagerFinder.php (1)
21-25: Constructor delegation to addManager is fine.Keeps the type check in one place.
src/Util/ConsoleUtil.php (1)
69-74: LGTM — explicit option handling is clearer.tests/Fixtures/Asset/StubAssetManager.php (1)
18-33: LGTM — readonly promotion is clean for the fixture.src/Asset/AbstractAssetManager.php (1)
63-66: LGTM — clearer guards and return value; command formatting is cleaner.Also applies to: 126-144, 175-176, 195-203, 245-251
src/Converter/SemverUtil.php (1)
44-45: LGTM — cleanup is straightforward and consistent.Also applies to: 73-87, 129-130
src/Fallback/ComposerFallback.php (1)
41-177: LGTM — option parsing refactor is clearer and more explicit.src/Util/AssetUtil.php (1)
95-97: No guard needed—$installPathis already protected.The concatenation at line 96 is already within the
if (null !== $installPath)guard at line 86, so$installPathcannot be null when this code executes. The proposed fix is unnecessary.Likely an incorrect or invalid review comment.
src/Foxy.php (1)
126-130: LGTM! Stricter enabled check is appropriate for PHPStan level 5.The explicit type comparisons handle the various ways
enabledcan be configured (boolean, integer, or string from environment variables). This pattern is consistent with the same check insrc/Solver/Solver.phpandsrc/Fallback/AssetFallback.php.Consider extracting this repeated pattern into a helper method if it appears in more places, but for now the duplication is acceptable.
src/Solver/Solver.php (3)
52-56: LGTM! Inverted enabled check correctly short-circuits when disabled.The negated condition (
!== true && !== 1 && !== '1') properly returns early when the feature is disabled, maintaining consistency with the same validation pattern used elsewhere in the codebase.
71-75: LGTM! Strict null check aligns with the typed property.Using
!== nullis the correct approach given theFallbackInterface|nullproperty type, and satisfies PHPStan's stricter type analysis.
98-101: LGTM! Robust filename validation.The combined
is_string($filename) && $filename !== ''check properly validates that the filename is both the expected type and non-empty before proceeding with file operations.tests/Util/ComposerUtilTest.php (1)
31-43: LGTM! Clean use ofexpectNotToPerformAssertions().Using
expectNotToPerformAssertions()for the valid case is the idiomatic PHPUnit approach when the test's purpose is to verify that code executes without throwing an exception.tests/Config/ConfigTest.php (3)
142-151: LGTM! Cleaner message comparison with precomputed strings.Extracting the expected messages into variables improves readability and avoids repeated
sprintfcalls within the callback.
161-163: Edge case:envKeybecomes empty string when'='is not found.If
$envdoesn't contain'=', then$envKeyPosisfalseand$envKeybecomes''. Callingputenv('')is a no-op, but this seems unintentional for the cleanup logic.However, given the test data always includes
'='in env strings (e.g.,'FOXY__ENV_BOOLEAN=false'), this edge case won't occur in practice. The current implementation is acceptable.
211-213: LGTM! More specific exception type.Using
RuntimeException(fromFoxy\Exception) is more appropriate than the genericExceptionand aligns with the exception types used elsewhere in the codebase.src/Fallback/AssetFallback.php (3)
25-25: LGTM! Correct use of null coalescing operator.Changing from
?:to??is the right fix. The Elvis operator (?:) treats any falsy value (including a hypothetical Filesystem subclass evaluating to falsy) as triggering the default, whereas??only triggers onnull, which matches theFilesystem|nullparameter type.
30-34: LGTM! Consistent fallback-asset check pattern.The strict comparison pattern matches
ComposerFallback::restore()(shown in relevant snippets at lines 46-65) and maintains consistency across fallback implementations.
48-54: LGTM! Guard against writing empty content.The additional
$this->originalContent !== ''check prevents unnecessarily recreating an empty file when the original content was empty, which is a sensible defensive measure.tests/FoxyTest.php (6)
42-50: LGTM! Clean test structure withexpectNotToPerformAssertions().Using
expectNotToPerformAssertions()correctly expresses the test intent: verifying thatactivate()andinit()execute without throwing exceptions.
86-99: LGTM! Test properly validates the install-on-event flow.The test correctly sets up mock expectations and verifies the
initOnInstallbehavior when the package name matches.
111-131: Acceptable long variable names for clarity.The static analysis hints flag
$assetManagersPropertyand$originalAssetManagersas exceeding 20 characters. However, these descriptive names improve readability in reflection-heavy code. The trade-off favors clarity over brevity here.
152-159: LGTM! Deactivate test correctly uses no-assertion expectation.Since
deactivate()is a no-op method,expectNotToPerformAssertions()is the appropriate way to validate it runs without error.
184-191: LGTM! Uninstall test follows the same pattern.Consistent with the other no-op method tests.
193-224: LGTM! Localized mock improves test isolation.Changing
$this->composerConfigto a local$composerConfigvariable is appropriate since it's only used withinsetUp()to configure the composer mock. This simplifies the test class by removing an unnecessary class property.tests/Solver/SolverTest.php (1)
138-144: Clean simplification of RepositoryManager setup.The removal of the conditional
class_exists(HttpDownloader::class)check in favor of always using the 3-argument constructor is a good improvement that aligns with the current Composer 2.x API.The static analysis tool (PHPMD) flagged
$rmas a short variable name. While$rmis idiomatic in the Composer ecosystem, you could rename it to$repoManagerfor consistency with the project's naming conventions if desired.[approve_code_changes, suggest_optional_refactor]
Optional: Rename to satisfy PHPMD
- $rm = new RepositoryManager( + $repoManager = new RepositoryManager( $this->io, $this->composerConfig, new HttpDownloader($this->io, $this->composerConfig), ); - $rm->setLocalRepository($this->localRepo); + $repoManager->setLocalRepository($this->localRepo); - $this->composer->expects(self::any())->method('getRepositoryManager')->willReturn($rm); + $this->composer->expects(self::any())->method('getRepositoryManager')->willReturn($repoManager);
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
…omposerFallbackTest` class.
…pdate restore tests.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/Fallback/ComposerFallbackTest.php`:
- Around line 68-139: Extract the duplicated setup in
testRestoreWithIgnorePlatformReqs and testRestoreWithIgnorePlatformReq into a
private helper like buildAndRunRestore(string $optionName, mixed $optionValue)
that creates composer.json/ composer.lock (using LockerUtil::getLocker), sets up
$this->input->getOption callback, config mocks (getEventDispatcher,
getRepositoryManager, getInstallationManager), stubs $this->composer->getLocker
and getConfig (with config->get returning vendor-dir), expects $this->io->write
and $this->installer->run, then calls $this->composerFallback->save() and
$this->composerFallback->restore(); update both tests to call this helper
passing only the option-specific args so they remain focused on differences.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/Fallback/ComposerFallbackTest.php
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: terabytesoftw
Repo: php-forge/foxy PR: 115
File: tests/Fallback/AssetFallbackTest.php:158-189
Timestamp: 2026-01-23T11:22:00.118Z
Learning: User terabytesoftw prefers not to use aliases when importing classes in PHP. They are comfortable with either fully-qualified class names or direct imports without aliases.
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: 2026-01-22T11:42:32.878Z
Learnt from: terabytesoftw
Repo: php-forge/foxy PR: 111
File: tests/Fallback/AssetFallbackTest.php:98-98
Timestamp: 2026-01-22T11:42:32.878Z
Learning: When using xepozz/internal-mocker in PHP tests, the library's namespaced wrapper records and matches function calls with all default parameters filled in. To ensure proper matching, always specify every parameter in MockerState::addCondition(), including defaults. For example, a call to file_get_contents($path) is observed as file_get_contents($path, false, null, 0, null). Use the same complete argument list (including default values) when adding conditions for mocks in tests like tests/Fallback/AssetFallbackTest.php.
Applied to files:
tests/Fallback/ComposerFallbackTest.php
📚 Learning: 2026-01-23T11:22:00.118Z
Learnt from: terabytesoftw
Repo: php-forge/foxy PR: 115
File: tests/Fallback/AssetFallbackTest.php:158-189
Timestamp: 2026-01-23T11:22:00.118Z
Learning: In PHP tests (e.g., tests/Fallback/AssetFallbackTest.php) avoid using import aliases. Prefer either importing the full namespace without an alias or using fully-qualified class names directly. For example, avoid statements like 'use Some\Long\Namespace as Alias;' and either 'use Some\\Long\\Namespace;' or reference 'Some\\Long\\Namespace' in code. This guideline applies to PHP files under tests and improves readability and refactor safety.
Applied to files:
tests/Fallback/ComposerFallbackTest.php
🧬 Code graph analysis (1)
tests/Fallback/ComposerFallbackTest.php (2)
src/Util/LockerUtil.php (2)
LockerUtil(12-26)getLocker(17-25)src/Fallback/FallbackInterface.php (2)
save(17-17)restore(12-12)
🪛 GitHub Actions: ecs
tests/Fallback/ComposerFallbackTest.php
[error] 1-1: PHPUnit test failures detected: mismatch in method names and data providers (getIgnorePlatformReqsData vs getIgnorePlatformReqData). Test expectations may be out of sync with code.
🪛 PHPMD (2.15.0)
tests/Fallback/ComposerFallbackTest.php
106-106: Avoid variables with short names like $ed. Configured minimum length is 3. (undefined)
(ShortVariable)
110-110: Avoid variables with short names like $rm. Configured minimum length is 3. (undefined)
(ShortVariable)
114-114: Avoid variables with short names like $im. Configured minimum length is 3. (undefined)
(ShortVariable)
119-119: Avoid using static access to class '\Foxy\Util\LockerUtil' in method 'testRestoreWithIgnorePlatformReqs'. (undefined)
(StaticAccess)
180-180: Avoid variables with short names like $ed. Configured minimum length is 3. (undefined)
(ShortVariable)
184-184: Avoid variables with short names like $rm. Configured minimum length is 3. (undefined)
(ShortVariable)
188-188: Avoid variables with short names like $im. Configured minimum length is 3. (undefined)
(ShortVariable)
193-193: Avoid using static access to class '\Foxy\Util\LockerUtil' in method 'testRestoreWithIgnorePlatformReq'. (undefined)
(StaticAccess)
⏰ 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). (16)
- GitHub Check: phpunit / PHP 8.1-windows-2022
- GitHub Check: phpunit / PHP 8.5-ubuntu-latest
- GitHub Check: phpunit / PHP 8.4-ubuntu-latest
- GitHub Check: phpunit / PHP 8.2-windows-2022
- GitHub Check: phpunit / PHP 8.3-ubuntu-latest
- GitHub Check: phpunit / PHP 8.5-windows-2022
- GitHub Check: phpunit / PHP 8.4-windows-2022
- GitHub Check: phpunit / PHP 8.2-ubuntu-latest
- GitHub Check: phpunit / PHP 8.3-windows-2022
- GitHub Check: phpunit / PHP 8.1-ubuntu-latest
- GitHub Check: mutation / PHP 8.5-ubuntu-latest
- GitHub Check: linter / Super Linter
- GitHub Check: mutation / PHP 8.5-ubuntu-latest
- GitHub Check: phpunit / PHP 8.3-ubuntu-latest
- GitHub Check: phpunit / PHP 8.4-windows-2022
- GitHub Check: linter / Super Linter
🔇 Additional comments (2)
tests/Fallback/ComposerFallbackTest.php (2)
141-213: Good coverage of the ignore-platform-req fallback path.Explicitly nulling
ignore-platform-reqsbefore settingignore-platform-reqmakes the precedence behavior clear and testable.
42-56: The data provider names and their@dataProviderannotations are correctly matched in the codebase. No mismatch exists.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
…oreWithIgnorePlatformReqs` by extracting common setup logic.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Fallback/ComposerFallback.php`:
- Around line 124-126: The current check in ComposerFallback (using $lockData
and $hasPackage) only treats 'packages' as a valid lock presence; change the
logic to also consider 'packages-dev' so projects with only dev dependencies are
treated as locked. Update the $hasPackage computation in ComposerFallback.php to
return true when either isset($lockData['packages']) and non-empty OR
isset($lockData['packages-dev']) and non-empty (use the same is_array/empty
checks you already use) so the method that returns $isLocked && $hasPackage will
not erroneously remove vendor/ for dev-only lock files.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/Fallback/ComposerFallback.phptests/Fallback/ComposerFallbackTest.php
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: terabytesoftw
Repo: php-forge/foxy PR: 115
File: tests/Fallback/AssetFallbackTest.php:158-189
Timestamp: 2026-01-23T11:22:00.118Z
Learning: User terabytesoftw prefers not to use aliases when importing classes in PHP. They are comfortable with either fully-qualified class names or direct imports without aliases.
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:
tests/Fallback/ComposerFallbackTest.php
📚 Learning: 2026-01-22T11:42:32.878Z
Learnt from: terabytesoftw
Repo: php-forge/foxy PR: 111
File: tests/Fallback/AssetFallbackTest.php:98-98
Timestamp: 2026-01-22T11:42:32.878Z
Learning: When using xepozz/internal-mocker in PHP tests, the library's namespaced wrapper records and matches function calls with all default parameters filled in. To ensure proper matching, always specify every parameter in MockerState::addCondition(), including defaults. For example, a call to file_get_contents($path) is observed as file_get_contents($path, false, null, 0, null). Use the same complete argument list (including default values) when adding conditions for mocks in tests like tests/Fallback/AssetFallbackTest.php.
Applied to files:
tests/Fallback/ComposerFallbackTest.php
📚 Learning: 2026-01-23T11:22:00.118Z
Learnt from: terabytesoftw
Repo: php-forge/foxy PR: 115
File: tests/Fallback/AssetFallbackTest.php:158-189
Timestamp: 2026-01-23T11:22:00.118Z
Learning: In PHP tests (e.g., tests/Fallback/AssetFallbackTest.php) avoid using import aliases. Prefer either importing the full namespace without an alias or using fully-qualified class names directly. For example, avoid statements like 'use Some\Long\Namespace as Alias;' and either 'use Some\\Long\\Namespace;' or reference 'Some\\Long\\Namespace' in code. This guideline applies to PHP files under tests and improves readability and refactor safety.
Applied to files:
tests/Fallback/ComposerFallbackTest.php
📚 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:
src/Fallback/ComposerFallback.php
🧬 Code graph analysis (2)
tests/Fallback/ComposerFallbackTest.php (7)
src/Fallback/ComposerFallback.php (2)
save(68-82)restore(47-66)src/Fallback/AssetFallback.php (2)
save(57-70)restore(28-55)src/Fallback/FallbackInterface.php (2)
save(17-17)restore(12-12)src/Util/LockerUtil.php (2)
LockerUtil(12-26)getLocker(17-25)src/Config/Config.php (1)
Config(21-207)tests/Fallback/AssetFallbackTest.php (2)
testRestore(44-66)AssetFallbackTest(21-190)tests/FoxyTest.php (1)
FoxyTest(30-238)
src/Fallback/ComposerFallback.php (3)
src/Fallback/AssetFallback.php (1)
restore(28-55)src/Util/ConsoleUtil.php (2)
ConsoleUtil(17-79)getPreferredInstallOptions(48-78)src/Asset/AbstractAssetManager.php (1)
run(116-176)
🪛 PHPMD (2.15.0)
tests/Fallback/ComposerFallbackTest.php
315-315: Avoid using static access to class '\Foxy\Util\LockerUtil' in method 'setupRestoreEnvironment'. (undefined)
(StaticAccess)
src/Fallback/ComposerFallback.php
138-138: Avoid using static access to class '\Foxy\Util\ConsoleUtil' in method 'restorePreviousLockFile'. (undefined)
(StaticAccess)
171-177: The method restorePreviousLockFile uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them. (undefined)
(ElseExpression)
179-179: Avoid using static access to class '\Composer\Filter\PlatformRequirementFilter\PlatformRequirementFilterFactory' in method 'restorePreviousLockFile'. (undefined)
(StaticAccess)
⏰ 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). (14)
- GitHub Check: phpunit / PHP 8.5-windows-2022
- GitHub Check: phpunit / PHP 8.2-windows-2022
- GitHub Check: phpunit / PHP 8.1-windows-2022
- GitHub Check: phpunit / PHP 8.3-ubuntu-latest
- GitHub Check: phpunit / PHP 8.4-windows-2022
- GitHub Check: phpunit / PHP 8.3-windows-2022
- GitHub Check: linter / Super Linter
- GitHub Check: phpunit / PHP 8.1-ubuntu-latest
- GitHub Check: phpunit / PHP 8.5-ubuntu-latest
- GitHub Check: easy-coding-standard / PHP 8.5-ubuntu-latest
- GitHub Check: mutation / PHP 8.5-ubuntu-latest
- GitHub Check: mutation / PHP 8.5-ubuntu-latest
- GitHub Check: linter / Super Linter
- GitHub Check: easy-coding-standard / PHP 8.5-ubuntu-latest
🔇 Additional comments (6)
src/Fallback/ComposerFallback.php (3)
41-41: Null-coalescing init looks correct.The switch to
??avoids treating falsy-but-valid Filesystem instances as null.
49-52: Strict truthy check is consistent.This aligns
fallback-composerhandling with explicit boolean-like values.
138-183: Option normalization and platform-req handling look solid.The unified boolean checks and the
ignore-platform-req(s)resolution are clear and testable.tests/Fallback/ComposerFallbackTest.php (3)
42-56: Data providers are clear and focused.Nice separation of
ignore-platform-reqvsignore-platform-reqscases.
152-197: Tests are concise and cover both option paths.Good use of providers to validate both option names and list/boolean values.
276-330: Shared setup helper improves readability.Centralizing the restore environment setup makes the tests easier to maintain.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
…ackages in lock data.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Pull Request