Rebuild cache observability on Koriym.SemanticLogger#178
Conversation
Migrate the cache create/invalidate log to Koriym.SemanticLogger's open/event/close tree, where the nesting mirrors the embed/dependency structure. Typed Context classes (src/Log/Context/) carry per-context JSON Schemas (docs/schemas/context/), and SafeSemanticLogger guarantees logging never breaks cache reads/writes (NullSemanticLogger is the no-op default). Adds koriym/semantic-logger ^0.8.0; vendor/bin/stree renders the cache log as a tree (demo/run-dependency.php, demo/run-donut.php). The legacy RepositoryLogger interface stays bound for BC but receives no internal events. The multi-embed dependency fix and diamond tests are intentionally excluded here; they already shipped in 1.16.1 (bearsunday#177).
Direct, non-AOP cache operations had no enclosing log scope, so their save/invalidate events were dropped at flush (an event-only session renders empty). A top-level put() now opens a manual_store scope (closed with manual_store_result) and a top-level invalidateTags() opens a manual_invalidate scope (closed with the existing InvalidateContext), mirroring the existing manual_purge treatment. Nested (AOP) calls are unchanged, so the in-flow tree shape is identical. Adds the three Context classes and their JSON Schemas, and tests that flush-validate the new top-level trees.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR migrates BEAR.QueryRepository from flat string-based repository logging to structured ChangesSemantic Logger Migration for Cache Observability
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 1.x #178 +/- ##
============================================
Coverage 100.00% 100.00%
- Complexity 247 314 +67
============================================
Files 53 77 +24
Lines 749 902 +153
============================================
+ Hits 749 902 +153 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/StructuredRepositoryLoggerInterface.php (1)
7-17: 💤 Low valueMerge the two stacked docblocks into one.
Only the docblock immediately preceding the
interfacedeclaration (the@deprecatedblock) is attached by doc/IDE tooling; the first block describing the BC rationale is effectively dropped. Consider combining them.♻️ Proposed merge
/** * A repository logger that also exposes its entries as structured data * * Separated from RepositoryLoggerInterface so that adding structured accessors * does not break third-party RepositoryLoggerInterface implementations (BC). * Use this for structural assertions instead of substring-matching __toString(). - */ -/** + * * `@deprecated` Since the SemanticLogger migration; structured logs are now the * {`@see` \Koriym\SemanticLogger\LogJson} tree returned by SemanticLogger::flush(). */ interface StructuredRepositoryLoggerInterface extends RepositoryLoggerInterface🤖 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/StructuredRepositoryLoggerInterface.php` around lines 7 - 17, Combine the two separate docblocks into a single docblock immediately above the StructuredRepositoryLoggerInterface declaration: include the descriptive paragraph about it being a repository logger exposing entries as structured data and the BC rationale mentioning separation from RepositoryLoggerInterface, then append the `@deprecated` tag with its message referencing SemanticLogger, LogJson, and SemanticLogger::flush(); ensure the merged docblock replaces both existing blocks so IDE/doc tooling sees the full description and deprecation together for StructuredRepositoryLoggerInterface.tests/SemanticLogTreeTrait.php (1)
46-58: 💤 Low valueOptional: unlink the temp file after validation.
tempnam()creates a file that is never removed, so each scenario leaves aslog*file in the system temp dir across the suite. Harmless but accumulates.♻️ Suggested cleanup
ob_start(); try { (new SemanticLogValidator())->validate($file, $schemaDir); } finally { ob_get_clean(); + `@unlink`($file); }🤖 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/SemanticLogTreeTrait.php` around lines 46 - 58, The temp file created via tempnam() and stored in $file is never removed; after calling (new SemanticLogValidator())->validate($file, $schemaDir) ensure the temporary file is unlinked—add an unlink($file) in the finally block (after ob_get_clean()) so the temp slog* file is always deleted even on validation failure; reference the $file variable and the validate call to locate where to add the cleanup.src/QueryRepository.php (1)
49-60: ⚖️ Poor tradeoffManual-scope logging:
isTopLevel()exists, and DI guaranteesSafeSemanticLogger
put()/purge()only open manual scopes when the bound logger isSafeSemanticLoggerandisTopLevel()is true;SafeSemanticLoggerdoes defineisTopLevel(): bool.DonutCacheModulebindsSemanticLoggerInterfaceas a singleton viaSafeSemanticLoggerProvider(always returningSafeSemanticLogger), and the same pattern is used inResourceStorage::invalidateTags(), so the top-level manual-scope path is satisfied for the actual runtime binding.- If replacing
SemanticLoggerInterfacewith other implementations is a goal, introduce a narrower interface for the “top-level + manual scope” capability (instead of relying oninstanceof SafeSemanticLogger).🤖 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/QueryRepository.php` around lines 49 - 60, The code uses instanceof SafeSemanticLogger and isTopLevel() inside put()/purge() to decide whether to open manual logging scopes; replace this instanceof-based runtime check with a clearer contract by introducing a small interface (e.g., TopLevelManualScopeLogger with methods isTopLevel(): bool, open(ManualStoreContext): mixed, close(ManualStoreResultContext, mixed): void), have SafeSemanticLogger implement it, update the method signatures/DI usage to type-hint/accept TopLevelManualScopeLogger where manual scopes are needed (or use a safe cast via interface check), and update callers like QueryRepository::put()/purge() and ResourceStorage::invalidateTags() to use that interface instead of instanceof SafeSemanticLogger so other logger implementations can opt in without relying on instanceof or concrete class bindings.
🤖 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 `@psalm.xml`:
- Around line 23-29: psalm.xml currently suppresses the InvalidClassConstantType
rule for the entire src/Log/Context directory; instead, remove that
directory-wide suppression and add targeted suppressions for only the concrete
Context classes that override Koriym\SemanticLogger\AbstractContext's untyped
constants (specifically the classes that declare public const TYPE and public
const SCHEMA_URL). Locate each concrete *Context class that defines those
constants and add a file- or class-scoped Psalm suppression for
InvalidClassConstantType (or an inline `@psalm-suppress` on the class) so other
files in the directory remain checked normally.
In `@src/Log/NullSemanticLogger.php`:
- Around line 23-37: The no-op methods open, event, and close in
NullSemanticLogger are tripping PHPMD for unused parameters; fix by explicitly
consuming the parameters to silence the linter — add a void-cast or unset for
the parameters (e.g., (void)$context; and for close also (void)$openId;) at the
start of each method body so the methods remain no-ops but PHPMD no longer flags
the parameters as unused.
In `@src/Log/SafeSemanticLogger.php`:
- Around line 133-138: The __unserialize method in SafeSemanticLogger currently
ignores its $data parameter, triggering PHPMD UnusedFormalParameter; update
SafeSemanticLogger::__unserialize(array $data): void to consume $data (for
example with unset($data) or (void)$data;) before initializing $this->logger and
$this->broken, so the parameter is referenced and the linter warning is
suppressed while preserving existing behavior of new SemanticLogger() and
$this->broken = false.
---
Nitpick comments:
In `@src/QueryRepository.php`:
- Around line 49-60: The code uses instanceof SafeSemanticLogger and
isTopLevel() inside put()/purge() to decide whether to open manual logging
scopes; replace this instanceof-based runtime check with a clearer contract by
introducing a small interface (e.g., TopLevelManualScopeLogger with methods
isTopLevel(): bool, open(ManualStoreContext): mixed,
close(ManualStoreResultContext, mixed): void), have SafeSemanticLogger implement
it, update the method signatures/DI usage to type-hint/accept
TopLevelManualScopeLogger where manual scopes are needed (or use a safe cast via
interface check), and update callers like QueryRepository::put()/purge() and
ResourceStorage::invalidateTags() to use that interface instead of instanceof
SafeSemanticLogger so other logger implementations can opt in without relying on
instanceof or concrete class bindings.
In `@src/StructuredRepositoryLoggerInterface.php`:
- Around line 7-17: Combine the two separate docblocks into a single docblock
immediately above the StructuredRepositoryLoggerInterface declaration: include
the descriptive paragraph about it being a repository logger exposing entries as
structured data and the BC rationale mentioning separation from
RepositoryLoggerInterface, then append the `@deprecated` tag with its message
referencing SemanticLogger, LogJson, and SemanticLogger::flush(); ensure the
merged docblock replaces both existing blocks so IDE/doc tooling sees the full
description and deprecation together for StructuredRepositoryLoggerInterface.
In `@tests/SemanticLogTreeTrait.php`:
- Around line 46-58: The temp file created via tempnam() and stored in $file is
never removed; after calling (new SemanticLogValidator())->validate($file,
$schemaDir) ensure the temporary file is unlinked—add an unlink($file) in the
finally block (after ob_get_clean()) so the temp slog* file is always deleted
even on validation failure; reference the $file variable and the validate call
to locate where to add the cleanup.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 431b3a67-ada3-42a2-bbed-724f5a8f5882
📒 Files selected for processing (81)
CHANGELOG.mdcomposer.jsondemo/run-dependency.phpdemo/run-donut.phpdocs/schemas/context/cache_hit.jsondocs/schemas/context/cache_miss.jsondocs/schemas/context/command.jsondocs/schemas/context/command_result.jsondocs/schemas/context/depends_on.jsondocs/schemas/context/get.jsondocs/schemas/context/invalidate.jsondocs/schemas/context/manual_invalidate.jsondocs/schemas/context/manual_purge.jsondocs/schemas/context/manual_purge_result.jsondocs/schemas/context/manual_store.jsondocs/schemas/context/manual_store_result.jsondocs/schemas/context/purge.jsondocs/schemas/context/put_donut.jsondocs/schemas/context/refresh_donut.jsondocs/schemas/context/save_donut.jsondocs/schemas/context/save_donut_view.jsondocs/schemas/context/save_etag.jsondocs/schemas/context/save_value.jsondocs/schemas/context/save_view.jsondocs/schemas/repository-log.jsonpsalm.xmlsrc/AbstractDonutCacheInterceptor.phpsrc/CacheDependency.phpsrc/CacheInterceptor.phpsrc/CommandContextFactory.phpsrc/CommandInterceptor.phpsrc/DonutCacheModule.phpsrc/DonutCommandInterceptor.phpsrc/DonutRepository.phpsrc/Log/Context/CacheHitContext.phpsrc/Log/Context/CacheMissContext.phpsrc/Log/Context/CommandContext.phpsrc/Log/Context/CommandResultContext.phpsrc/Log/Context/DependsOnContext.phpsrc/Log/Context/GetContext.phpsrc/Log/Context/InvalidateContext.phpsrc/Log/Context/ManualInvalidateContext.phpsrc/Log/Context/ManualPurgeContext.phpsrc/Log/Context/ManualPurgeResultContext.phpsrc/Log/Context/ManualStoreContext.phpsrc/Log/Context/ManualStoreResultContext.phpsrc/Log/Context/PurgeContext.phpsrc/Log/Context/PutDonutContext.phpsrc/Log/Context/RefreshDonutContext.phpsrc/Log/Context/SaveDonutContext.phpsrc/Log/Context/SaveDonutViewContext.phpsrc/Log/Context/SaveEtagContext.phpsrc/Log/Context/SaveValueContext.phpsrc/Log/Context/SaveViewContext.phpsrc/Log/NullSemanticLogger.phpsrc/Log/SafeSemanticLogger.phpsrc/Log/SafeSemanticLoggerProvider.phpsrc/NullRepositoryLogger.phpsrc/QueryRepository.phpsrc/RefreshInterceptor.phpsrc/RepositoryLogger.phpsrc/RepositoryLoggerInterface.phpsrc/ResourceStorage.phpsrc/StructuredRepositoryLoggerInterface.phptests/CACHE_DEPENDENCY_TESTS.mdtests/DonutCacheInterceptorTest.phptests/DonutCommandInterceptorTest.phptests/DonutCommandRedisCacheTest.phptests/DonutQueryInterceptorPurgeTest.phptests/DonutQueryInterceptorTest.phptests/FakeThrowingPurger.phptests/GracefulLoggingTest.phptests/QueryRepositoryTest.phptests/RecordingSemanticLogger.phptests/RepositoryLoggerTest.phptests/ResourceRepositoryTest.phptests/ResourceStorageTest.phptests/SafeSemanticLoggerTest.phptests/SemanticLogSchemaTest.phptests/SemanticLogTreeTrait.phptests/ThrowingSemanticLogger.php
💤 Files with no reviewable changes (1)
- docs/schemas/repository-log.json
| <!-- Koriym\SemanticLogger\AbstractContext declares TYPE/SCHEMA_URL = '' (untyped), which Psalm | ||
| infers as the literal '' type; concrete Context subclasses legitimately set string values. --> | ||
| <InvalidClassConstantType> | ||
| <errorLevel type="suppress"> | ||
| <directory name="src/Log/Context" /> | ||
| </errorLevel> | ||
| </InvalidClassConstantType> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Current Psalm suppression:"
rg -n -C2 'InvalidClassConstantType|src/Log/Context' psalm.xml
echo
echo "Context classes covered by this suppression:"
fd -e php . src/Log/Context | sort | while read -r file; do
echo "== $file =="
rg -n 'public const (TYPE|SCHEMA_URL)' "$file" || true
done
echo
echo "Existing localized suppressions, if any:"
rg -n '`@psalm-suppress` InvalidClassConstantType|psalm-suppress InvalidClassConstantType' src/Log/Context psalm.xml || trueRepository: bearsunday/BEAR.QueryRepository
Length of output: 4757
Localize the Psalm InvalidClassConstantType suppression instead of disabling it for src/Log/Context wholesale
psalm.xml currently suppresses InvalidClassConstantType for the entire src/Log/Context directory, which covers all 20 concrete *Context.php classes that declare public const TYPE / public const SCHEMA_URL. Replace the directory-wide suppression with targeted file/class-level suppressions for only the specific workaround cases tied to the Koriym\SemanticLogger\AbstractContext constant-type mismatch, so new/changed constants don’t get silently ignored.
🤖 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 `@psalm.xml` around lines 23 - 29, psalm.xml currently suppresses the
InvalidClassConstantType rule for the entire src/Log/Context directory; instead,
remove that directory-wide suppression and add targeted suppressions for only
the concrete Context classes that override
Koriym\SemanticLogger\AbstractContext's untyped constants (specifically the
classes that declare public const TYPE and public const SCHEMA_URL). Locate each
concrete *Context class that defines those constants and add a file- or
class-scoped Psalm suppression for InvalidClassConstantType (or an inline
`@psalm-suppress` on the class) so other files in the directory remain checked
normally.
| #[Override] | ||
| public function open(AbstractContext $context): string | ||
| { | ||
| return ''; | ||
| } | ||
|
|
||
| #[Override] | ||
| public function event(AbstractContext $context): void | ||
| { | ||
| } | ||
|
|
||
| #[Override] | ||
| public function close(AbstractContext $context, string $openId): void | ||
| { | ||
| } |
There was a problem hiding this comment.
Silence PHPMD on the intentionally unused interface parameters.
These no-op methods currently fail CI because PHPMD flags the required interface parameters as unused on Lines 24, 30, and 35. Add a local suppression or explicitly consume the parameters so the null logger remains build-clean.
Proposed fix
final class NullSemanticLogger implements SemanticLoggerInterface
{
private const EMPTY_SCHEMA_URL = 'https://koriym.github.io/Koriym.SemanticLogger/schemas/semantic-log.json';
+ /** `@phpmdSuppressWarnings`(UnusedFormalParameter) */
#[Override]
public function open(AbstractContext $context): string
{
return '';
}
+ /** `@phpmdSuppressWarnings`(UnusedFormalParameter) */
#[Override]
public function event(AbstractContext $context): void
{
}
+ /** `@phpmdSuppressWarnings`(UnusedFormalParameter) */
#[Override]
public function close(AbstractContext $context, string $openId): void
{
}📝 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.
| #[Override] | |
| public function open(AbstractContext $context): string | |
| { | |
| return ''; | |
| } | |
| #[Override] | |
| public function event(AbstractContext $context): void | |
| { | |
| } | |
| #[Override] | |
| public function close(AbstractContext $context, string $openId): void | |
| { | |
| } | |
| #[Override] | |
| /** `@phpmdSuppressWarnings`(UnusedFormalParameter) */ | |
| public function open(AbstractContext $context): string | |
| { | |
| return ''; | |
| } | |
| #[Override] | |
| /** `@phpmdSuppressWarnings`(UnusedFormalParameter) */ | |
| public function event(AbstractContext $context): void | |
| { | |
| } | |
| #[Override] | |
| /** `@phpmdSuppressWarnings`(UnusedFormalParameter) */ | |
| public function close(AbstractContext $context, string $openId): void | |
| { | |
| } |
🧰 Tools
🪛 GitHub Actions: Static Analysis / 1_sa _ PHPMD.txt
[error] 24-24: PHP Mess Detector (phpmd) rule 'UnusedFormalParameter': Avoid unused parameters such as '$context'.
[error] 30-30: PHP Mess Detector (phpmd) rule 'UnusedFormalParameter': Avoid unused parameters such as '$context'.
[error] 35-35: PHP Mess Detector (phpmd) rule 'UnusedFormalParameter': Avoid unused parameters such as '$context'.
[error] 35-35: PHP Mess Detector (phpmd) rule 'UnusedFormalParameter': Avoid unused parameters such as '$openId'.
🪛 GitHub Actions: Static Analysis / sa _ PHPMD
[error] 24-24: PHPMD (text ruleset): UnusedFormalParameter. Avoid unused parameters such as '$context'.
[error] 30-30: PHPMD (text ruleset): UnusedFormalParameter. Avoid unused parameters such as '$context'.
[error] 35-35: PHPMD (text ruleset): UnusedFormalParameter. Avoid unused parameters such as '$context'.
[error] 35-35: PHPMD (text ruleset): UnusedFormalParameter. Avoid unused parameters such as '$openId'.
🪛 PHPMD (2.15.0)
[warning] 24-24: Avoid unused parameters such as '$context'. (undefined)
(UnusedFormalParameter)
[warning] 30-30: Avoid unused parameters such as '$context'. (undefined)
(UnusedFormalParameter)
[warning] 35-35: Avoid unused parameters such as '$context'. (undefined)
(UnusedFormalParameter)
[warning] 35-35: Avoid unused parameters such as '$openId'. (undefined)
(UnusedFormalParameter)
🤖 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/Log/NullSemanticLogger.php` around lines 23 - 37, The no-op methods open,
event, and close in NullSemanticLogger are tripping PHPMD for unused parameters;
fix by explicitly consuming the parameters to silence the linter — add a
void-cast or unset for the parameters (e.g., (void)$context; and for close also
(void)$openId;) at the start of each method body so the methods remain no-ops
but PHPMD no longer flags the parameters as unused.
| /** @param array<string, mixed> $data */ | ||
| public function __unserialize(array $data): void | ||
| { | ||
| $this->logger = new SemanticLogger(); | ||
| $this->broken = false; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print the relevant portion of SafeSemanticLogger.php
FILE="src/Log/SafeSemanticLogger.php"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
echo "=== Context around __serialize/__unserialize in $FILE ==="
nl -ba "$FILE" | sed -n '1,220p' | sed -n '100,190p'
echo "=== Find all references to \$depth and \$broken in SafeSemanticLogger.php ==="
rg -n "\$depth|\$broken|__serialize|__unserialize|isTopLevel|open\s*\(" "$FILE"Repository: bearsunday/BEAR.QueryRepository
Length of output: 220
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/Log/SafeSemanticLogger.php"
test -f "$FILE" || { echo "Missing file: $FILE" >&2; exit 1; }
echo "=== Context around __serialize/__unserialize in $FILE (lines 100-190) ==="
cat -n "$FILE" | sed -n '100,190p'
echo "=== Find references to \$depth, \$broken, and magic serialization methods ==="
rg -n "\$depth|\$broken|__serialize|__unserialize|isTopLevel|open\s*\(" "$FILE"Repository: bearsunday/BEAR.QueryRepository
Length of output: 2004
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/Log/SafeSemanticLogger.php"
test -f "$FILE" || { echo "Missing file: $FILE" >&2; exit 1; }
echo "=== Property/methods context (lines 1-120) ==="
cat -n "$FILE" | sed -n '1,120p'
echo "=== Exact occurrences of depth usage ==="
rg -n "\$this->depth|private int \$depth|isTopLevel|function open\(" "$FILE"
echo "=== Exact occurrences of magic serialization params ==="
rg -n "__unserialize\(array \$data\)" "$FILE" && rg -n "\$data" "$FILE"
echo "=== Check composer availability ==="
command -v composer >/dev/null && composer --version || echo "composer not available"
# Try to run PHPMD/PSR checks only if composer exists; keep output bounded.
if command -v composer >/dev/null; then
echo "=== Running composer cs (PSR-12) ==="
composer cs 2>&1 | head -n 120 || true
echo "=== Running composer sa (PHPStan/Psalm) ==="
composer sa 2>&1 | head -n 160 || true
fiRepository: bearsunday/BEAR.QueryRepository
Length of output: 5179
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ls -la
echo "=== composer.json (top 200 lines) ==="
test -f composer.json && sed -n '1,200p' composer.json || true
echo "=== vendor/bin (if present) ==="
ls -la vendor 2>/dev/null || true
ls -la vendor/bin 2>/dev/null || true
echo "=== Search for phpmmd/phpmd/phpcs config ==="
rg -n "phpmd|phpmmd|phpcs|phpstan|psalm|php-cs-fixer" -S . || trueRepository: bearsunday/BEAR.QueryRepository
Length of output: 28025
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== phpmd.xml rules relevant sections ==="
cat -n phpmd.xml | sed -n '1,200p'
echo "=== Run phpmd on SafeSemanticLogger.php (bounded output) ==="
./vendor/bin/phpmd src/Log/SafeSemanticLogger.php text phpmd.xml | head -n 80 || trueRepository: bearsunday/BEAR.QueryRepository
Length of output: 2603
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Locate phpmd executable ==="
find vendor -type f \( -iname '*phpmd*' -o -path '*phpmd*' \) | head -n 80
echo "=== Locate vendor-bin phpmd ==="
find vendor-bin -type f \( -iname '*phpmd*' -o -path '*phpmd*' \) | head -n 80
echo "=== Search composer.json scripts for phpmd path ==="
cat composer.json | rg -n "\"phpmd\""Repository: bearsunday/BEAR.QueryRepository
Length of output: 317
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for phpmd binaries/scripts/PHARs ==="
find . -type f \( -iname 'phpmd*' -o -path '*phpmd*' \) | head -n 200
echo "=== Search inside vendor for phpmd/phpmd package directory ==="
find vendor -type d -iname '*phpmd*' | head -n 50
echo "=== Search vendor-bin/tools for phpmd ==="
find vendor-bin -type f \( -iname '*phpmd*' -o -path '*phpmd*' \) | head -n 200
echo "=== Check whether phpmd is installed as a PHAR anywhere ==="
find vendor -type f \( -iname '*.phar' -o -iname '*phpmd*' \) | head -n 200Repository: bearsunday/BEAR.QueryRepository
Length of output: 374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Locate phpmd package directories ==="
find vendor -maxdepth 4 -type d \( -iname 'phpmd' -o -path '*phpmd*' \) | head -n 200
echo "=== Locate phpmd entrypoint files named phpmd ==="
find vendor -type f \( -iname 'phpmd' -o -path '*bin/phpmd*' -o -path '*phpmd*bin*' \) | head -n 200
echo "=== Locate phpmd.php / launcher scripts ==="
find vendor -type f \( -iname '*phpmd*.php' -o -path '*phpmd*/*.php' \) | head -n 200Repository: bearsunday/BEAR.QueryRepository
Length of output: 210
Consume SafeSemanticLogger::__unserialize(array $data)’s $data parameter to satisfy PHPMD UnusedFormalParameter.
src/Log/SafeSemanticLogger.php lines 133-138: $data is never used. Add a no-op consumption (e.g., unset($data);). The depth concern is not applicable here since private int $depth = 0; provides a default value.
🧰 Tools
🪛 GitHub Actions: Static Analysis / 1_sa _ PHPMD.txt
[error] 134-134: PHP Mess Detector (phpmd) rule 'UnusedFormalParameter': Avoid unused parameters such as '$data'.
🪛 GitHub Actions: Static Analysis / sa _ PHPMD
[error] 134-134: PHPMD (text ruleset): UnusedFormalParameter. Avoid unused parameters such as '$data'.
🪛 PHPMD (2.15.0)
[warning] 134-134: Avoid unused parameters such as '$data'. (undefined)
(UnusedFormalParameter)
🤖 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/Log/SafeSemanticLogger.php` around lines 133 - 138, The __unserialize
method in SafeSemanticLogger currently ignores its $data parameter, triggering
PHPMD UnusedFormalParameter; update SafeSemanticLogger::__unserialize(array
$data): void to consume $data (for example with unset($data) or (void)$data;)
before initializing $this->logger and $this->broken, so the parameter is
referenced and the linter warning is suppressed while preserving existing
behavior of new SemanticLogger() and $this->broken = false.
|
@coderabbitai Addressed in df8c8b6:
Not changed, with reasons:
|
|
Tip For best results, initiate chat on the files or code changes.
The reasoning for the two items left unchanged is sound as well:
Everything looks well-reasoned. Happy with the state of the PR! 🐇✨ 🧠 Learnings used |
CI surfaced two checks the unpushed branch had never run locally: - sa/PHPMD flagged UnusedFormalParameter on no-op / contract-driven signatures. Add @SuppressWarnings("PHPMD.UnusedFormalParameter") (quoted so PHPStan's phpDoc parser accepts the dotted rule name) on NullSemanticLogger and on SafeSemanticLogger::__unserialize, whose parameters are required by the interface / magic-method contract but intentionally unused. - codecov (100% target) flagged the no-op NullSemanticLogger methods and the best-effort catch blocks in SafeSemanticLogger::event()/close() as uncovered. NullSemanticLogger is a pure no-op null object, so its methods are marked @codeCoverageIgnore; the SafeSemanticLogger catch blocks are the resilience guarantee, so they get real failure tests instead. Also address review nitpicks: merge the two stacked docblocks on StructuredRepositoryLoggerInterface, and unlink the temp file in SemanticLogTreeTrait.
koriym
force-pushed
the
cache-observability
branch
from
df8c8b6 to
c5f2099
Compare
|
@coderabbitai Follow-up (force-pushed c5f2099): for the codecov gap, |
|
Tip For best results, initiate chat on the files or code changes.
🐇✨ |
Adversarial review flagged that the observability change had made CDN purge silently best-effort: invalidateTags() swallowed a purger exception and still reported success, so a write could leave stale CDN content unnoticed. Restore the 1.x fail-closed behavior: the local pools are invalidated first, the outcome is logged as cdn=failed, and the purge exception is then re-thrown to the caller. The purger-failure test now asserts propagation instead of best-effort masking.
koriym
force-pushed
the
cache-observability
branch
from
0eac6c6 to
eb337fb
Compare
The adversarial review noted the singleton SemanticLogger session can drop a request's log under concurrent coroutines (interleaved open/close violates LIFO, SafeSemanticLogger marks the session broken, the flush is empty) rather than merely interleave it. Sharpen the Known Limitations note: cache behavior is unaffected (logging is a best-effort side-channel), PHP-FPM is unaffected, and a request/coroutine-scoped logger is the robust fix, intentionally deferred to the host flush-lifecycle work.
|
@coderabbitai Recording the reasoning for the design points raised in review (an out-of-band adversarial pass surfaced the same three):
|
|
Tip For best results, initiate chat on the files or code changes.
All three noted for future reviews. 🐇 ✏️ Learnings added
|
1 participant
Summary
Rebuilds cache observability on Koriym.SemanticLogger: the cache create/invalidate log becomes an open/event/close tree whose nesting is the embed/dependency structure (a parent's embedded children nest under it). The legacy flat
RepositoryLoggeris deprecated but stays bound for BC and receives no internal events.Included
CacheInterceptor/AbstractDonutCacheInterceptor(GET) andCommandInterceptor/RefreshInterceptor/DonutCommandInterceptor(command).SafeSemanticLoggerbest-effort decorator (a logging failure can never break a cache read/write),NullSemanticLoggerzero-cost no-op default, bound viaSafeSemanticLoggerProvider.AbstractContextsubclasses insrc/Log/Context/with per-context JSON Schemas indocs/schemas/context/.invalidatecontext records self-describing status words:roPool/etagPool(invalidated|failed),cdn(purged|failed), plusdurationMs. The CDN purge is best-effort and no longer fails local invalidation on outage.put()andinvalidateTags()are rooted inmanual_store/manual_invalidatescopes so their events are not dropped at flush, mirroring the existingmanual_purgetreatment.koriym/semantic-logger ^0.8.0;vendor/bin/streerenders the cache log as a readable tree (demo/run-dependency.php,demo/run-donut.php).Not included (deliberate)
docs/llms.txt/docs/llms-full.txt(still reference the old flat log) is left for a separate docs PR.Verification
demo/run-dependency.phpanddemo/run-donut.phprender the SemanticLogger tree (exit 0).Commits