Harden FakeQuery release baseline#2
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR adds factory-backed hydration and PostQueryInterface (Ray.MediaQuery ^1.1) select-result support, makes JsonHydrator DbQuery-aware, implements recursive fixture validation, updates CI/composer/README, and adds tests/fixtures for static and injected factory flows and post-query selection hydration. ChangesFactory hydration and PostQueryInterface result flow
sequenceDiagram
participant Caller
participant Interceptor
participant Filesystem
participant JsonHydrator
participant Injector
Caller->>Interceptor: invoke(queryMethod, DbQuery)
Interceptor->>Filesystem: readContent/readJsonl(queryId)
Filesystem-->>Interceptor: json/jsonl data
Interceptor->>JsonHydrator: hydrate(data, entityClass, isRow?, DbQuery)
JsonHydrator->>Injector: resolve factory (if configured)
Injector-->>JsonHydrator: factory callable or null
JsonHydrator-->>Interceptor: entity or PostQuery wrapper
Interceptor-->>Caller: return result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Downstream verification completed in koriym/BEAR.AppKata#2. Verified usage:
BEAR.AppKata checks after push:
Ray.FakeQuery checks are also passing for PHP 8.2, 8.3, 8.4, 8.5 and Quality. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
composer.json (1)
106-108: ⚡ Quick winConsider using
"8.2"instead of"8.2.0"for the platform PHP version.The
platform.phpconstraint is set to the exact version8.2.0. While this is functionally valid, it's less common than omitting the patch version (e.g.,"php": "8.2"). Using the minor version only provides more flexibility with patch releases while still constraining the major and minor versions for dependency resolution.🤖 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 `@composer.json` around lines 106 - 108, The composer.json "platform" PHP constraint currently pins the exact patch version ("php": "8.2.0"); update the value to the minor-only constraint "8.2" (i.e., change platform -> php from "8.2.0" to "8.2") so dependency resolution allows any 8.2.x patch release while still constraining major/minor; locate the "platform" -> "php" entry in composer.json to make this change.src/FakeQueryModule.php (1)
51-51: 💤 Low valueHard-coding
'factory'makes the factory method name a global, undocumented convention.
#[FactoryMethod]is bound to the literal string'factory', which forces everyDbQuery(factory: ...)target across the entire app to expose a method named exactlyfactory. This is fine as a v1.0 convention, but it's worth surfacing in the README/documentation and ideally driving it from a constant (e.g.,JsonHydrator::DEFAULT_FACTORY_METHOD) so the string lives in exactly one place.🤖 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/FakeQueryModule.php` at line 51, The binding currently hard-codes the factory method name ('factory') via FactoryMethod::class in FakeQueryModule (->toInstance('factory')), which makes the convention global and undocumented; change the literal to a single shared constant (e.g. replace 'factory' with JsonHydrator::DEFAULT_FACTORY_METHOD or another appropriately named constant on the hydrator/DbQuery target) and update the README/docs to document the #[FactoryMethod] convention so the string/value lives in one place and the convention is explicit.src/FakeQueryInterceptor.php (1)
113-126: 💤 Low value
readContent()ignores the nullable-return convention used elsewhere.The standard query path (lines 74–80) honours
isNullable($returnType)and returnsnullwhen a fixture file is missing on a nullable return type, butselectPostQuery()→readContent()unconditionally throwsFakeJsonNotFoundException. If a method declares?SomePostQueryas its return type, behaviour will diverge from regular row/list queries. If this asymmetry is intentional (containers are always required), please document it onreadContent()/selectPostQuery()so future maintainers don't "fix" it inconsistently.🤖 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/FakeQueryInterceptor.php` around lines 113 - 126, readContent() currently always throws FakeJsonNotFoundException when the fixture file is missing, which diverges from the standard query path that uses isNullable($returnType) to return null for nullable returns; update the code so selectPostQuery() (or readContent()) checks the return type nullability via isNullable($returnType) and returns null when the file is missing and the return type is nullable, otherwise keep throwing FakeJsonNotFoundException; alternatively, if container fixtures must always exist, add a clear docblock to readContent() and selectPostQuery() stating that container post queries are required and will always throw when missing to prevent future unintended changes.
🤖 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 @.planning/progress.md:
- Line 43: Add a blank line between the "Test Results" heading and the table row
starting with "| Date | Command | Result | Notes |" so the table is surrounded
by a blank line per markdownlint MD058; locate the "Test Results" heading and
insert an empty line immediately above the table (the line containing "| Date |
Command | Result | Notes |") to satisfy the lint rule.
In @.planning/task_plan.md:
- Line 120: Add a single blank line immediately before the "Errors Encountered"
markdown table header (the line starting with "| Error | Attempt | Resolution
|") so the table is separated from preceding content and satisfies markdownlint
MD058; locate the table header in .planning/task_plan.md and insert one empty
line above it.
In `@src/FakeQueryInterceptor.php`:
- Around line 99-105: selectPostQuery currently instantiates the post-query via
Reflection and constructor args which breaks Ray.MediaQuery v1.1's contract;
instead build a PostQueryContext using the hydrated rows returned by
$this->hydrator->hydrate($this->readJsonl($dbQuery), $entityClass, false,
$dbQuery) plus the upstream context (statement, PDO, bind values) and then call
the static factory $postQueryClass::fromContext($context) (referencing
PostQueryInterface::fromContext and PostQueryContext) rather than newInstance;
update selectPostQuery to create and pass that PostQueryContext to the static
fromContext method so implementations like TodoSelection and others follow the
documented contract.
In `@src/FakeQueryModule.php`:
- Around line 72-91: The recursive directory iteration in configure() using new
RecursiveDirectoryIterator($this->fakeDir, FilesystemIterator::SKIP_DOTS) will
throw UnexpectedValueException when $this->fakeDir is missing or unreadable;
change configure() to first check is_dir($this->fakeDir) (or is_readable) and
either return early (skip scanning) or throw a clear, actionable exception that
references $this->fakeDir, so the failure mode matches the previous glob()
behavior and avoids confusing DI-time stack traces from
RecursiveDirectoryIterator; update any tests or callers that expect
UnknownFakeJsonException to still be thrown for missing files inside a valid
directory.
- Around line 94-100: queryIdFromFile() breaks when $this->fakeDir has a
trailing slash; normalize the stored fakeDir once in the class constructor
(e.g., trim trailing DIRECTORY_SEPARATOR and '/' from the promoted property or
remove readonly and rtrim in __construct) so later calls to queryIdFromFile()
can safely use substr($file->getPathname(), strlen($this->fakeDir) + 1) or,
better, compute the offset from strlen($this->fakeDir) and only add 1 when the
stored fakeDir does not end with a separator; ensure you use DIRECTORY_SEPARATOR
(and '/' for cross-platform) when trimming and keep the existing
str_replace(DIRECTORY_SEPARATOR, '/', $queryId) for final normalization.
In `@src/JsonHydrator.php`:
- Around line 117-119: The current check in JsonHydrator that returns null when
a configured factory can't be resolved hides misconfigurations; update the logic
so that when DbQuery::factory is non-empty but the referenced class or method
does not exist (the branch checking class_exists($factoryClass) ||
method_exists($factoryClass, $factoryMethod)), you throw a clear exception
(e.g., InvalidArgumentException or a small custom FactoryResolutionException)
instead of returning null; include the offending $factoryClass and
$factoryMethod in the exception message and reference DbQuery::factory and the
JsonHydrator::factory (or the factory-resolving method) so callers can diagnose
typos/refactors immediately.
---
Nitpick comments:
In `@composer.json`:
- Around line 106-108: The composer.json "platform" PHP constraint currently
pins the exact patch version ("php": "8.2.0"); update the value to the
minor-only constraint "8.2" (i.e., change platform -> php from "8.2.0" to "8.2")
so dependency resolution allows any 8.2.x patch release while still constraining
major/minor; locate the "platform" -> "php" entry in composer.json to make this
change.
In `@src/FakeQueryInterceptor.php`:
- Around line 113-126: readContent() currently always throws
FakeJsonNotFoundException when the fixture file is missing, which diverges from
the standard query path that uses isNullable($returnType) to return null for
nullable returns; update the code so selectPostQuery() (or readContent()) checks
the return type nullability via isNullable($returnType) and returns null when
the file is missing and the return type is nullable, otherwise keep throwing
FakeJsonNotFoundException; alternatively, if container fixtures must always
exist, add a clear docblock to readContent() and selectPostQuery() stating that
container post queries are required and will always throw when missing to
prevent future unintended changes.
In `@src/FakeQueryModule.php`:
- Line 51: The binding currently hard-codes the factory method name ('factory')
via FactoryMethod::class in FakeQueryModule (->toInstance('factory')), which
makes the convention global and undocumented; change the literal to a single
shared constant (e.g. replace 'factory' with
JsonHydrator::DEFAULT_FACTORY_METHOD or another appropriately named constant on
the hydrator/DbQuery target) and update the README/docs to document the
#[FactoryMethod] convention so the string/value lives in one place and the
convention is explicit.
🪄 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: 3bf05c88-ccf7-4d1a-ab58-8b31a8d19dcf
⛔ Files ignored due to path filters (3)
composer.lockis excluded by!**/*.lockvendor-bin/require-checker/composer.lockis excluded by!**/*.lockvendor-bin/tools/composer.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
.github/workflows/ci.yml.gitignore.planning/findings.md.planning/progress.md.planning/task_plan.mdREADME.mdcomposer.jsonsrc/FakeQueryInterceptor.phpsrc/FakeQueryModule.phpsrc/JsonHydrator.phptests/Fake/Entity/FactoryTodoEntity.phptests/Fake/Factory/InjectedTodoFactory.phptests/Fake/Factory/StaticTodoFactory.phptests/Fake/Query/FactoryTodoQueryInterface.phptests/Fake/Query/TodoSelectionQueryInterface.phptests/Fake/Result/TodoSelection.phptests/Fake/factory_injected_item.jsontests/Fake/factory_static_item.jsontests/Fake/factory_static_list.jsonltests/Fake/nested/factory_static_item.jsontests/FakeQueryModuleTest.phptests/FakeUnknownNested/nested/stray_query.json
| - [ ] Phase 3: テスト実装 | ||
| - [ ] Phase 4: 品質チェック | ||
| ## Test Results | ||
| | Date | Command | Result | Notes | |
There was a problem hiding this comment.
Add a blank line before the table under “Test Results”.
Line 43 triggers markdownlint MD058 (“tables should be surrounded by blank lines”). Add an empty line between the heading and the table.
Suggested diff
## Test Results
+
| Date | Command | Result | Notes |
|------|---------|--------|-------|📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | Date | Command | Result | Notes | | |
| ## Test Results | |
| | Date | Command | Result | Notes | | |
| |------|---------|--------|-------| |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 43-43: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🤖 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 @.planning/progress.md at line 43, Add a blank line between the "Test
Results" heading and the table row starting with "| Date | Command | Result |
Notes |" so the table is surrounded by a blank line per markdownlint MD058;
locate the "Test Results" heading and insert an empty line immediately above the
table (the line containing "| Date | Command | Result | Notes |") to satisfy the
lint rule.
| - [ ] Create release PR. | ||
|
|
||
| ## Errors Encountered | ||
| | Error | Attempt | Resolution | |
There was a problem hiding this comment.
Insert a blank line before the “Errors Encountered” table.
Line 120 hits markdownlint MD058 because the table is not surrounded by blank lines.
Suggested diff
## Errors Encountered
+
| Error | Attempt | Resolution |
|-------|---------|------------|📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | Error | Attempt | Resolution | | |
| ## Errors Encountered | |
| | Error | Attempt | Resolution | | |
| |-------|---------|------------| |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 120-120: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🤖 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 @.planning/task_plan.md at line 120, Add a single blank line immediately
before the "Errors Encountered" markdown table header (the line starting with "|
Error | Attempt | Resolution |") so the table is separated from preceding
content and satisfies markdownlint MD058; locate the table header in
.planning/task_plan.md and insert one empty line above it.
| private function selectPostQuery(string $postQueryClass, DbQuery $dbQuery, string|null $entityClass): PostQueryInterface | ||
| { | ||
| $rows = $this->hydrator->hydrate($this->readJsonl($dbQuery), $entityClass, false, $dbQuery); | ||
| assert(is_array($rows)); | ||
|
|
||
| return (new ReflectionClass($postQueryClass))->newInstance($rows); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Ray.MediaQuery 1.1 PostQueryInterface constructor contract documentation
💡 Result:
Ray.MediaQuery is a PHP library (ray/media-query on Packagist, version 1.1.0 released 2026-05-06) for database query execution via interfaces with Ray DI/AOP. Ray\MediaQuery\Result\PostQueryInterface is a contract for typed post-execution results from #[DbQuery] methods, introduced in PR #85 (merged April 2026) and extended to SELECT in PR #89.[1][2][3][4] The interface defines a single static factory method: public static function fromContext(PostQueryContext $context): static; It has no constructor as it's an interface. Implementing classes (e.g., AffectedRows, InsertedRow, custom wrappers) provide their own constructor and the fromContext factory, which the DbQueryInterceptor calls after SQL execution.[5][1][6] Example implementation from docs: php use Ray\MediaQuery\Result\PostQueryContext; use Ray\MediaQuery\Result\PostQueryInterface; final class RowCountWithQuery implements PostQueryInterface { public function construct( public readonly int $count, public readonly string $queryString, ) {} public static function fromContext(PostQueryContext $context): static { return new static($context->statement->rowCount(), $context->statement->queryString); } } PostQueryContext constructor (from source diff in PRs): php final class PostQueryContext { / * `@param` array<string, mixed> $values * `@param` array<mixed> $rows Hydrated rows for SELECT; `[]` for DML. */ public function construct( public readonly PDOStatement $statement, public readonly ExtendedPdoInterface $pdo, public readonly array $values, public readonly array $rows = [], ) {} } For SELECT, $rows contains pre-hydrated results (entities or arrays); empty for DML. Custom classes own hydration/logic extraction from context.[4][6][1] Official docs are in README on GitHub (https://github.com/ray-di/Ray.MediaQuery) and Packagist.[7][1][8] No separate API docs site found; source/PRs provide authoritative details.[4][2][5][3][6]
Citations:
- 1: https://packagist.org/packages/ray/media-query
- 2: ray-di/Ray.MediaQuery@ec9290f
- 3: ray-di/Ray.MediaQuery@d34d022
- 4: Unify SELECT / Collection under PostQueryInterface (replaces #87) Ray.MediaQuery#89
- 5: ray-di/Ray.MediaQuery@cbd5faa
- 6: ray-di/Ray.MediaQuery@56c9bda
- 7: https://github.com/ray-di/ray.mediaquery
- 8: https://github.com/ray-di/Ray.MediaQuery
🏁 Script executed:
# Find and examine src/FakeQueryInterceptor.php lines 99-105
fd FakeQueryInterceptor.phpRepository: ray-di/Ray.FakeQuery
Length of output: 93
🏁 Script executed:
# Search for PostQueryInterface implementations
rg "implements PostQueryInterface|fromContext" --type php -A 3Repository: ray-di/Ray.FakeQuery
Length of output: 695
🏁 Script executed:
# Check the TodoSelection fixture mentioned
fd -i todoselector
fd -i todo.*selectionRepository: ray-di/Ray.FakeQuery
Length of output: 149
🏁 Script executed:
# Read the exact code at src/FakeQueryInterceptor.php lines 99-105
sed -n '95,110p' src/FakeQueryInterceptor.phpRepository: ray-di/Ray.FakeQuery
Length of output: 718
🏁 Script executed:
# Check all PostQueryInterface implementations and their constructors
fd "\.php$" tests/ src/ | xargs grep -l "implements PostQueryInterface"Repository: ray-di/Ray.FakeQuery
Length of output: 100
🏁 Script executed:
# Verify what PostQueryContext should contain and how fromContext should be called
rg "PostQueryContext|fromContext" src/FakeQueryInterceptor.php -B 5 -A 5Repository: ray-di/Ray.FakeQuery
Length of output: 46
🏁 Script executed:
# Check if PostQueryContext is imported or available in FakeQueryInterceptor
head -40 src/FakeQueryInterceptor.phpRepository: ray-di/Ray.FakeQuery
Length of output: 1115
🏁 Script executed:
# Check if Ray.MediaQuery has PostQueryContext available and what selectPostQuery method should actually do
rg "PostQueryContext" --type phpRepository: ray-di/Ray.FakeQuery
Length of output: 254
🏁 Script executed:
# Look at the full selectPostQuery method and its context to understand what should be passed
sed -n '85,115p' src/FakeQueryInterceptor.phpRepository: ray-di/Ray.FakeQuery
Length of output: 1155
Fix selectPostQuery() to use Ray.MediaQuery's documented PostQueryInterface::fromContext() contract instead of direct constructor invocation.
The code calls (new ReflectionClass($postQueryClass))->newInstance($rows), which violates Ray.MediaQuery v1.1's documented interface contract. PostQueryInterface requires implementations to provide a static fromContext(PostQueryContext $context): static factory method — constructors are not part of the contract. TodoSelection works only because its constructor signature happens to match the assumption; any other implementation following Ray.MediaQuery's documented pattern will fail.
Instead, construct a PostQueryContext with the hydrated rows and upstream context (statement, PDO, bind values), then call $postQueryClass::fromContext($context).
🤖 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/FakeQueryInterceptor.php` around lines 99 - 105, selectPostQuery
currently instantiates the post-query via Reflection and constructor args which
breaks Ray.MediaQuery v1.1's contract; instead build a PostQueryContext using
the hydrated rows returned by
$this->hydrator->hydrate($this->readJsonl($dbQuery), $entityClass, false,
$dbQuery) plus the upstream context (statement, PDO, bind values) and then call
the static factory $postQueryClass::fromContext($context) (referencing
PostQueryInterface::fromContext and PostQueryContext) rather than newInstance;
update selectPostQuery to create and pass that PostQueryContext to the static
fromContext method so implementations like TodoSelection and others follow the
documented contract.
| $files = new RecursiveIteratorIterator( | ||
| new RecursiveDirectoryIterator($this->fakeDir, FilesystemIterator::SKIP_DOTS), | ||
| ); | ||
| foreach ($files as $file) { | ||
| $basename = basename($file); | ||
| $ext = pathinfo($file, PATHINFO_EXTENSION); | ||
| $stem = $ext === 'jsonl' ? basename($file, '.jsonl') : basename($file, '.json'); | ||
| assert($file instanceof SplFileInfo); | ||
| if (! $file->isFile()) { | ||
| continue; | ||
| } | ||
|
|
||
| $ext = pathinfo($file->getPathname(), PATHINFO_EXTENSION); | ||
| if ($ext !== 'json' && $ext !== 'jsonl') { | ||
| continue; | ||
| } | ||
|
|
||
| $basename = basename($file->getPathname()); | ||
| $stem = $this->queryIdFromFile($file, $ext); | ||
| if (! isset($knownIds[$stem])) { | ||
| throw new UnknownFakeJsonException($basename, $this->fakeDir); | ||
| } | ||
| } |
There was a problem hiding this comment.
Recursive iteration changes failure mode when fakeDir is missing or empty.
The previous glob() scan silently returned an empty array when fakeDir didn't exist or contained no matches. new RecursiveDirectoryIterator($this->fakeDir, FilesystemIterator::SKIP_DOTS) instead throws UnexpectedValueException if the directory is missing or unreadable, which now propagates out of configure() and turns a missing fixtures directory into a confusing DI-time stack trace. Consider guarding with is_dir($this->fakeDir) (early return or a dedicated, actionable exception) so module configuration fails with a message that points users at the misconfigured path.
🤖 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/FakeQueryModule.php` around lines 72 - 91, The recursive directory
iteration in configure() using new RecursiveDirectoryIterator($this->fakeDir,
FilesystemIterator::SKIP_DOTS) will throw UnexpectedValueException when
$this->fakeDir is missing or unreadable; change configure() to first check
is_dir($this->fakeDir) (or is_readable) and either return early (skip scanning)
or throw a clear, actionable exception that references $this->fakeDir, so the
failure mode matches the previous glob() behavior and avoids confusing DI-time
stack traces from RecursiveDirectoryIterator; update any tests or callers that
expect UnknownFakeJsonException to still be thrown for missing files inside a
valid directory.
| private function queryIdFromFile(SplFileInfo $file, string $ext): string | ||
| { | ||
| $relative = substr($file->getPathname(), strlen($this->fakeDir) + 1); | ||
| $queryId = substr($relative, 0, -strlen('.' . $ext)); | ||
|
|
||
| return str_replace(DIRECTORY_SEPARATOR, '/', $queryId); | ||
| } |
There was a problem hiding this comment.
queryIdFromFile() is fragile to trailing separators on fakeDir.
substr($file->getPathname(), strlen($this->fakeDir) + 1) assumes $this->fakeDir has no trailing / (or \). If a caller constructs FakeQueryModule with __DIR__ . '/fakes/' (a common pattern), the + 1 will eat the first character of the relative path and you'll either compute an off-by-one query id or, on Windows, get a mixed-separator result before the str_replace. Normalising $this->fakeDir once in the constructor (e.g., rtrim($fakeDir, DIRECTORY_SEPARATOR . '/')) — and using DIRECTORY_SEPARATOR length explicitly — would make this robust.
🛡️ Suggested fix
public function __construct(
- private readonly string $fakeDir,
+ string $fakeDir,
private readonly string $interfaceDir,
AbstractModule|null $module = null,
) {
+ $this->fakeDir = rtrim($fakeDir, '/' . DIRECTORY_SEPARATOR);
parent::__construct($module);
}(Drop readonly from the promoted property, or normalize before passing in.)
🤖 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/FakeQueryModule.php` around lines 94 - 100, queryIdFromFile() breaks when
$this->fakeDir has a trailing slash; normalize the stored fakeDir once in the
class constructor (e.g., trim trailing DIRECTORY_SEPARATOR and '/' from the
promoted property or remove readonly and rtrim in __construct) so later calls to
queryIdFromFile() can safely use substr($file->getPathname(),
strlen($this->fakeDir) + 1) or, better, compute the offset from
strlen($this->fakeDir) and only add 1 when the stored fakeDir does not end with
a separator; ensure you use DIRECTORY_SEPARATOR (and '/' for cross-platform)
when trimming and keep the existing str_replace(DIRECTORY_SEPARATOR, '/',
$queryId) for final normalization.
|
@coderabbitai Follow-up fix added after downstream use in BEAR.AppKata exposed the important case: installing Root cause: Ray.Aop keys annotated pointcuts by annotation class, so the existing Fix: FakeQuery now binds a priority interceptor using a non-deduplicated annotated matcher, so fake fixtures win in override contexts and the SQL interceptor is never reached. Regression coverage added in Ray.FakeQuery: a module with both Re-verified:
|
|
Design correction: FakeQuery now replaces the Ray\MediaQuery\DbQueryInterceptor binding with FakeQueryInterceptor instead of trying to win annotated pointcut priority. This keeps existing MediaQuery #[DbQuery] pointcuts installed while DI resolves them to the fake interceptor. A regression test covers MediaQueryModule + throwing SqlQueryInterface + FakeQueryModule override. CI is passing for Quality and Unit PHP 8.2-8.5. |
PR #2 レビュー: Harden FakeQuery release baseline概要リリース前のベースライン強化 PR。CI(PHP 8.2–8.5)、 良い点
指摘事項重要:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll look at the current implementation to review the fix properly. 🏁 Script executed: Length of output: 9469 🏁 Script executed: Length of output: 7912 🏁 Script executed: Length of output: 66 🏁 Script executed: Length of output: 11137
Override mechanism ✅The final design (as implemented) is the clean alias approach: $this->bindInterceptor(
$this->matcher->any(),
$this->matcher->annotatedWith(DbQuery::class),
[DbQueryInterceptor::class],
);
$this->bind(DbQueryInterceptor::class)->to(FakeQueryInterceptor::class)->in(Scope::SINGLETON);This is better than the intermediate "priority interceptor" approach because it doesn't race against existing pointcuts at all — Ray.Di's singleton binding resolution just redirects
Two concerns from the earlier review — already resolved ✅
$method = new ReflectionMethod($factoryClass, $factoryMethod);
if ($method->isStatic()) {
return static fn (array $row): mixed => $method->invokeArgs(null, array_values($row));
}
$factory = $this->injector->getInstance($factoryClass);
return static fn (array $row): mixed => $method->invokeArgs($factory, array_values($row));This correctly separates static dispatch from injected-instance dispatch without relying on PHP's context-sensitive Factory silent failure — fixed with if (! class_exists($factoryClass) || ! method_exists($factoryClass, $factoryMethod)) {
throw new InvalidFactoryException($factoryClass, $factoryMethod);
}
if (! $method->isPublic()) {
throw new InvalidFactoryException($factoryClass, $factoryMethod);
}
Remaining open itemsThese are lower priority but worth tracking:
Would you like me to open issues for any of the remaining items? 🐇✨ |
1 participant
Summary
#[DbQuery(factory: ...)]factories.PostQueryInterfaceresult wrapper support.Scope
PostQueryContextfor DML metadata.AffectedRows/InsertedRowsupport for a separate explicit metadata fixture design if needed.Tests
composer testscomposer crccomposer validate --strictSummary by CodeRabbit
New Features
Documentation
Tests
Chores