feat: ability to upload and work with HEIF images by convert them to JPEG #3946
Conversation
- Introduced `HeifToJpeg` class for converting HEIC/HEIF images to JPEG format. - Added `ImageTypeFactory` to determine conversion classes based on file extensions. - Implemented `ConvertUnsupportedMedia` pipe to handle unsupported media conversions. - Created `ConvertMediaFileInterface` for consistent conversion method signatures. - Added `CannotConvertMediaFileException` for error handling during conversion. - Updated `Create` action to include the new conversion pipe. - Introduced `ConvertableImageType` enum to manage image type checks.
- Updated variable names to use snake case in `HeifToJpeg` and `ConvertMediaFileInterface` for consistency and clarity.
…ecause titles over 100 chars throws error when uploading images
…000) in photos table for improved title length handling
…o TEXT in photos table, updating index to accommodate new column type and ensuring proper handling of long titles.
…existence check before dropping it, ensuring smoother migration process.
…o-jpg - Updated `composer.json` and `composer.lock` to include the new dependency. - Enhanced `HeifToJpeg` class to utilize the `HeicToJpg` library for conversion. - Modified methods to handle both Imagick and HeicToJpg instances for image processing.
…ception messages for better debugging.
…g instances for image processing
📝 WalkthroughWalkthroughAdds HEIC/HEIF → JPEG conversion: new conversion interface and HeifToJpeg implementation (Imagick with maestroerror fallback), ImageTypeFactory, pipeline pipe to auto-convert unsupported media, enums and exception, composer dependency, unit tests, and a migration changing photos.title to TEXT. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.php📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
🧠 Learnings (11)📚 Learning: 2025-08-18T10:19:04.946ZApplied to files:
📚 Learning: 2025-12-22T14:12:18.082ZApplied to files:
📚 Learning: 2025-08-20T20:32:03.676ZApplied to files:
📚 Learning: 2025-08-24T14:33:37.747ZApplied to files:
📚 Learning: 2025-09-21T20:09:31.762ZApplied to files:
📚 Learning: 2025-12-22T14:11:17.217ZApplied to files:
📚 Learning: 2025-12-22T14:12:18.082ZApplied to files:
📚 Learning: 2025-12-22T14:12:18.082ZApplied to files:
📚 Learning: 2025-12-19T21:01:40.134ZApplied to files:
📚 Learning: 2025-12-22T14:12:18.082ZApplied to files:
📚 Learning: 2025-12-28T18:12:55.752ZApplied to files:
🔇 Additional comments (4)
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 |
…#298-HEIC-HEIF # Conflicts: # composer.lock
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (2)
app/Actions/Photo/Convert/ImageTypeFactory.php (1)
15-24: Consider making the property readonly and using FQCN.The public mutable property
$convertionClass(after fixing the typo to$conversionClass) could be made readonly since it's only set in the constructor. Additionally, storing the fully-qualified class name (FQCN) instead of just the class name would eliminate fragile string concatenation in themake()method.🔎 Suggested refactor
- public ?string $convertionClass = null; + public readonly ?string $converterClass; public function __construct(string $extension) { - $this->convertionClass = match (true) { - ConvertableImageType::isHeifImageType($extension) => 'HeifToJpeg', + $this->converterClass = match (true) { + ConvertableImageType::isHeifImageType($extension) => HeifToJpeg::class, // TODO: Add more convertion types/classes default => null, }; }Then simplify the
make()method:public function make(): ConvertMediaFileInterface { - $class = 'App\Actions\Photo\Convert\\' . $this->convertionClass; + if ($this->converterClass === null) { + throw new \RuntimeException('No conversion class available for this file type'); + } + + $instance = new ($this->converterClass)(); - return new $class(); + if (!$instance instanceof ConvertMediaFileInterface) { + throw new \RuntimeException("Converter must implement ConvertMediaFileInterface"); + } + + return $instance; }app/Actions/Photo/Pipes/Init/ConvertUnsupportedMedia.php (1)
27-31: Typo: "convertionClass" should be "conversionClass".The property name in
ImageTypeFactorycontains a spelling error. While consistent across files, correcting it would improve code readability.This would require updating
ImageTypeFactory.phpas well:- if ($factory->convertionClass === null) { + if ($factory->conversionClass === null) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/Actions/Photo/Convert/HeifToJpeg.phpapp/Actions/Photo/Convert/ImageTypeFactory.phpapp/Actions/Photo/Create.phpapp/Actions/Photo/Pipes/Init/ConvertUnsupportedMedia.phpapp/Contracts/Image/ConvertMediaFileInterface.phpapp/Enum/ConvertableImageType.phpapp/Enum/ImageType.phpapp/Exceptions/CannotConvertMediaFileException.phpcomposer.jsondatabase/migrations/2025_12_22_105523_change_title_to_text_on_photos_table.php
🧰 Additional context used
📓 Path-based instructions (1)
**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.php: Any new PHP file should contain the license header and have a single blank line after the opening PHP tag
Variable names should be in snake_case in PHP
Apply the PSR-4 coding standard in PHP
Use in_array() with true as the third parameter in PHP
Only use booleans in if statements, not integers or strings
Use strict comparison (===) instead of loose comparison (==)
Avoid code duplication in both if and else statements
Do not use empty() in PHP
Use the moneyphp/money library for handling monetary values in PHP
Never use floats or doubles to represent monetary values; use integers representing the smallest currency unit (e.g., cents for USD)
**/*.php: Write or extend executable specifications (unit, behaviour, or scenario tests) ahead of implementation, confirm they fail, and then drive code to green before refactoring. List the expected success, validation, and failure branches and add thin failing tests for each path.
For PHP code, adhere to conventions: license headers in new files, strict comparison (===), no empty(), in_array() with third parameter true, snake_case variables, PSR-4 standard, test base classes (AbstractTestCase for Unit, BaseApiWithDataTest for Feature_v2).
Always run phpunit tests. If a test remains red, disable it with a TODO, note the reason, and capture the follow-up in the relevant plan.
Spotless now uses Palantir Java Format 2.78.0 with a 120-character wrap; configure IDE formatters to match before pushing code changes.
Keep each increment's control flow flat by delegating validation/normalisation into tiny pure helpers that return simple enums or result records, then compose them instead of introducing inline branching that inflates the branch count per change.
When introducing new helpers/utilities or editing files prone to style violations (records, DTOs, generated adapters), run the narrowest applicable lint target (for example phpstan) before the full pipeline. Note the command in the related plan/task.
For PHP changes, ru...
Files:
app/Exceptions/CannotConvertMediaFileException.phpapp/Enum/ConvertableImageType.phpapp/Enum/ImageType.phpapp/Actions/Photo/Pipes/Init/ConvertUnsupportedMedia.phpapp/Actions/Photo/Convert/HeifToJpeg.phpapp/Actions/Photo/Convert/ImageTypeFactory.phpapp/Contracts/Image/ConvertMediaFileInterface.phpdatabase/migrations/2025_12_22_105523_change_title_to_text_on_photos_table.phpapp/Actions/Photo/Create.php
🧠 Learnings (5)
📓 Common learnings
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3626
File: database/migrations/2025_06_07_144157_photo_tags_to_table.php:87-105
Timestamp: 2025-08-18T10:19:04.946Z
Learning: In the Lychee photo management system, the migration `2025_06_07_144157_photo_tags_to_table.php` runs on data that only contains comma-separated tag names in tag_albums.show_tags - OR/AND expressions do not exist at this migration time, so handling them is unnecessary.
📚 Learning: 2025-08-25T08:48:12.321Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 2562
File: app/Exceptions/Internal/ZipExtractionException.php:9-17
Timestamp: 2025-08-25T08:48:12.321Z
Learning: In the Lychee codebase, LycheeDomainException exists in the App\Exceptions\Internal namespace. Exception classes within the same App\Exceptions\Internal namespace can extend LycheeDomainException without requiring explicit use statements, following standard PHP namespace resolution rules.
Applied to files:
app/Exceptions/CannotConvertMediaFileException.php
📚 Learning: 2025-08-25T19:07:01.275Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 2562
File: tests/Feature_v2/Photo/PhotoZipUploadTest.php:81-95
Timestamp: 2025-08-25T19:07:01.275Z
Learning: In the Lychee codebase, when testing ZIP file uploads in PhotoZipUploadTest, Laravel middleware does not trigger for ZipInvalidException, so the exception is thrown directly to PHPUnit rather than being converted to an HTTP response. This means using $this->expectException(ZipInvalidException::class) is the correct approach rather than asserting HTTP status codes.
Applied to files:
app/Exceptions/CannotConvertMediaFileException.php
📚 Learning: 2025-12-28T18:12:55.752Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3901
File: app/Providers/AppServiceProvider.php:0-0
Timestamp: 2025-12-28T18:12:55.752Z
Learning: When using Laravel Octane's tick API, Octane::tick(...) returns an InvokeTickCallable that only has ->seconds(int) and ->immediate() methods. There is no ->every(N) method. Use the correct usage: Octane::tick('name', fn() => ...)->seconds(N) or Octane::tick('name', fn() => ..., N). Apply this guideline to PHP files across the project (not just AppServiceProvider.php).
Applied to files:
app/Exceptions/CannotConvertMediaFileException.phpapp/Enum/ConvertableImageType.phpapp/Enum/ImageType.phpapp/Actions/Photo/Pipes/Init/ConvertUnsupportedMedia.phpapp/Actions/Photo/Convert/HeifToJpeg.phpapp/Actions/Photo/Convert/ImageTypeFactory.phpapp/Contracts/Image/ConvertMediaFileInterface.phpdatabase/migrations/2025_12_22_105523_change_title_to_text_on_photos_table.phpapp/Actions/Photo/Create.php
📚 Learning: 2025-08-18T10:19:04.946Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3626
File: database/migrations/2025_06_07_144157_photo_tags_to_table.php:87-105
Timestamp: 2025-08-18T10:19:04.946Z
Learning: In the Lychee photo management system, the migration `2025_06_07_144157_photo_tags_to_table.php` runs on data that only contains comma-separated tag names in tag_albums.show_tags - OR/AND expressions do not exist at this migration time, so handling them is unnecessary.
Applied to files:
database/migrations/2025_12_22_105523_change_title_to_text_on_photos_table.php
🧬 Code graph analysis (4)
app/Enum/ConvertableImageType.php (1)
app/DTO/ImportEventReport.php (1)
str(93-96)
app/Actions/Photo/Pipes/Init/ConvertUnsupportedMedia.php (5)
app/Actions/Photo/Convert/ImageTypeFactory.php (2)
ImageTypeFactory(13-32)make(26-31)app/DTO/PhotoCreate/InitDTO.php (1)
InitDTO(18-61)app/Exceptions/CannotConvertMediaFileException.php (1)
CannotConvertMediaFileException(18-26)app/Contracts/Image/ConvertMediaFileInterface.php (1)
handle(17-17)app/Actions/Photo/Convert/HeifToJpeg.php (1)
handle(24-45)
app/Actions/Photo/Convert/ImageTypeFactory.php (1)
app/Enum/ConvertableImageType.php (1)
isHeifImageType(16-24)
app/Actions/Photo/Create.php (1)
app/Actions/Photo/Pipes/Init/ConvertUnsupportedMedia.php (1)
ConvertUnsupportedMedia(16-41)
🔇 Additional comments (5)
app/Actions/Photo/Create.php (1)
82-88: LGTM! Proper placement in the pipeline.The new
ConvertUnsupportedMediapipe is correctly positioned beforeAssertSupportedMedia, allowing unsupported formats to be converted before validation. The error handling in the pipe properly throwsCannotConvertMediaFileExceptionfor conversion failures.app/Enum/ImageType.php (1)
1-19: LGTM! Well-structured enum.The enum correctly maps to PHP's native
IMAGETYPE_*constants, providing type-safe image type representation. The implementation follows all coding guidelines including proper license headers and formatting.app/Enum/ConvertableImageType.php (1)
1-25: LGTM!The enum implementation is clean and follows best practices:
- License header present with correct format
in_array()correctly usestrueas the third parameter for strict comparison- The
str()->lower()->toString()approach properly handles case-insensitive extension matchingapp/Exceptions/CannotConvertMediaFileException.php (1)
1-26: LGTM!The exception class is well-structured:
- Appropriate HTTP 422 status for media conversion failures
- Follows the existing
BaseLycheeExceptionpattern in the codebase- Clean constructor with optional message and previous exception chaining
app/Actions/Photo/Convert/HeifToJpeg.php (1)
80-103: Solid fallback strategy for HEIC/HEIF conversion.The implementation correctly handles the known limitation where ImageMagick may fail on newer iPhone images by falling back to the
php-heic-to-jpglibrary. The multi-frame handling and auto-orientation are good practices for preserving image quality.The
maestroerror/php-heic-to-jpgpackage is compatible with PHP 7.4 and above. Ensure the installed version is 1.0.5 or later to avoid the RCE vulnerability (CVE-2024-48514) present in earlier versions.
| { | ||
| public function handle(NativeLocalFile $tmp_file): TemporaryJobFile; | ||
|
|
||
| public function storeNewImage(\Imagick|HeicToJpg $image_instance, string $store_to_path); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add explicit return type and reconsider the tight coupling.
The storeNewImage method is missing a return type declaration. Additionally, the union type \Imagick|HeicToJpg tightly couples this interface to specific implementation classes. Consider accepting a more generic type or creating an abstraction to reduce coupling.
🔎 Suggested refactor
- public function storeNewImage(\Imagick|HeicToJpg $image_instance, string $store_to_path);
+ public function storeNewImage(\Imagick|HeicToJpg $image_instance, string $store_to_path): void;For better design, consider creating an adapter interface:
interface ImageResourceInterface {
public function save(string $path): void;
}Then change the signature to:
public function storeNewImage(ImageResourceInterface $image_instance, string $store_to_path): void;As per coding guidelines, all methods should have explicit return types.
database/migrations/2025_12_22_105523_change_title_to_text_on_photos_table.php
Show resolved
Hide resolved
database/migrations/2025_12_22_105523_change_title_to_text_on_photos_table.php
Outdated
Show resolved
Hide resolved
database/migrations/2025_12_22_105523_change_title_to_text_on_photos_table.php
Outdated
Show resolved
Hide resolved
database/migrations/2025_12_22_105523_change_title_to_text_on_photos_table.php
Outdated
Show resolved
Hide resolved
- Bump `maestroerror/php-heic-to-jpg` dependency version in `composer.json`. - Refactor `HeifToJpeg` class to use filename instead of basename for output path. - Enhance `ImageTypeFactory` to ensure conversion class validity and implement interface checks. - Improve error handling in `ConvertUnsupportedMedia` by including the original exception in the thrown `CannotConvertMediaFileException`. - Update migration for 'title' column in photos table to allow null values and adjust indexing accordingly.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
app/Actions/Photo/Pipes/Init/ConvertUnsupportedMedia.php (1)
27-31: Prefer encapsulated check over direct property access.Accessing
$factory->conversionClassdirectly exposes internal factory state and couples the pipe to implementation details. Consider adding a method toImageTypeFactorysuch ascanConvert(): boolorhasConverter(): boolfor better encapsulation.🔎 Example refactor in ImageTypeFactory
Add a method to
ImageTypeFactory:public function canConvert(): bool { return $this->conversionClass !== null; }Then update this pipe:
$factory = new ImageTypeFactory($ext); -if ($factory->conversionClass === null) { +if (!$factory->canConvert()) { return $next($state); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/Actions/Photo/Convert/HeifToJpeg.phpapp/Actions/Photo/Convert/ImageTypeFactory.phpapp/Actions/Photo/Pipes/Init/ConvertUnsupportedMedia.phpcomposer.jsondatabase/migrations/2025_12_22_105523_change_title_to_text_on_photos_table.php
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Actions/Photo/Convert/ImageTypeFactory.php
🧰 Additional context used
📓 Path-based instructions (1)
**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.php: Any new PHP file should contain the license header and have a single blank line after the opening PHP tag
Variable names should be in snake_case in PHP
Apply the PSR-4 coding standard in PHP
Use in_array() with true as the third parameter in PHP
Only use booleans in if statements, not integers or strings
Use strict comparison (===) instead of loose comparison (==)
Avoid code duplication in both if and else statements
Do not use empty() in PHP
Use the moneyphp/money library for handling monetary values in PHP
Never use floats or doubles to represent monetary values; use integers representing the smallest currency unit (e.g., cents for USD)
**/*.php: Write or extend executable specifications (unit, behaviour, or scenario tests) ahead of implementation, confirm they fail, and then drive code to green before refactoring. List the expected success, validation, and failure branches and add thin failing tests for each path.
For PHP code, adhere to conventions: license headers in new files, strict comparison (===), no empty(), in_array() with third parameter true, snake_case variables, PSR-4 standard, test base classes (AbstractTestCase for Unit, BaseApiWithDataTest for Feature_v2).
Always run phpunit tests. If a test remains red, disable it with a TODO, note the reason, and capture the follow-up in the relevant plan.
Spotless now uses Palantir Java Format 2.78.0 with a 120-character wrap; configure IDE formatters to match before pushing code changes.
Keep each increment's control flow flat by delegating validation/normalisation into tiny pure helpers that return simple enums or result records, then compose them instead of introducing inline branching that inflates the branch count per change.
When introducing new helpers/utilities or editing files prone to style violations (records, DTOs, generated adapters), run the narrowest applicable lint target (for example phpstan) before the full pipeline. Note the command in the related plan/task.
For PHP changes, ru...
Files:
app/Actions/Photo/Pipes/Init/ConvertUnsupportedMedia.phpapp/Actions/Photo/Convert/HeifToJpeg.phpdatabase/migrations/2025_12_22_105523_change_title_to_text_on_photos_table.php
🧠 Learnings (4)
📚 Learning: 2025-12-28T18:12:55.752Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3901
File: app/Providers/AppServiceProvider.php:0-0
Timestamp: 2025-12-28T18:12:55.752Z
Learning: When using Laravel Octane's tick API, Octane::tick(...) returns an InvokeTickCallable that only has ->seconds(int) and ->immediate() methods. There is no ->every(N) method. Use the correct usage: Octane::tick('name', fn() => ...)->seconds(N) or Octane::tick('name', fn() => ..., N). Apply this guideline to PHP files across the project (not just AppServiceProvider.php).
Applied to files:
app/Actions/Photo/Pipes/Init/ConvertUnsupportedMedia.phpapp/Actions/Photo/Convert/HeifToJpeg.phpdatabase/migrations/2025_12_22_105523_change_title_to_text_on_photos_table.php
📚 Learning: 2025-08-18T10:19:04.946Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3626
File: database/migrations/2025_06_07_144157_photo_tags_to_table.php:87-105
Timestamp: 2025-08-18T10:19:04.946Z
Learning: In the Lychee photo management system, the migration `2025_06_07_144157_photo_tags_to_table.php` runs on data that only contains comma-separated tag names in tag_albums.show_tags - OR/AND expressions do not exist at this migration time, so handling them is unnecessary.
Applied to files:
database/migrations/2025_12_22_105523_change_title_to_text_on_photos_table.php
📚 Learning: 2025-12-22T14:12:18.082Z
Learnt from: CR
Repo: LycheeOrg/Lychee PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T14:12:18.082Z
Learning: Applies to **/*.php : For PHP code, adhere to conventions: license headers in new files, strict comparison (===), no empty(), in_array() with third parameter true, snake_case variables, PSR-4 standard, test base classes (AbstractTestCase for Unit, BaseApiWithDataTest for Feature_v2).
Applied to files:
database/migrations/2025_12_22_105523_change_title_to_text_on_photos_table.php
📚 Learning: 2025-08-20T20:32:03.676Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3637
File: database/migrations/2025_08_19_160144_create_renamer_rules_table.php:106-108
Timestamp: 2025-08-20T20:32:03.676Z
Learning: In the Lychee codebase, ildyria intentionally uses destructive migration patterns where up() calls down() to ensure clean state, as indicated by "Working as intended" and "No mercy" comments in migrations like database/migrations/2025_08_19_160144_create_renamer_rules_table.php.
Applied to files:
database/migrations/2025_12_22_105523_change_title_to_text_on_photos_table.php
🧬 Code graph analysis (1)
app/Actions/Photo/Pipes/Init/ConvertUnsupportedMedia.php (5)
app/Actions/Photo/Convert/ImageTypeFactory.php (2)
ImageTypeFactory(14-47)make(27-46)app/DTO/PhotoCreate/InitDTO.php (1)
InitDTO(18-61)app/Exceptions/CannotConvertMediaFileException.php (1)
CannotConvertMediaFileException(18-26)app/Actions/Photo/Convert/HeifToJpeg.php (1)
handle(24-45)app/Contracts/Image/ConvertMediaFileInterface.php (1)
handle(17-17)
🔇 Additional comments (4)
composer.json (1)
71-71: LGTM! Dependency constraint addresses critical vulnerability.The version constraint
^1.0.5correctly excludes versions affected by CVE-2024-48514 (critical RCE vulnerability). Latest version v1.0.8 is available and will be installed.app/Actions/Photo/Pipes/Init/ConvertUnsupportedMedia.php (1)
33-37: LGTM! Exception chain properly preserved.The conversion logic correctly wraps exceptions in
CannotConvertMediaFileExceptionwhile preserving the original exception chain for debugging.app/Actions/Photo/Convert/HeifToJpeg.php (2)
24-45: LGTM! Conversion workflow is well-structured.The handle method implements a clean workflow: convert → store → delete → return. Exception handling correctly preserves the original error message for debugging.
80-103: LGTM! Robust conversion with appropriate fallback.The conversion method correctly:
- Handles multi-frame images by selecting the first frame
- Applies appropriate JPEG settings (quality 92, auto-orient)
- Falls back to HeicToJpg library when Imagick fails
- Preserves error messages from both conversion attempts (line 100)
database/migrations/2025_12_22_105523_change_title_to_text_on_photos_table.php
Show resolved
Hide resolved
database/migrations/2025_12_22_105523_change_title_to_text_on_photos_table.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/Actions/Photo/Pipes/Init/ConvertUnsupportedMedia.php (1)
18-22: Update docblock to reflect actual exception type.The docblock specifies
@throws \Exceptionbut the method actually throwsCannotConvertMediaFileException(line 36), which is more specific and informative.🔎 Proposed fix
/** * Tries to convert the file to a supported format. * - * @throws \Exception + * @throws CannotConvertMediaFileException */app/Actions/Photo/Convert/ImageTypeFactory.php (1)
16-16: Consider making the property readonly.Since
$conversionClassis set in the constructor and never modified afterward, declaring it asreadonlywould better express its immutability.🔎 Proposed change
- public ?string $conversionClass = null; + public readonly ?string $conversionClass;Note: You'll also need to adjust the constructor to set it via parameter or assignment, as readonly properties must be initialized during construction.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
app/Actions/Photo/Convert/HeifToJpeg.phpapp/Actions/Photo/Convert/ImageTypeFactory.phpapp/Actions/Photo/Pipes/Init/ConvertUnsupportedMedia.phpapp/Contracts/Image/ConvertMediaFileInterface.phpapp/Enum/ConvertableImageType.phpapp/Enum/ImageType.phpapp/Exceptions/CannotConvertMediaFileException.phpdatabase/migrations/2025_12_22_105523_change_title_to_text_on_photos_table.php
🚧 Files skipped from review as they are similar to previous changes (5)
- app/Contracts/Image/ConvertMediaFileInterface.php
- app/Exceptions/CannotConvertMediaFileException.php
- app/Actions/Photo/Convert/HeifToJpeg.php
- database/migrations/2025_12_22_105523_change_title_to_text_on_photos_table.php
- app/Enum/ConvertableImageType.php
🧰 Additional context used
📓 Path-based instructions (1)
**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.php: Any new PHP file should contain the license header and have a single blank line after the opening PHP tag
Variable names should be in snake_case in PHP
Apply the PSR-4 coding standard in PHP
Use in_array() with true as the third parameter in PHP
Only use booleans in if statements, not integers or strings
Use strict comparison (===) instead of loose comparison (==)
Avoid code duplication in both if and else statements
Do not use empty() in PHP
Use the moneyphp/money library for handling monetary values in PHP
Never use floats or doubles to represent monetary values; use integers representing the smallest currency unit (e.g., cents for USD)
**/*.php: Write or extend executable specifications (unit, behaviour, or scenario tests) ahead of implementation, confirm they fail, and then drive code to green before refactoring. List the expected success, validation, and failure branches and add thin failing tests for each path.
For PHP code, adhere to conventions: license headers in new files, strict comparison (===), no empty(), in_array() with third parameter true, snake_case variables, PSR-4 standard, test base classes (AbstractTestCase for Unit, BaseApiWithDataTest for Feature_v2).
Always run phpunit tests. If a test remains red, disable it with a TODO, note the reason, and capture the follow-up in the relevant plan.
Spotless now uses Palantir Java Format 2.78.0 with a 120-character wrap; configure IDE formatters to match before pushing code changes.
Keep each increment's control flow flat by delegating validation/normalisation into tiny pure helpers that return simple enums or result records, then compose them instead of introducing inline branching that inflates the branch count per change.
When introducing new helpers/utilities or editing files prone to style violations (records, DTOs, generated adapters), run the narrowest applicable lint target (for example phpstan) before the full pipeline. Note the command in the related plan/task.
For PHP changes, ru...
Files:
app/Actions/Photo/Pipes/Init/ConvertUnsupportedMedia.phpapp/Enum/ImageType.phpapp/Actions/Photo/Convert/ImageTypeFactory.php
🧠 Learnings (1)
📚 Learning: 2025-12-28T18:12:55.752Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3901
File: app/Providers/AppServiceProvider.php:0-0
Timestamp: 2025-12-28T18:12:55.752Z
Learning: When using Laravel Octane's tick API, Octane::tick(...) returns an InvokeTickCallable that only has ->seconds(int) and ->immediate() methods. There is no ->every(N) method. Use the correct usage: Octane::tick('name', fn() => ...)->seconds(N) or Octane::tick('name', fn() => ..., N). Apply this guideline to PHP files across the project (not just AppServiceProvider.php).
Applied to files:
app/Actions/Photo/Pipes/Init/ConvertUnsupportedMedia.phpapp/Enum/ImageType.phpapp/Actions/Photo/Convert/ImageTypeFactory.php
🧬 Code graph analysis (2)
app/Actions/Photo/Pipes/Init/ConvertUnsupportedMedia.php (5)
app/Actions/Photo/Convert/ImageTypeFactory.php (2)
ImageTypeFactory(14-47)make(27-46)app/DTO/PhotoCreate/InitDTO.php (1)
InitDTO(18-61)app/Exceptions/CannotConvertMediaFileException.php (1)
CannotConvertMediaFileException(18-26)app/Actions/Photo/Convert/HeifToJpeg.php (1)
handle(24-45)app/Contracts/Image/ConvertMediaFileInterface.php (1)
handle(17-17)
app/Actions/Photo/Convert/ImageTypeFactory.php (1)
app/Enum/ConvertableImageType.php (1)
isHeifImageType(16-24)
🔇 Additional comments (2)
app/Enum/ImageType.php (1)
11-18: LGTM! Clean enum implementation.The enum correctly maps to PHP's built-in
IMAGETYPE_*constants and provides a type-safe way to represent supported image types.app/Actions/Photo/Convert/ImageTypeFactory.php (1)
27-46: Write tests for ImageTypeFactory following test-first development practices.No tests currently exist for ImageTypeFactory. Per coding guidelines requiring test-driven development, add unit tests covering:
- HEIF/HEIC extension identification and converter instantiation
- Exception handling for null conversion class (unsupported extensions)
- Exception handling for missing converter class
- Exception handling for invalid ConvertMediaFileInterface implementation
…ror handling and testing - Change `conversionClass` property in `ImageTypeFactory` to be readonly. - Update exception type in `ConvertUnsupportedMedia` to `CannotConvertMediaFileException`. - Add unit tests for `ConvertUnsupportedMedia` to ensure proper handling of various image formats and conversion scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/Unit/Actions/Photo/Pipes/Init/ConvertUnsupportedMediaTest.php (2)
46-116: Consider using a data provider to reduce duplication in passthrough tests.The three passthrough tests (
testHandleWithJpegExtensionPassesThrough,testHandleWithPngExtensionPassesThrough,testHandleWithExtensionWithoutDotPassesThrough) have nearly identical logic with only the extension value differing.🔎 Proposed refactor using data provider
+ /** + * @dataProvider passthroughExtensionsProvider + */ + public function testHandleWithNonConvertibleExtensionPassesThrough(string $extension): void + { + $originalFile = \Mockery::mock(NativeLocalFile::class); + + $originalFile->shouldReceive('getOriginalExtension') + ->once() + ->andReturn($extension); + + $state = $this->createInitDTO($originalFile); + + $nextCalled = false; + $next = function (InitDTO $state) use (&$nextCalled, $originalFile): InitDTO { + $nextCalled = true; + self::assertSame($originalFile, $state->source_file); + + return $state; + }; + + $result = $this->pipe->handle($state, $next); + + $this->assertTrue($nextCalled); + $this->assertSame($originalFile, $result->source_file); + } + + public static function passthroughExtensionsProvider(): array + { + return [ + 'jpeg with dot' => ['.jpg'], + 'png with dot' => ['.png'], + 'jpeg without dot' => ['jpg'], + ]; + }
118-149: Unused mock variable on line 121.The
$convertedFilemock is created but never used in the test. This appears to be leftover code from a different test approach.🔎 Proposed fix
public function testHandleWithHeifExtensionTriggersConversion(): void { $originalFile = \Mockery::mock(NativeLocalFile::class); - $convertedFile = \Mockery::mock(TemporaryJobFile::class); $originalFile->shouldReceive('getOriginalExtension')app/Actions/Photo/Convert/ImageTypeFactory.php (1)
22-22: Fix typo in comment: "convertion" → "conversion".🔎 Proposed fix
- // TODO: Add more convertion types/classes + // TODO: Add more conversion types/classes
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/Actions/Photo/Convert/ImageTypeFactory.phpapp/Actions/Photo/Pipes/Init/ConvertUnsupportedMedia.phptests/Unit/Actions/Photo/Pipes/Init/ConvertUnsupportedMediaTest.php
🧰 Additional context used
📓 Path-based instructions (3)
**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.php: Any new PHP file should contain the license header and have a single blank line after the opening PHP tag
Variable names should be in snake_case in PHP
Apply the PSR-4 coding standard in PHP
Use in_array() with true as the third parameter in PHP
Only use booleans in if statements, not integers or strings
Use strict comparison (===) instead of loose comparison (==)
Avoid code duplication in both if and else statements
Do not use empty() in PHP
Use the moneyphp/money library for handling monetary values in PHP
Never use floats or doubles to represent monetary values; use integers representing the smallest currency unit (e.g., cents for USD)
**/*.php: Write or extend executable specifications (unit, behaviour, or scenario tests) ahead of implementation, confirm they fail, and then drive code to green before refactoring. List the expected success, validation, and failure branches and add thin failing tests for each path.
For PHP code, adhere to conventions: license headers in new files, strict comparison (===), no empty(), in_array() with third parameter true, snake_case variables, PSR-4 standard, test base classes (AbstractTestCase for Unit, BaseApiWithDataTest for Feature_v2).
Always run phpunit tests. If a test remains red, disable it with a TODO, note the reason, and capture the follow-up in the relevant plan.
Spotless now uses Palantir Java Format 2.78.0 with a 120-character wrap; configure IDE formatters to match before pushing code changes.
Keep each increment's control flow flat by delegating validation/normalisation into tiny pure helpers that return simple enums or result records, then compose them instead of introducing inline branching that inflates the branch count per change.
When introducing new helpers/utilities or editing files prone to style violations (records, DTOs, generated adapters), run the narrowest applicable lint target (for example phpstan) before the full pipeline. Note the command in the related plan/task.
For PHP changes, ru...
Files:
app/Actions/Photo/Convert/ImageTypeFactory.phpapp/Actions/Photo/Pipes/Init/ConvertUnsupportedMedia.phptests/Unit/Actions/Photo/Pipes/Init/ConvertUnsupportedMediaTest.php
tests/Unit/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Tests in the tests/Unit directory should extend from AbstractTestCase
Files:
tests/Unit/Actions/Photo/Pipes/Init/ConvertUnsupportedMediaTest.php
tests/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
No need to mock the database in tests; use the in-memory SQLite database instead
Files:
tests/Unit/Actions/Photo/Pipes/Init/ConvertUnsupportedMediaTest.php
🧠 Learnings (6)
📚 Learning: 2025-12-28T18:12:55.752Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3901
File: app/Providers/AppServiceProvider.php:0-0
Timestamp: 2025-12-28T18:12:55.752Z
Learning: When using Laravel Octane's tick API, Octane::tick(...) returns an InvokeTickCallable that only has ->seconds(int) and ->immediate() methods. There is no ->every(N) method. Use the correct usage: Octane::tick('name', fn() => ...)->seconds(N) or Octane::tick('name', fn() => ..., N). Apply this guideline to PHP files across the project (not just AppServiceProvider.php).
Applied to files:
app/Actions/Photo/Convert/ImageTypeFactory.phpapp/Actions/Photo/Pipes/Init/ConvertUnsupportedMedia.phptests/Unit/Actions/Photo/Pipes/Init/ConvertUnsupportedMediaTest.php
📚 Learning: 2025-12-22T14:12:18.082Z
Learnt from: CR
Repo: LycheeOrg/Lychee PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T14:12:18.082Z
Learning: Applies to **/*.php : Write or extend executable specifications (unit, behaviour, or scenario tests) ahead of implementation, confirm they fail, and then drive code to green before refactoring. List the expected success, validation, and failure branches and add thin failing tests for each path.
Applied to files:
app/Actions/Photo/Pipes/Init/ConvertUnsupportedMedia.php
📚 Learning: 2025-12-22T14:12:18.082Z
Learnt from: CR
Repo: LycheeOrg/Lychee PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T14:12:18.082Z
Learning: Applies to **/*.php : Always run phpunit tests. If a test remains red, disable it with a TODO, note the reason, and capture the follow-up in the relevant plan.
Applied to files:
app/Actions/Photo/Pipes/Init/ConvertUnsupportedMedia.php
📚 Learning: 2025-08-25T19:07:01.275Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 2562
File: tests/Feature_v2/Photo/PhotoZipUploadTest.php:81-95
Timestamp: 2025-08-25T19:07:01.275Z
Learning: In the Lychee codebase, when testing ZIP file uploads in PhotoZipUploadTest, Laravel middleware does not trigger for ZipInvalidException, so the exception is thrown directly to PHPUnit rather than being converted to an HTTP response. This means using $this->expectException(ZipInvalidException::class) is the correct approach rather than asserting HTTP status codes.
Applied to files:
app/Actions/Photo/Pipes/Init/ConvertUnsupportedMedia.phptests/Unit/Actions/Photo/Pipes/Init/ConvertUnsupportedMediaTest.php
📚 Learning: 2025-12-22T14:12:18.082Z
Learnt from: CR
Repo: LycheeOrg/Lychee PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T14:12:18.082Z
Learning: Applies to **/*.php : For PHP code, adhere to conventions: license headers in new files, strict comparison (===), no empty(), in_array() with third parameter true, snake_case variables, PSR-4 standard, test base classes (AbstractTestCase for Unit, BaseApiWithDataTest for Feature_v2).
Applied to files:
app/Actions/Photo/Pipes/Init/ConvertUnsupportedMedia.php
📚 Learning: 2025-12-22T14:12:18.082Z
Learnt from: CR
Repo: LycheeOrg/Lychee PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T14:12:18.082Z
Learning: Applies to **/*.php : For PHP changes, run php artisan test to ensure all tests pass before committing.
Applied to files:
app/Actions/Photo/Pipes/Init/ConvertUnsupportedMedia.php
🧬 Code graph analysis (2)
app/Actions/Photo/Convert/ImageTypeFactory.php (1)
app/Enum/ConvertableImageType.php (1)
isHeifImageType(16-24)
tests/Unit/Actions/Photo/Pipes/Init/ConvertUnsupportedMediaTest.php (6)
app/Actions/Photo/Pipes/Init/ConvertUnsupportedMedia.php (2)
ConvertUnsupportedMedia(16-41)handle(23-40)app/DTO/ImportMode.php (1)
ImportMode(14-41)app/DTO/ImportParam.php (1)
ImportParam(14-33)app/DTO/PhotoCreate/InitDTO.php (1)
InitDTO(18-61)app/Exceptions/CannotConvertMediaFileException.php (1)
CannotConvertMediaFileException(18-26)app/Image/Files/TemporaryJobFile.php (1)
TemporaryJobFile(20-76)
🔇 Additional comments (4)
app/Actions/Photo/Pipes/Init/ConvertUnsupportedMedia.php (1)
1-41: LGTM! Clean implementation of the conversion pipe.The implementation correctly:
- Preserves the exception chain by passing the original exception as the second parameter to
CannotConvertMediaFileException- Uses strict comparison (
===) for null check- Handles extensions with or without leading dots via
ltrim()- Follows the single responsibility principle by delegating conversion to the factory
tests/Unit/Actions/Photo/Pipes/Init/ConvertUnsupportedMediaTest.php (2)
183-243: Good coverage of exception wrapping behavior.The tests properly verify that:
- The original exception is preserved via
getPrevious()- The wrapper message includes context about the conversion failure
- Both base
ExceptionandRuntimeExceptionare handled consistently
245-257: Clean helper method for test setup.The
createInitDTOhelper properly encapsulates the DTO construction, making tests more readable and maintainable.app/Actions/Photo/Convert/ImageTypeFactory.php (1)
27-46: Well-structured factory method with proper guards.The
make()method includes all necessary validations:
- Null check before attempting instantiation
- Class existence verification
- Interface implementation check
This defensive approach prevents runtime errors from bubbling up unexpectedly.
|
Hi, Thank you for your pull request. I really agree that we need to have a pre-processing step that can be further extended. However, I have a few issues that needs to be resolved:
Looking more into details:
So converting via Imagick, no issue, totally approved. With regard to Imagick failing: |
|
@ildyria I'll explore the possible solutions for libhief. libheif version greater than 1.15 is not able to get installed on Debian versions <= 12 or Ubuntu <= 25.04 out of the box and as from your comment the possible solution will be to update libheif to >= 1.18.2 I'll update the PR once I have a resolution regarding this and update the docs if needed Regards, |
Yeah, but Debian versions <= 12 are no longer considered. 😃 https://tracker.debian.org/pkg/libheif Ubuntu is not in the scope of Lychee as our base images are on alpine or debian stable. What I would do instead is to see if we can have a check in diagnostics for the version of libheif, and display a warning if it is too old. That way we do not have to deal with the fallback package. 👍 |
3 participants
This new feature adds a additional pre-processing step in app/Actions/Photo/Create.php ** $pre_pipes** to detect HEIF file types and convert them to jpeg in order to support working with these file types.
Also during our work we found that the photos.title is varchar(100) but our test images from IPhone 15+ had longer titles, so we had to add a migration to change the title to text column in order to support longer titles.
Ability to add more convertible file types in app/Actions/Photo/Convert/ImageTypeFactory.php
Added fallback image conversion via Go https://github.com/MaestroError/php-heic-to-jpg#php-heic-to-jpg because ImageMagick fails to convert some HEIF images, mostly from IPhone 15+ devices
Key points:
• Pre-processing happens early in the upload/import lifecycle, before AssertSupportedMedia.
• Conversion uses existing ImageMagick support when available (no new hard dependency).
• Converted file transparently replaces the original HEIC/HEIF file for downstream processing.
• Original file remains untouched if conversion fails.
• Fully compatible with S3 / remote storage setups, as conversion happens while the file is still local/temporary.
• Designed as an extensible pre-processing stage, not a HEIC-only hack.
Summary by CodeRabbit
New Features
Database Changes
Dependencies
Error Handling
Tests
✏️ Tip: You can customize this high-level summary in your review settings.