Add project content processors#84
Conversation
|
Warning Review limit reached
More reviews will be available in 40 minutes and 1 second. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughAdds a project-level content processor system for static-binary users. Introduces ChangesProject-Level Content Processors
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI / BuildCommand
participant Parser as SiteConfigParser
participant Loader as ProjectProcessorLoader
participant Pipeline as ContentProcessorPipeline
participant FS as Filesystem
CLI->>Parser: parseSiteConfig(config.yaml)
Parser-->>CLI: SiteConfig (with ProcessorConfig)
CLI->>Loader: new ProjectProcessorLoader(projectDir, configPath)
CLI->>Loader: load(ProcessorConfig)
Loader->>FS: glob(processors/*.php) if discover=true
FS-->>Loader: discovered paths
loop each path
Loader->>FS: realpath + require path
FS-->>Loader: ContentProcessorInterface or callable
Loader->>Loader: wrap callable in CallbackContentProcessor
end
Loader-->>CLI: ProjectProcessorSet
CLI->>Pipeline: insertBefore(MarkdownProcessor, contentBeforeMarkdown[])
CLI->>Pipeline: insertAfter(MarkdownProcessor, contentAfterMarkdown[])
Note over CLI,Pipeline: Same pattern for feed pipeline
Pipeline-->>CLI: ready for build
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Processor/Shortcode/ProjectShortcodeProcessor.php (1)
120-120: ⚡ Quick winConsider documenting the
=== 1return value convention.Line 120's logic
$result === 1 ? $output : $output . (string) $resultrelies on a subtle PHP behavior: when arequirestatement has no explicitreturn, it returns1. This allows templates to use eitherecho(captured via output buffering) orreturn(appended to output), but the exact-equality check against1is not self-documenting.Consider adding an inline comment explaining this convention, or documenting it in the class-level docblock.
📝 Suggested inline comment
- return $result === 1 ? $output : $output . (string) $result; + // require returns 1 when template has no explicit return; use only buffered output in that case + return $result === 1 ? $output : $output . (string) $result;🤖 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/Processor/Shortcode/ProjectShortcodeProcessor.php` at line 120, The ternary in ProjectShortcodeProcessor method returning "$result === 1 ? $output : $output . (string) $result" relies on PHP's require returning 1 when no explicit return is used; add a short inline comment at that line explaining that "require" with no return yields int(1) so the code distinguishes between echoed output (buffered) and returned string, and also update the class-level docblock of ProjectShortcodeProcessor to document this convention for template authors (mentioning that templates may either echo or return content and that a raw integer 1 means "no return").
🤖 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.
Nitpick comments:
In `@src/Processor/Shortcode/ProjectShortcodeProcessor.php`:
- Line 120: The ternary in ProjectShortcodeProcessor method returning "$result
=== 1 ? $output : $output . (string) $result" relies on PHP's require returning
1 when no explicit return is used; add a short inline comment at that line
explaining that "require" with no return yields int(1) so the code distinguishes
between echoed output (buffered) and returned string, and also update the
class-level docblock of ProjectShortcodeProcessor to document this convention
for template authors (mentioning that templates may either echo or return
content and that a raw integer 1 means "no return").
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 46f5aa16-1f28-4ca6-a28d-ee50f762ced6
📒 Files selected for processing (6)
benchmarks/ProjectShortcodeProcessorBench.phpconfig/common/di/content-pipeline.phpdocs/plugins.mdroadmap.mdsrc/Processor/Shortcode/ProjectShortcodeProcessor.phptests/Unit/Processor/ProjectShortcodeProcessorTest.php
samdark
changed the title
samdark
force-pushed
the
add-project-shortcodes
branch
from
6dae34d to
f0c5e96
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/Unit/Processor/ContentProcessorPipelineTest.php (1)
52-80: ⚡ Quick winReduce test code duplication by extracting the shared marker processor.
Both
testCanInsertProcessorsBeforeMarkerProcessor(lines 54-59) andtestCanInsertProcessorsAfterMarkerProcessor(lines 69-74) define identical anonymous marker classes. This duplication violates the DRY principle.♻️ Refactor to reduce duplication
Extract the marker processor creation to a helper method:
+ private function createMarkerProcessor(): ContentProcessorInterface + { + return new class implements ContentProcessorInterface { + public function process(string $content, Entry $entry): string + { + return $content . ' [marker]'; + } + }; + } + public function testCanInsertProcessorsBeforeMarkerProcessor(): void { - $marker = new class implements ContentProcessorInterface { - public function process(string $content, Entry $entry): string - { - return $content . ' [marker]'; - } - }; + $marker = $this->createMarkerProcessor(); $pipeline = new ContentProcessorPipeline($marker); $pipeline->insertBefore($marker::class, $this->createProcessor(fn(string $c) => $c . ' [before]')); assertSame('start [before] [marker]', $pipeline->process('start', $this->createEntry())); } public function testCanInsertProcessorsAfterMarkerProcessor(): void { - $marker = new class implements ContentProcessorInterface { - public function process(string $content, Entry $entry): string - { - return $content . ' [marker]'; - } - }; + $marker = $this->createMarkerProcessor(); $pipeline = new ContentProcessorPipeline($marker); $pipeline->insertAfter($marker::class, $this->createProcessor(fn(string $c) => $c . ' [after]')); assertSame('start [marker] [after]', $pipeline->process('start', $this->createEntry())); }🤖 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/Processor/ContentProcessorPipelineTest.php` around lines 52 - 80, Both testCanInsertProcessorsBeforeMarkerProcessor and testCanInsertProcessorsAfterMarkerProcessor methods contain identical anonymous implementations of the marker ContentProcessorInterface. Extract this shared marker processor creation into a private helper method (e.g., createMarkerProcessor) that returns the processor instance, then replace the duplicate anonymous class instantiations in both test methods with calls to this helper method to reduce code duplication.
🤖 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 `@benchmarks/ProjectProcessorBench.php`:
- Around line 61-63: The benchmark pipeline in the ProjectProcessorBench class
is only initializing ContentProcessorPipeline with contentBeforeMarkdown
processors, which does not reflect real-world usage. According to BuildCommand,
project processors are inserted before and after MarkdownProcessor within a
complete pipeline that includes core processors. Modify the pipeline
initialization to include MarkdownProcessor and core processors alongside the
project processors from contentBeforeMarkdown, ensuring the benchmark measures
realistic end-to-end performance rather than isolated project-processor
overhead.
In `@src/Console/BuildCommand.php`:
- Around line 1518-1521: The $this->contentPipeline and $this->feedPipeline are
injected singleton objects being mutated by insertBefore() and insertAfter()
calls without reset, causing processor accumulation if the BuildCommand instance
is reused. Before the insertBefore and insertAfter calls, either reset both
pipelines by removing existing processors or create fresh pipeline copies to
ensure idempotency. This prevents duplicate processor registration and execution
in scenarios where the BuildCommand is reused across multiple build executions.
In `@src/Processor/CallbackContentProcessor.php`:
- Around line 16-19: The process method in the CallbackContentProcessor class
performs an unsafe string cast on the callback return value without validation.
Add explicit type validation after calling the callback to check if the return
value is actually a string before the cast. If the return value is not a string,
throw an exception with a helpful error message that indicates the callback must
return a string type. This will prevent silent data corruption from null values
converting to empty strings, boolean/integer/float conversions, or fatal errors
from objects without __toString methods.
---
Nitpick comments:
In `@tests/Unit/Processor/ContentProcessorPipelineTest.php`:
- Around line 52-80: Both testCanInsertProcessorsBeforeMarkerProcessor and
testCanInsertProcessorsAfterMarkerProcessor methods contain identical anonymous
implementations of the marker ContentProcessorInterface. Extract this shared
marker processor creation into a private helper method (e.g.,
createMarkerProcessor) that returns the processor instance, then replace the
duplicate anonymous class instantiations in both test methods with calls to this
helper method to reduce code duplication.
🪄 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: 5bb4b831-db53-4a0a-8405-5896fda53ecb
📒 Files selected for processing (16)
benchmarks/ProjectProcessorBench.phpdocs/configuration.mddocs/plugins.mdroadmap.mdsrc/Console/BuildCommand.phpsrc/Content/Model/ProcessorConfig.phpsrc/Content/Model/SiteConfig.phpsrc/Content/Parser/SiteConfigParser.phpsrc/Processor/CallbackContentProcessor.phpsrc/Processor/ContentProcessorPipeline.phpsrc/Processor/ProjectProcessorLoader.phpsrc/Processor/ProjectProcessorSet.phptests/Unit/Console/BuildCommandTest.phptests/Unit/Content/Parser/SiteConfigParserTest.phptests/Unit/Processor/ContentProcessorPipelineTest.phptests/Unit/Processor/ProjectProcessorLoaderTest.php
✅ Files skipped from review due to trivial changes (2)
- docs/configuration.md
- roadmap.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/plugins.md
| $processors = (new ProjectProcessorLoader($this->projectDir, $this->projectDir . '/content/config.yaml')) | ||
| ->load(new ProcessorConfig()); | ||
| $this->pipeline = new ContentProcessorPipeline(...$processors->contentBeforeMarkdown); |
There was a problem hiding this comment.
Benchmark pipeline missing MarkdownProcessor and core processors.
The pipeline is initialized with only project processors from contentBeforeMarkdown, but real-world usage (as seen in BuildCommand) inserts project processors before/after MarkdownProcessor within a complete pipeline. This benchmark measures isolated project-processor overhead rather than realistic end-to-end performance where project processors work alongside core processors.
Consider including MarkdownProcessor and other core processors to match actual usage:
- $this->pipeline = new ContentProcessorPipeline(...$processors->contentBeforeMarkdown);
+ $markdownProcessor = new MarkdownProcessor(/* ... */);
+ $this->pipeline = new ContentProcessorPipeline($markdownProcessor);
+ $this->pipeline->insertBefore(MarkdownProcessor::class, ...$processors->contentBeforeMarkdown);🤖 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 `@benchmarks/ProjectProcessorBench.php` around lines 61 - 63, The benchmark
pipeline in the ProjectProcessorBench class is only initializing
ContentProcessorPipeline with contentBeforeMarkdown processors, which does not
reflect real-world usage. According to BuildCommand, project processors are
inserted before and after MarkdownProcessor within a complete pipeline that
includes core processors. Modify the pipeline initialization to include
MarkdownProcessor and core processors alongside the project processors from
contentBeforeMarkdown, ensuring the benchmark measures realistic end-to-end
performance rather than isolated project-processor overhead.
| $this->contentPipeline->insertBefore(MarkdownProcessor::class, ...$processors->contentBeforeMarkdown); | ||
| $this->contentPipeline->insertAfter(MarkdownProcessor::class, ...$processors->contentAfterMarkdown); | ||
| $this->feedPipeline->insertBefore(MarkdownProcessor::class, ...$processors->feedBeforeMarkdown); | ||
| $this->feedPipeline->insertAfter(MarkdownProcessor::class, ...$processors->feedAfterMarkdown); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether BuildCommand or ContentProcessorPipeline has a reset/clone path
# that prevents project processors from accumulating across repeated execute() calls.
rg -n -C3 'new BuildCommand|CommandTester|execute\(|insertBefore\(|insertAfter\(|clone|reset|applySiteConfig' \
--type=phpRepository: yiipress/engine
Length of output: 22810
Make project processor registration idempotent per build.
The insertBefore() and insertAfter() calls mutate the injected singleton pipelines. While Symfony console typically creates fresh command instances per execution, the code violates idempotency principles. If a BuildCommand instance is reused (in tests, long-running harnesses, or custom service configurations), project processors accumulate and apply multiple times. Reset the pipelines before re-inserting, or construct fresh pipeline copies per execution.
🤖 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/Console/BuildCommand.php` around lines 1518 - 1521, The
$this->contentPipeline and $this->feedPipeline are injected singleton objects
being mutated by insertBefore() and insertAfter() calls without reset, causing
processor accumulation if the BuildCommand instance is reused. Before the
insertBefore and insertAfter calls, either reset both pipelines by removing
existing processors or create fresh pipeline copies to ensure idempotency. This
prevents duplicate processor registration and execution in scenarios where the
BuildCommand is reused across multiple build executions.
| public function process(string $content, Entry $entry): string | ||
| { | ||
| return (string) ($this->callback)($content, $entry); | ||
| } |
There was a problem hiding this comment.
Validate callback return type or document the string cast behavior.
The string cast on line 18 may cause runtime errors or silent data corruption:
- If the callback returns an object without
__toString, PHP will throw a fatal error - If the callback returns
null, it silently becomes an empty string - If the callback returns
bool/int/float, values are silently converted
Consider adding explicit type validation with a helpful error message, or document that user callbacks must return string.
🛡️ Proposed fix to add validation
public function process(string $content, Entry $entry): string
{
- return (string) ($this->callback)($content, $entry);
+ $result = ($this->callback)($content, $entry);
+
+ if (!is_string($result)) {
+ throw new \RuntimeException(sprintf(
+ 'Project processor callback must return string, %s returned',
+ get_debug_type($result)
+ ));
+ }
+
+ return $result;
}📝 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.
| public function process(string $content, Entry $entry): string | |
| { | |
| return (string) ($this->callback)($content, $entry); | |
| } | |
| public function process(string $content, Entry $entry): string | |
| { | |
| $result = ($this->callback)($content, $entry); | |
| if (!is_string($result)) { | |
| throw new \RuntimeException(sprintf( | |
| 'Project processor callback must return string, %s returned', | |
| get_debug_type($result) | |
| )); | |
| } | |
| return $result; | |
| } |
🤖 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/Processor/CallbackContentProcessor.php` around lines 16 - 19, The process
method in the CallbackContentProcessor class performs an unsafe string cast on
the callback return value without validation. Add explicit type validation after
calling the callback to check if the return value is actually a string before
the cast. If the return value is not a string, throw an exception with a helpful
error message that indicates the callback must return a string type. This will
prevent silent data corruption from null values converting to empty strings,
boolean/integer/float conversions, or fatal errors from objects without
__toString methods.
f0c5e96 to
c4c2eec
Compare
Summary
content/processors/*.phpwhenprocessorsconfig is omitted, whileprocessors: falsedisables them and explicit config uses only the listed filesContentProcessorInterfaceinstances or callables, validate paths stay inside the content directory, include processor files in build invalidation, and document the featureTests
make test -- --filter BuildCommandTest::testBuildAppliesProjectProcessorsmake test -- --filter ProjectProcessorLoaderTestmake test -- --filter SiteConfigParserTestmake psalmmake testBENCH_FILTER=ProjectProcessorBench make bench