Discover providers from the project itself. - #84
Conversation
📝 WalkthroughWalkthroughManifest recompilation now checks the project root ChangesManifest Root composer.json Handling
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
♻️ Duplicate comments (1)
src/Manifests/ContainerMixinManifest.php (1)
145-147: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSame
filemtimeguard issue asPackageManifest::shouldRecompile().
filemtime($this->basePath.'/composer.json')will emit a PHP warning if the file doesn't exist. Apply the sameis_file()guard suggested forPackageManifest::shouldRecompile().🛡️ Proposed fix
public function shouldRecompile(): bool { return !is_file($this->containerMixinPath) || // We check here if the manifest has been generated before changing the installed.json composer file or the project composer.json - filemtime($this->containerMixinPath) <= filemtime($this->vendorPath.'/composer/installed.json') || - filemtime($this->containerMixinPath) <= filemtime($this->basePath.'/composer.json'); + (is_file($this->vendorPath.'/composer/installed.json') && filemtime($this->containerMixinPath) <= filemtime($this->vendorPath.'/composer/installed.json')) || + (is_file($this->basePath.'/composer.json') && filemtime($this->containerMixinPath) <= filemtime($this->basePath.'/composer.json')); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Manifests/ContainerMixinManifest.php` around lines 145 - 147, The `ContainerMixinManifest` freshness check has the same unsafe `filemtime` usage as `PackageManifest::shouldRecompile()`, and `filemtime($this->basePath.'/composer.json')` can warn when the file is missing. Update the logic around the manifest rebuild guard to use an `is_file()` check before calling `filemtime()` for the project composer file, keeping the existing comparison behavior in `ContainerMixinManifest` while avoiding warnings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Manifests/PackageManifest.php`:
- Around line 144-147: The shouldRecompile() logic in PackageManifest currently
calls filemtime() on composer.json without verifying the file exists, which can
trigger warnings on missing layouts. Update shouldRecompile() to mirror the
build() guard by checking is_file() before any filemtime() call on the basePath
composer.json (and any other potentially absent paths), and only compare mtimes
when the files are present.
---
Duplicate comments:
In `@src/Manifests/ContainerMixinManifest.php`:
- Around line 145-147: The `ContainerMixinManifest` freshness check has the same
unsafe `filemtime` usage as `PackageManifest::shouldRecompile()`, and
`filemtime($this->basePath.'/composer.json')` can warn when the file is missing.
Update the logic around the manifest rebuild guard to use an `is_file()` check
before calling `filemtime()` for the project composer file, keeping the existing
comparison behavior in `ContainerMixinManifest` while avoiding warnings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0bb7e42d-a324-4d6a-bce2-f9e069c8f188
📒 Files selected for processing (3)
src/Manifests/ContainerMixinManifest.phpsrc/Manifests/PackageManifest.phptests/Support/composer.json
|
Interesting changes, thank you. I added some little feedbacks but the main thing is cool ! |
Cover the root composer.json branch added to PackageManifest::build(): providers declared by the project itself, merge with the installed packages ones, the "root" fallback key when the project has no name, and the ignored cases (no extra.faker, no composer.json at all). Also cover the new project composer.json freshness check in both PackageManifest::shouldRecompile() and ContainerMixinManifest::shouldRecompile().
|
Reviewed, and I pushed the missing tests onto the branch (8841a31) so we can close @martinsoenen's point. DocumentationCompanion doc PR: xefi/faker-doc#43 — a Project discovery section next to the existing Package discovery one, plus a pointer from the Custom Extension page, which until now documented Tests added
Each one is built on a throwaway project directory ( I checked they actually pin the behaviour: reverting Two notes, neither blocking
Nice feature — this closes a real gap for apps that have extensions but no package to publish them in. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/Unit/PackageManifestTest.php`:
- Around line 195-219: The PackageManifest test must cover projects without a
root composer.json during recompilation checks. In
PackageManifest::shouldRecompile(), guard the composer.json filemtime lookup
with is_file() so missing files do not emit warnings, then update
testProjectWithoutComposerFileIsIgnored() to assert the expected
shouldRecompile() result after build().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c33d5371-87cb-4bad-8494-7fd6063e9d5a
📒 Files selected for processing (3)
tests/Support/Concerns/CreatesTemporaryProjects.phptests/Unit/ContainerMixinManifestTest.phptests/Unit/PackageManifestTest.php
| public function testProjectWithoutComposerFileIsIgnored() | ||
| { | ||
| $projectPath = $this->createTemporaryProject(null, [ | ||
| [ | ||
| 'name' => 'xefi/faker-number', | ||
| 'extra' => [ | ||
| 'faker' => [ | ||
| 'providers' => [TestServiceProvider::class], | ||
| ], | ||
| ], | ||
| ], | ||
| ]); | ||
|
|
||
| $manifest = new PackageManifest($projectPath, $projectPath.'/packages.php'); | ||
| $manifest->build(); | ||
|
|
||
| $this->assertEquals( | ||
| [ | ||
| 'xefi/faker-number' => [ | ||
| 'providers' => [TestServiceProvider::class], | ||
| ], | ||
| ], | ||
| require $projectPath.'/packages.php' | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle a missing root composer.json in shouldRecompile().
This test declares that a project without composer.json is supported, but it only calls build(). PackageManifest::shouldRecompile() unconditionally calls filemtime($this->basePath.'/composer.json'), which emits a warning when this file is absent.
Guard that timestamp check with is_file(). Add a shouldRecompile() assertion here after build().
🧰 Tools
🪛 OpenGrep (1.26.0)
[ERROR] 217-217: Dynamic file path passed to include/require. This can lead to local or remote file inclusion. Use a fixed allowlist of paths.
(coderabbit.file-inclusion.php-dynamic-include)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/Unit/PackageManifestTest.php` around lines 195 - 219, The
PackageManifest test must cover projects without a root composer.json during
recompilation checks. In PackageManifest::shouldRecompile(), guard the
composer.json filemtime lookup with is_file() so missing files do not emit
warnings, then update testProjectWithoutComposerFileIsIgnored() to assert the
expected shouldRecompile() result after build().
GautierDele
left a comment
GautierDele
left a comment
There was a problem hiding this comment.
Thanks a lot @sukei, merging
3 participants
The auto-discovery feature of installed packages is nice, however it fails to discover project-local providers and extensions. This PR address this issue by reading the project
composer.jsonfile too.Summary by CodeRabbit
New Features
extra.fakerconfiguration from the root Composer file.Bug Fixes