Move from document/{docId}/pdf to /document/{docId}/content endpoint#100
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (6)
WalkthroughThe changes add content-focused APIs: Document gains setFile(string $filePath), getContent(bool $signed = true), and getFormat(): ?string, plus protected properties 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.54)PHPStan was skipped because the sandbox runner could not parse its output. 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: 2
🤖 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 `@docs/document.md`:
- Line 84: The call to $myDocument->getContent(signed: false) uses PHP 8 named
arguments and breaks PHP 7.2–7.4 compatibility; replace the named argument with
a positional one by calling getContent with the boolean value for the signed
parameter (i.e., pass false as the positional argument) so the invocation uses
positional arguments and remains compatible; locate the call site on the
$myDocument->getContent invocation and update it accordingly.
In `@src/Document.php`:
- Around line 136-139: The setFile method on the Document class currently
assigns an unreadable path to $this->file; change it to validate the provided
$filePath (using PHP functions like file_exists and is_readable) and throw a
clear exception (e.g., InvalidArgumentException) when the path is missing or not
readable so callers fail fast; update Document::setFile to perform this check
and only assign $this->file when the path passes validation, ensuring later
payload construction won't silently omit the file.
🪄 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: Enterprise
Run ID: 7cc92730-8a85-4970-9324-adff63f86391
📒 Files selected for processing (6)
CHANGELOG.mdREADME.mddocs/document.mdexamples/casefile-e2e-demo.phpsrc/Document.phpsrc/Entity.php
| public function setFile(string $filePath): void | ||
| { | ||
| $this->file = $filePath; | ||
| } |
There was a problem hiding this comment.
Validate file readability in setFile() to fail fast.
On Line 138, an unreadable path is accepted and later silently omitted from request payload construction, which makes upload failures harder to diagnose.
Proposed fix
public function setFile(string $filePath): void
{
+ if (!is_readable($filePath)) {
+ throw new \InvalidArgumentException('Document file is not readable: ' . $filePath);
+ }
$this->file = $filePath;
+ $this->pdfFile = null; // avoid sending both legacy and new file fields
}🤖 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/Document.php` around lines 136 - 139, The setFile method on the Document
class currently assigns an unreadable path to $this->file; change it to validate
the provided $filePath (using PHP functions like file_exists and is_readable)
and throw a clear exception (e.g., InvalidArgumentException) when the path is
missing or not readable so callers fail fast; update Document::setFile to
perform this check and only assign $this->file when the path passes validation,
ensuring later payload construction won't silently omit the file.
yeu1
force-pushed
the
move-to-content-endpoint
branch
2 times, most recently
from
3654ddc to
b9a64fc
Compare
yeu1
force-pushed
the
move-to-content-endpoint
branch
from
b9a64fc to
deef0d9
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Document.php (1)
138-149:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden file setter and enforce single upload source.
On Line 140 and Line 148, the setters can leave both
$fileand$pdfFilepopulated, andsetFile()still accepts unreadable paths. That can produce ambiguous create payloads and late upload failures.Suggested fix
public function setFile(string $filePath): void { + if (!is_file($filePath) || !is_readable($filePath)) { + throw new \InvalidArgumentException('Document file is not readable: ' . $filePath); + } $this->file = $filePath; + $this->pdfFile = null; } /** * `@deprecated` Use setFile() instead. */ public function setPdfFile($pdfFile) { $this->pdfFile = $pdfFile; + $this->file = null; }🤖 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/Document.php` around lines 138 - 149, Ensure the setters enforce a single source and validate the path: modify setFile(string $filePath) to verify the file path is readable (throw or return error on invalid/unreadable paths) and clear any legacy $pdfFile when a new $file is set; likewise update deprecated setPdfFile($pdfFile) to validate its input and clear $file when used (or forward to setFile after validation) so both properties cannot be populated simultaneously and unreadable paths are rejected; refer to methods setFile and setPdfFile in Document to locate and implement these changes.
🧹 Nitpick comments (1)
examples/casefile-e2e-demo.php (1)
264-270: ⚡ Quick winUse unsigned content in the demo to reduce expected skips.
On Line 264,
getContent()defaults to signed content, which usually is unavailable at this point in the script flow. Prefer fetching unsigned content so the example succeeds more consistently.Suggested tweak
- $binary = $document->getContent(); + $binary = $document->getContent(false);🤖 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 `@examples/casefile-e2e-demo.php` around lines 264 - 270, The demo currently calls $document->getContent() which defaults to signed content and often fails; change the call to request unsigned content (e.g. $binary = $document->getContent(false)) so the example downloads the unsigned payload consistently, keep the existing error catch and $document->getFormat() handling unchanged.
🤖 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.
Duplicate comments:
In `@src/Document.php`:
- Around line 138-149: Ensure the setters enforce a single source and validate
the path: modify setFile(string $filePath) to verify the file path is readable
(throw or return error on invalid/unreadable paths) and clear any legacy
$pdfFile when a new $file is set; likewise update deprecated
setPdfFile($pdfFile) to validate its input and clear $file when used (or forward
to setFile after validation) so both properties cannot be populated
simultaneously and unreadable paths are rejected; refer to methods setFile and
setPdfFile in Document to locate and implement these changes.
---
Nitpick comments:
In `@examples/casefile-e2e-demo.php`:
- Around line 264-270: The demo currently calls $document->getContent() which
defaults to signed content and often fails; change the call to request unsigned
content (e.g. $binary = $document->getContent(false)) so the example downloads
the unsigned payload consistently, keep the existing error catch and
$document->getFormat() handling unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 2f4d734b-ae97-475a-85dd-26554dffcaf0
📒 Files selected for processing (6)
CHANGELOG.mdREADME.mddocs/document.mdexamples/casefile-e2e-demo.phpsrc/Document.phpsrc/Entity.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Entity.php
2 participants
No description provided.