Resource Image Encoding - #63
Merged
Merged
Conversation
- added method `ResourceInterface::getEncodedImage()` - method `ImagePersister::save()` now uses the `getEncodedImage()` from a resource during saving - fixed unit tests
📝 WalkthroughWalkthroughEncoding responsibility is moved from ChangesEncoding responsibility shift
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Resource/ImageResource.php (1)
62-73: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEncode-only modifier changes can be skipped.
If
modifyImage()updatesencodeFormat/encodeQualitybutModifyResult::modifiedisfalse,getEncodedImage()still returns local file bytes via the unmodified shortcut. That bypasses requested re-encoding and can persist incorrect output.💡 Proposed fix
public function modifyImage(string|array $modifiers, bool $stripMeta = false): self { $resource = clone $this; $modifyResult = $this->modifierFacade->modifyImage($this->image, $this->pathInfo, $modifiers, $stripMeta); $resource->image = $modifyResult->image; + $needsReencode = $modifyResult->modified; - if ($modifyResult->modified) { - $resource->modified = $modifyResult->modified; - $resource->encodedImage = null; - } - if (null !== $modifyResult->encodeFormat) { $resource->encodeFormat = $modifyResult->encodeFormat; + $needsReencode = true; } if (null !== $modifyResult->encodeQuality) { $resource->encodeQuality = $modifyResult->encodeQuality; + $needsReencode = true; + } + + if ($needsReencode) { + $resource->modified = true; + $resource->encodedImage = null; } return $resource; }Also applies to: 106-112
🤖 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/Resource/ImageResource.php` around lines 62 - 73, The current code in ImageResource.php updates encodeFormat and encodeQuality properties independently from the modified flag check. If only the encode settings change but the image itself is not modified (ModifyResult::modified is false), getEncodedImage() will bypass re-encoding and return the original file bytes. To fix this, ensure that when encodeFormat or encodeQuality are updated (in the conditional blocks checking null !== $modifyResult->encodeFormat and null !== $modifyResult->encodeQuality), you also set the modified flag to true so that getEncodedImage() will properly re-encode the image with the new settings. This ensures encode-only modifications trigger necessary re-encoding instead of being skipped.
🧹 Nitpick comments (2)
tests/Resource/ImageResourceTest.phpt (1)
83-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winProve cache behavior by mutating the temp file between reads.
These assertions can still pass if
getEncodedImage()rereads disk each time, because the file bytes never change. Mutating the file after the first read makes the cache check deterministic.Proposed test tweak
- Assert::same('... original bytes ...', $resource->getEncodedImage()); - # the result is cached - Assert::same('... original bytes ...', $resource->getEncodedImage()); + $first = $resource->getEncodedImage(); + file_put_contents($localFile, '... mutated bytes ...'); + # second read should still return cached bytes + Assert::same('... original bytes ...', $first); + Assert::same('... original bytes ...', $resource->getEncodedImage());🤖 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/Resource/ImageResourceTest.phpt` around lines 83 - 85, The current cache test in ImageResourceTest.phpt does not actually prove caching behavior because the file contents remain the same between both getEncodedImage() calls. To make the cache check deterministic, modify the temp file on disk after the first getEncodedImage() call but before the second call. This way, if caching is not working, the second call would return different content from the file, and the assertion would fail. If caching is working correctly, both assertions will still pass because the second call returns the cached value from the first read, not the newly mutated file contents.tests/Resource/ResourceFactoryTest.phpt (1)
328-339: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd one test for non-default
encode_qualitypropagation.
createConfig()only validates the default fallback path. Add a case withConfig::ENCODE_QUALITYpresent and assert the created resource carries that configured quality.Suggested direction
+ public function testConfiguredEncodeQualityShouldBePropagatedToResource(): void + { + $imageManager = Mockery::mock(ImageManager::class); + $modifierFacade = Mockery::mock(ModifierFacadeInterface::class); + $pathInfo = Mockery::mock(FilePathInfoInterface::class); + $image = Mockery::mock(Image::class); + + $imageManager->shouldReceive('make') + ->once() + ->with('filename') + ->andReturn($image); + + $resourceFactory = new ResourceFactory( + $this->createFilesystem(), + $imageManager, + $modifierFacade, + $this->createConfig(72), + ); + + $resource = $resourceFactory->createResourceFromFile($pathInfo, 'filename'); + Assert::same(72, $resource->getEncodeQuality()); + } + - private function createConfig(): ConfigInterface + private function createConfig(?int $encodeQuality = null): ConfigInterface { $config = Mockery::mock(ConfigInterface::class); - - # the resource reads the encode quality from the config; falling back to the default is enough here - $config->shouldReceive('offsetExists') - ->with(Config::ENCODE_QUALITY) - ->andReturn(false); + $config->shouldReceive('offsetExists') + ->with(Config::ENCODE_QUALITY) + ->andReturn(null !== $encodeQuality); + if (null !== $encodeQuality) { + $config->shouldReceive('offsetGet') + ->with(Config::ENCODE_QUALITY) + ->andReturn($encodeQuality); + } return $config; }🤖 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/Resource/ResourceFactoryTest.phpt` around lines 328 - 339, The createConfig() method only tests the default fallback path where ENCODE_QUALITY is not present in the config. Add a new test case that verifies the non-default path where ENCODE_QUALITY is configured. This test should mock the offsetExists method to return true for Config::ENCODE_QUALITY, mock offsetGet to return a specific non-default quality value, create a resource with this config, and then assert that the created resource carries the configured quality value instead of using the default.
🤖 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.
Outside diff comments:
In `@src/Resource/ImageResource.php`:
- Around line 62-73: The current code in ImageResource.php updates encodeFormat
and encodeQuality properties independently from the modified flag check. If only
the encode settings change but the image itself is not modified
(ModifyResult::modified is false), getEncodedImage() will bypass re-encoding and
return the original file bytes. To fix this, ensure that when encodeFormat or
encodeQuality are updated (in the conditional blocks checking null !==
$modifyResult->encodeFormat and null !== $modifyResult->encodeQuality), you also
set the modified flag to true so that getEncodedImage() will properly re-encode
the image with the new settings. This ensures encode-only modifications trigger
necessary re-encoding instead of being skipped.
---
Nitpick comments:
In `@tests/Resource/ImageResourceTest.phpt`:
- Around line 83-85: The current cache test in ImageResourceTest.phpt does not
actually prove caching behavior because the file contents remain the same
between both getEncodedImage() calls. To make the cache check deterministic,
modify the temp file on disk after the first getEncodedImage() call but before
the second call. This way, if caching is not working, the second call would
return different content from the file, and the assertion would fail. If caching
is working correctly, both assertions will still pass because the second call
returns the cached value from the first read, not the newly mutated file
contents.
In `@tests/Resource/ResourceFactoryTest.phpt`:
- Around line 328-339: The createConfig() method only tests the default fallback
path where ENCODE_QUALITY is not present in the config. Add a new test case that
verifies the non-default path where ENCODE_QUALITY is configured. This test
should mock the offsetExists method to return true for Config::ENCODE_QUALITY,
mock offsetGet to return a specific non-default quality value, create a resource
with this config, and then assert that the created resource carries the
configured quality value instead of using the default.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 108f1881-3bfe-42fc-8270-beb2f347b4f6
📒 Files selected for processing (10)
src/Bridge/Nette/DI/ImageStorageExtension.phpsrc/Persistence/ImagePersister.phpsrc/Resource/ImageResource.phpsrc/Resource/ResourceFactory.phpsrc/Resource/ResourceInterface.phpsrc/Resource/TmpFileImageResource.phptests/Persistence/ImagePersisterTest.phpttests/Resource/ImageResourceTest.phpttests/Resource/ResourceFactoryTest.phpttests/Resource/TmpFileImageResourceTest.phpt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit