Skip to content

feat: add file upload and memo status checking#5

Open
skald-bot wants to merge 1 commit intomainfrom
feat/file-upload-and-status-check
Open

feat: add file upload and memo status checking#5
skald-bot wants to merge 1 commit intomainfrom
feat/file-upload-and-status-check

Conversation

@skald-bot
Copy link
Copy Markdown

Summary

  • Adds file upload support via createMemoFromFile() method for uploading documents (PDF, DOC, DOCX, PPTX up to 100MB)
  • Adds status checking via checkMemoStatus() method to monitor memo processing status
  • Implements features from skald-python commit a7fbaf6

Changes

New Methods

  • createMemoFromFile(string $filePath, ?MemoFileData $memoData): Upload document files with optional metadata
  • checkMemoStatus(string $memoId, string $idType): Check processing status of memos

New Types

  • MemoFileData: Data structure for file upload metadata
  • MemoStatusResponse: Response type with status and convenience methods (isProcessing(), isProcessed(), isError())

Examples

  • examples/upload_file.php: Demonstrates file upload with metadata
  • examples/check_memo_status.php: Shows status checking and polling patterns

Tests

  • Added 10 new unit tests covering file upload and status checking
  • All existing tests pass

Documentation

  • Updated README with comprehensive documentation for both features
  • Added examples and usage patterns including polling

Test plan

  • Unit tests added for all new methods
  • Examples created and verified
  • Documentation updated
  • Follows existing code patterns and style
  • Matches Python SDK interface

🤖 Generated with Claude Code

Add support for uploading documents and checking memo processing status,
matching features from skald-python commit a7fbaf6.

New features:
- createMemoFromFile(): Upload documents (PDF, DOC, DOCX, PPTX) up to 100MB
- checkMemoStatus(): Check memo processing status (processing, processed, error)
- MemoFileData type for file upload metadata
- MemoStatusResponse type with convenience methods (isProcessing, isProcessed, isError)

Testing:
- Added comprehensive unit tests for both features
- Added example scripts: upload_file.php and check_memo_status.php
- Updated README with full documentation and examples

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude bot commented Nov 12, 2025

Pull Request Review: File Upload and Memo Status Checking

Summary

This PR adds file upload support and memo status checking functionality to the Skald PHP SDK. The implementation is well-structured and follows the existing codebase patterns. Overall, this is high-quality code that maintains consistency with the rest of the SDK.

Strengths

Code Quality

  • Excellent adherence to PSR-12 standards with consistent formatting
  • Strong type safety with proper use of strict types, type hints, and readonly properties
  • Comprehensive PHPDoc documentation
  • Follows existing SDK patterns consistently
  • Good error handling with descriptive exceptions

Implementation

  • File upload validation: proper checks for file existence, readability, and size limits (100MB)
  • Clean API design: MemoFileData and MemoStatusResponse types are well-designed
  • Good abstraction: new get() method complements existing HTTP methods

Testing

  • 10 comprehensive tests covering file upload, status checking, error conditions
  • Proper cleanup with try-finally blocks
  • Good test isolation using mock HTTP server

Documentation

  • Excellent README updates with clear examples
  • Practical examples demonstrating real-world usage including polling
  • Helpful inline documentation

Issues and Recommendations

1. Security: Filename Sanitization (Medium Priority)

Location: src/Skald.php:123

The filename is not sanitized before inclusion in multipart form data. Filenames with quotes or special characters could break the format or lead to header injection.

Recommendation: Sanitize filename by removing quotes and newlines before use.

2. Performance: Memory Usage for Large Files (Medium Priority)

Location: src/Skald.php:124

The entire file is loaded into memory with file_get_contents(). For files near 100MB, this causes memory issues on servers with lower limits.

Recommendation: Use CURLFile object for memory-efficient streaming uploads instead of loading entire file.

3. Bug: JSON Encoding Error Handling (Low Priority)

Location: src/Skald.php:139

json_encode() can return false on error, but this is not checked.

Recommendation: Add error handling for JSON encoding failures.

4. Missing Test Coverage (Low Priority)

Tests needed for: file size exceeding 100MB, unreadable files, filesize() failure, JSON encoding failure in metadata.

5. Code Style: MemoFileData Inconsistency (Minor)

MemoData.toArray() defaults metadata to empty array, but MemoFileData.toArray() excludes it if null. Document why behavior differs.

6. Documentation: API Response Flow

Show complete flow from upload to status checking including how to get memo_uuid from upload response.

Security Assessment

Passed: Input validation, authentication, error handling, type safety

Needs attention: Filename sanitization, consider MIME type validation

Performance Considerations

  1. Memory usage: 100MB files require 100MB+ memory
  2. Streaming would be more efficient
  3. Polling interval is reasonable but consider exponential backoff

Overall Assessment

Recommendation: APPROVE with minor changes

  • Follows all project conventions
  • Includes comprehensive tests (10 new)
  • Excellent documentation
  • Maintains API consistency
  • Few minor issues to address

Priority:

  1. Must fix: filename sanitization
  2. Should fix: memory usage optimization
  3. Can follow up: issues 3-6

Great work! The code quality is excellent and this will be a valuable addition to the SDK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant