Feature: Image metadata strip - #59
Conversation
- added applicator `StripMeta` - applicators now yielding values instead of returning an image or null - cached image is now stored without exif data if the image has been modified by any other applicator
📝 WalkthroughWalkthroughRefactors applicators to return keyed iterables instead of single Images, introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ModifierFacade
participant Applicators
participant StripMeta
participant ModifyResult
Client->>ModifierFacade: modifyImage(image, pathInfo, modifiers, stripMeta?)
ModifierFacade->>Applicators: for each applicator.apply(image,...)
note right of Applicators `#eef3ff`: applicators yield keyed outputs\n('image','format','quality')
Applicators-->>ModifierFacade: iterable { 'image' => Image? , 'format' => string?, 'quality' => int? }
alt applicator yields image
ModifierFacade->>ModifierFacade: update image, mark modified
end
alt applicator yields format/quality
ModifierFacade->>ModifierFacade: set encodeFormat / encodeQuality
end
opt stripMeta enabled
ModifierFacade->>StripMeta: StripMeta.apply(image,...)
StripMeta-->>ModifierFacade: iterable (empty or with outputs)
end
ModifierFacade->>ModifyResult: Construct(image, modified, encodeFormat, encodeQuality)
ModifyResult-->>Client: return ModifyResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/Bridge/Nette/DI/ImageStorageExtension.php (1)
145-152: Add runtime validation to enforce StripMeta execution order.The "must be last" ordering for StripMeta is not enforced. Users can add custom applicators via configuration (e.g.,
applicators: ['@default', CustomApplicator::class]) that will execute after StripMeta, bypassing the intended execution sequence. ThenormalizeListOfStatementsWithDefaultsmethod appends user-provided applicators after the defaults, and applicators are executed in strict array order without any reordering logic. Consider adding validation to either:
- Prevent StripMeta from being positioned after other applicators
- Document that StripMeta execution order cannot be guaranteed when custom applicators are added
- Reorder applicators at runtime to ensure StripMeta always executes last
tests/Modifier/Applicator/FormatTest.phpt (3)
26-40: Update test name to reflect empty array return instead of null.The test name claims "null should be returned," but line 39 now asserts an empty array. This mismatch can confuse maintainers.
📝 Proposed fix
- public function testNullShouldBeReturnedIfQualityNotSpecifiedAndPathInfoHasSameExtensionAsImage(): void + public function testEmptyResultShouldBeReturnedIfQualityNotSpecifiedAndPathInfoHasSameExtensionAsImage(): void
42-56: Update test name to reflect empty array return instead of null.The test name claims "null should be returned," but line 55 now asserts an empty array. This mismatch can confuse maintainers.
📝 Proposed fix
- public function testNullShouldBeReturnedIfQualityIsNotSpecifiedAndPathInfoExtensionIsNull(): void + public function testEmptyResultShouldBeReturnedIfQualityIsNotSpecifiedAndPathInfoExtensionIsNull(): void
218-228: Remove unused helper method.The
createConfigForEncode()method is no longer called after the refactoring to use genericConfigInterfacemocks. This is dead code that should be removed.🧹 Proposed fix
- private function createConfigForEncode(): ConfigInterface - { - $config = Mockery::mock(ConfigInterface::class); - - $config->shouldReceive('offsetGet') - ->once() - ->with(Config::ENCODE_QUALITY) - ->andReturn(90); - - return $config; - } -
🤖 Fix all issues with AI agents
In @tests/Modifier/Applicator/StripMetaTest.phpt:
- Line 36: The StripMeta::apply() method returns an empty array using return []
instead of an empty generator, which causes the test to fail since the pipeline
expects an iterable/generator. Fix this by replacing the return [] statements in
the StripMeta::apply() method with bare return; statements (as done in similar
no-op cases in the Orientation and Format applicators), which will properly
yield an empty generator instead of an array.
🧹 Nitpick comments (7)
tests/Modifier/Applicator/ResizeTest.phpt (4)
121-123: Verify safe array key access in test assertions.The test accesses
$result['image']directly. While this should work correctly when the applicator yields an image, consider whether the test should explicitly verify the key exists or use a safer access pattern for better error messages if the applicator behavior changes.Consider adding explicit key existence check
$result = iterator_to_array($applicator->apply($image, $pathInfo, $modifierValues, $config)); +Assert::hasKey('image', $result); Assert::same($image, $result['image']);This would provide a clearer error message if the applicator doesn't yield the expected
imagekey.
149-151: Same array access pattern as previous test.Consider adding explicit key checks as suggested in the earlier comment for consistent error handling.
205-207: Same array access pattern as previous tests.Consider adding explicit key checks for consistency.
233-235: Same array access pattern as previous tests.Consider adding explicit key checks for consistency.
src/Modifier/Applicator/StripMeta.php (2)
19-40: Consider error handling for Imagick operations.The implementation correctly preserves ICC profiles while stripping metadata, but lacks error handling for potentially failing Imagick operations:
getImageProfiles()could fail or return unexpected valuesstripImage()could throwImagickExceptionprofileImage()could throwImagickExceptionWhile the
@throws ImagickExceptiondoc is present, consider wrapping the critical operations in a try-catch to provide better error messages or graceful fallback behavior.Note: The static analysis warnings about unused parameters
$pathInfoand$configare false positives—these parameters are required by theModifierApplicatorInterface.🛡️ Suggested error handling enhancement
public function apply(Image $image, PathInfoInterface $pathInfo, ModifierValues $values, ConfigInterface $config): iterable { if (true !== $values->getOptional('__stripMeta', false)) { return []; } $core = $image->getCore(); if (!($core instanceof Imagick)) { return []; } + try { $profiles = $core->getImageProfiles('icc'); $core->stripImage(); if (isset($profiles['icc'])) { $core->profileImage('icc', $profiles['icc']); } + } catch (ImagickException $e) { + // Log or handle the exception as needed + // For now, silently continue if stripping fails + } return []; }
21-39: Document the in-place mutation behavior.Unlike other applicators (e.g.,
Format,Orientation) that yieldOutImageto signal a transformation,StripMetamutates the image in-place and returns an empty array. While this works correctly, it's an unconventional pattern that could confuse future maintainers.Consider adding a comment explaining why this applicator doesn't yield outputs, or verify if this should instead yield
OutImage => $imageafter stripping to maintain consistency with other applicators.src/Modifier/Facade/ModifierFacade.php (1)
129-150: Add type validation for applicator outputs.The output handling loop processes values from applicators without validating their types. This could lead to runtime errors if an applicator yields invalid output types.
🔍 Proposed type validation
foreach ($this->applicators as $applicator) { foreach ($applicator->apply($image, $info, $values, $this->config) as $key => $value) { - if (ModifierApplicatorInterface::OutImage === $key && $value instanceof Image) { + if (ModifierApplicatorInterface::OutImage === $key) { + if (!$value instanceof Image) { + throw new InvalidArgumentException(sprintf( + 'Applicator %s yielded invalid OutImage value, expected Image instance.', + get_class($applicator), + )); + } $image = $value; $modified = true; continue; } if (ModifierApplicatorInterface::OutFormat === $key) { + if (!is_string($value)) { + throw new InvalidArgumentException(sprintf( + 'Applicator %s yielded invalid OutFormat value, expected string.', + get_class($applicator), + )); + } $encodeFormat = $value; $modified = true; continue; } if (ModifierApplicatorInterface::OutQuality === $key) { + if (!is_int($value)) { + throw new InvalidArgumentException(sprintf( + 'Applicator %s yielded invalid OutQuality value, expected int.', + get_class($applicator), + )); + } $encodeQuality = $value; $modified = true; } } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
src/Bridge/Nette/DI/ImageStorageExtension.phpsrc/Modifier/Applicator/Format.phpsrc/Modifier/Applicator/ModifierApplicatorInterface.phpsrc/Modifier/Applicator/Orientation.phpsrc/Modifier/Applicator/Resize.phpsrc/Modifier/Applicator/StripMeta.phpsrc/Modifier/Collection/ModifierValues.phpsrc/Modifier/Facade/ModifierFacade.phpsrc/Modifier/Facade/ModifierFacadeInterface.phpsrc/Modifier/Facade/ModifyResult.phpsrc/Persistence/ImagePersister.phpsrc/Resource/ImageResource.phpsrc/Resource/ResourceInterface.phptests/Bridge/Nette/DI/ImageStorageExtensionTest.phptests/Fixtures/TestApplicator.phptests/Modifier/Applicator/FormatTest.phpttests/Modifier/Applicator/OrientationTest.phpttests/Modifier/Applicator/ResizeTest.phpttests/Modifier/Applicator/StripMetaTest.phpttests/Modifier/Facade/ModifierFacadeTest.phpttests/Persistence/ImagePersisterTest.phpttests/Resource/ImageResourceTest.phpttests/Resource/TmpFileImageResourceTest.phpt
🧰 Additional context used
🧬 Code graph analysis (12)
src/Resource/ResourceInterface.php (3)
src/Modifier/Facade/ModifierFacade.php (1)
modifyImage(105-158)src/Modifier/Facade/ModifierFacadeInterface.php (1)
modifyImage(44-44)src/Resource/ImageResource.php (3)
modifyImage(54-73)getEncodeQuality(87-90)getEncodeFormat(92-95)
src/Modifier/Facade/ModifierFacadeInterface.php (3)
src/Modifier/Facade/ModifierFacade.php (1)
modifyImage(105-158)src/Resource/ImageResource.php (1)
modifyImage(54-73)src/Resource/ResourceInterface.php (1)
modifyImage(21-21)
src/Modifier/Applicator/Resize.php (4)
src/Modifier/Applicator/Format.php (1)
apply(19-51)src/Modifier/Applicator/ModifierApplicatorInterface.php (1)
apply(28-28)src/Modifier/Applicator/Orientation.php (1)
apply(17-36)src/Modifier/Applicator/StripMeta.php (1)
apply(19-40)
src/Modifier/Collection/ModifierValues.php (4)
src/Modifier/Collection/ModifierCollectionInterface.php (1)
add(19-19)src/Modifier/Collection/ModifierCollection.php (1)
add(21-37)src/Modifier/Preset/PresetCollection.php (1)
add(14-17)src/Modifier/Preset/PresetCollectionInterface.php (1)
add(12-12)
src/Modifier/Applicator/ModifierApplicatorInterface.php (4)
src/Modifier/Applicator/Format.php (1)
apply(19-51)src/Modifier/Applicator/Orientation.php (1)
apply(17-36)src/Modifier/Applicator/StripMeta.php (1)
apply(19-40)src/Modifier/Collection/ModifierValues.php (1)
ModifierValues(10-51)
src/Resource/ImageResource.php (3)
src/Modifier/Facade/ModifierFacade.php (1)
modifyImage(105-158)src/Modifier/Facade/ModifierFacadeInterface.php (1)
modifyImage(44-44)src/Resource/ResourceInterface.php (3)
modifyImage(21-21)getEncodeQuality(23-23)getEncodeFormat(25-25)
src/Modifier/Facade/ModifierFacade.php (5)
src/Modifier/Facade/ModifierFacadeInterface.php (1)
modifyImage(44-44)src/Resource/ImageResource.php (1)
modifyImage(54-73)src/Modifier/Facade/ModifyResult.php (1)
ModifyResult(9-17)src/Modifier/Applicator/ModifierApplicatorInterface.php (1)
apply(28-28)src/Modifier/Applicator/StripMeta.php (1)
apply(19-40)
src/Modifier/Applicator/StripMeta.php (3)
src/Modifier/Collection/ModifierValues.php (2)
ModifierValues(10-51)getOptional(42-45)src/Modifier/Applicator/ModifierApplicatorInterface.php (1)
apply(28-28)src/Modifier/Applicator/Orientation.php (1)
apply(17-36)
tests/Bridge/Nette/DI/ImageStorageExtensionTest.php (1)
src/Modifier/Applicator/StripMeta.php (1)
StripMeta(14-41)
src/Bridge/Nette/DI/ImageStorageExtension.php (2)
src/Modifier/Applicator/Format.php (1)
Format(17-69)src/Modifier/Applicator/StripMeta.php (1)
StripMeta(14-41)
src/Modifier/Applicator/Orientation.php (2)
src/Modifier/Applicator/ModifierApplicatorInterface.php (1)
apply(28-28)src/Modifier/Collection/ModifierValues.php (2)
ModifierValues(10-51)getOptional(42-45)
src/Persistence/ImagePersister.php (6)
src/Resource/ImageResource.php (3)
modifyImage(54-73)getEncodeQuality(87-90)getEncodeFormat(92-95)src/Resource/ResourceInterface.php (3)
modifyImage(21-21)getEncodeQuality(23-23)getEncodeFormat(25-25)src/PathInfoInterface.php (1)
getModifiers(14-14)src/PathInfo.php (1)
getModifiers(56-59)src/Config/Config.php (1)
Config(9-37)src/Bridge/Intervention/Image/DriverProxy.php (1)
encode(47-50)
🪛 GitHub Actions: Tests
tests/Modifier/Applicator/StripMetaTest.phpt
[error] 36-36: TypeError: iterator_to_array(): Argument #1 ($iterator) must be Traversable, array given
🪛 PHPMD (2.15.0)
src/Modifier/Applicator/Resize.php
30-30: Avoid unused parameters such as '$pathInfo'. (undefined)
(UnusedFormalParameter)
30-30: Avoid unused parameters such as '$config'. (undefined)
(UnusedFormalParameter)
src/Modifier/Applicator/StripMeta.php
19-19: Avoid unused parameters such as '$pathInfo'. (undefined)
(UnusedFormalParameter)
19-19: Avoid unused parameters such as '$config'. (undefined)
(UnusedFormalParameter)
tests/Fixtures/TestApplicator.php
15-15: Avoid unused parameters such as '$image'. (undefined)
(UnusedFormalParameter)
15-15: Avoid unused parameters such as '$pathInfo'. (undefined)
(UnusedFormalParameter)
15-15: Avoid unused parameters such as '$values'. (undefined)
(UnusedFormalParameter)
15-15: Avoid unused parameters such as '$config'. (undefined)
(UnusedFormalParameter)
src/Modifier/Applicator/Format.php
19-19: Avoid unused parameters such as '$config'. (undefined)
(UnusedFormalParameter)
src/Modifier/Applicator/Orientation.php
17-17: Avoid unused parameters such as '$pathInfo'. (undefined)
(UnusedFormalParameter)
17-17: Avoid unused parameters such as '$config'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (40)
src/Resource/ResourceInterface.php (1)
21-25: LGTM! Clean interface extension for metadata stripping and encoding control.The new
stripMetaparameter with a default value maintains backward compatibility, and the encoding getter methods are appropriately nullable to support optional encoding directives.tests/Bridge/Nette/DI/ImageStorageExtensionTest.php (2)
145-150: LGTM! Test expectations updated for the new StripMeta applicator.The test correctly expects
StripMetaas the last applicator in the default sequence.
249-255: LGTM! Custom configuration test updated correctly.The test validates that StripMeta is included even when custom applicators are configured with
@default.tests/Modifier/Applicator/ResizeTest.phpt (3)
39-42: LGTM! Test correctly handles iterable output from applicator.The test now wraps the applicator call with
iterator_to_array()to handle the new iterable return type.
54-58: LGTM! Exception test adapted for iterable output.Correctly wraps the call to ensure the iterator is consumed and any exceptions are thrown.
64-84: LGTM! Empty result assertion updated correctly.The test correctly expects an empty array
[]when no modifications are needed, which aligns with the iterable output pattern.tests/Fixtures/TestApplicator.php (1)
15-18: LGTM! Test fixture correctly updated for iterable output.The return type change to
iterablealigns with the interface update. Returning an empty array is appropriate for a no-op test fixture. The static analysis warnings about unused parameters are expected and acceptable for test fixtures implementing required interface methods.src/Modifier/Collection/ModifierValues.php (1)
47-50: LGTM! Visibility change aligns with collection patterns.Making
add()public allows external callers to dynamically add values after construction (e.g., the__stripMetaflag in the modifier pipeline), while maintaining the existing internal usage in the constructor. This change is consistent with other collection classes in the codebase that expose publicadd()methods.src/Modifier/Facade/ModifierFacadeInterface.php (1)
44-44: LGTM! Backward-compatible API extension for metadata stripping.The optional
stripMetaparameter with a default value offalseextends the interface in a backward-compatible manner, enabling the new metadata stripping capability without breaking existing callers.src/Modifier/Facade/ModifyResult.php (1)
14-15: LGTM! Clean extension to carry encoding directives.The addition of nullable
encodeFormatandencodeQualityproperties extendsModifyResultto carry encoding directives from applicators (Format, Orientation, Resize, StripMeta) to consumers (ModifierFacade, ImageResource). The readonly modifier ensures immutability, and nullable types allow gradual adoption across the pipeline.tests/Modifier/Applicator/OrientationTest.phpt (4)
35-35: LGTM! Correctly tests empty iterable result.The test now properly expects an empty array from
iterator_to_array()when no orientation modification is needed, correctly reflecting the new iterable-based applicator pattern.
61-61: LGTM! Correctly tests empty iterable result for normal orientation.The test properly verifies that no output is produced when the image already has normal EXIF orientation, consistent with the iterable pattern.
92-93: LGTM! Correctly tests structured iterable output.The test properly collects the iterable output and accesses the modified image via the
'image'key, demonstrating correct usage of the new structured output pattern where applicators yield key-value pairs.
116-117: LGTM! Correctly tests rotation with structured output.The test properly validates image rotation by collecting the iterable and accessing the result by key, consistent with the architectural refactor.
tests/Resource/ImageResourceTest.phpt (2)
44-49: LGTM! ModifyResult construction updated for new signature.The test correctly instantiates
ModifyResultwith the newencodeFormatandencodeQualityparameters set tonull, which is appropriate for a test case that doesn't involve encoding modifications.
51-54: LGTM! Mock expectation updated for new interface signature.The mock correctly expects the fourth parameter (
falseforstripMeta) in themodifyImagecall, properly reflecting the extendedModifierFacadeInterfacesignature.src/Persistence/ImagePersister.php (2)
54-55: LGTM!The
stripMetaflag is correctly passed astruewhen modifiers are present, enabling metadata stripping for cached/modified images. This aligns with the updatedmodifyImagesignature.
140-143: LGTM!The encoding configuration now properly derives quality and format from the resource with sensible fallbacks. The fallback chain
resource -> config -> 90for quality is correct.tests/Modifier/Applicator/StripMetaTest.phpt (2)
62-96: LGTM!The test correctly validates that ICC profiles are preserved after stripping metadata. The mock expectations properly verify the sequence: get ICC profile → strip image → restore ICC profile.
98-128: LGTM!The test correctly validates the case when no ICC profile exists -
stripImage()is called butprofileImage()is not.src/Modifier/Applicator/Orientation.php (1)
17-36: LGTM!The generator-based implementation is correct. Using bare
return;statements properly produces an empty generator for no-op cases, andyield self::OutImage =>correctly emits the modified image. The unused$pathInfoand$configparameters are mandated byModifierApplicatorInterface, so the PHPMD warnings are false positives.src/Modifier/Applicator/Resize.php (1)
72-91: LGTM!The refactoring to use a
matchexpression withyieldis clean and readable. The earlyreturn;when dimensions are unchanged correctly produces an empty generator, and the fit-specific transformations are properly yielded viaOutImage.tests/Resource/TmpFileImageResourceTest.phpt (1)
49-59: LGTM!The test correctly reflects the updated
ModifyResultsignature withencodeFormatandencodeQualityfields, and themodifyImagemock expectation properly includes the newstripMetaparameter (false).src/Modifier/Applicator/Format.php (1)
19-51: LGTM!The generator-based implementation correctly yields
OutImage,OutFormat, and conditionallyOutQuality. The JPEG preprocessing (white background, interlacing for pjpg) is properly preserved, and the earlyreturn;correctly produces an empty generator when encoding isn't needed.tests/Modifier/Facade/ModifierFacadeTest.phpt (4)
217-236: LGTM!The mock correctly returns an array with
OutImagekey, and theModifyResultassertion properly includes the newencodeFormatandencodeQualityfields.
259-278: LGTM!The test correctly validates the no-modification scenario where the applicator returns an empty array, resulting in
modified: false.
312-331: LGTM!Preset modifier scenario correctly updated with the new output contract.
365-384: LGTM!Preset no-modification scenario properly validates that an empty applicator result yields
modified: false.tests/Persistence/ImagePersisterTest.phpt (3)
228-236: LGTM: Encoding property expectations are correct.The new expectations for
getEncodeQuality()andgetEncodeFormat()correctly verify that resources without explicit encoding directives return null, which aligns with the default behavior in the updated architecture.Also applies to: 336-344, 406-414, 482-490, 534-542, 600-608
245-248: Implementation correctly uses the offsetExists/offsetGet pattern with the null coalescing operator.The tests expect
offsetExists(Config::ENCODE_QUALITY)followed byoffsetGet(). The implementation at line 140 uses$this->config[Config::ENCODE_QUALITY] ?? 90, which naturally triggers this exact behavior when the config object implements ArrayAccess. PHP's null coalescing operator automatically callsoffsetExists()first, thenoffsetGet()if the key exists.
997-1015: Empty string format parameter is correct—encodes to original format.The
encode()method in tg666/image (Intervention Image fork) treats an empty string as a directive to preserve the image's original format, with JPEG as fallback if the original type is unknown. The test expectations are correct.src/Modifier/Applicator/ModifierApplicatorInterface.php (1)
14-28: LGTM: Well-designed interface evolution.The interface evolution from single
?Imagereturn to structurediterableoutput is well-executed:
- Constants provide type-safe keys for outputs
- Documentation clearly specifies allowed outputs and their types
- The "return nothing if nothing changed" convention is explicit
- Supports extensibility for future output types
This breaking change enables the multi-output architecture required for metadata stripping and encoding control.
src/Modifier/Facade/ModifierFacade.php (1)
105-119: LGTM: stripMeta parameter correctly integrated.The new
$stripMetaparameter is correctly:
- Added to the public API signature
- Propagated to modifier values as
__stripMeta- Made available to all applicators via the
ModifierValuesobjectThis enables the
StripMetaapplicator to conditionally execute based on the flag.src/Resource/ImageResource.php (2)
15-17: LGTM: Encoding properties correctly integrated.The new encoding properties are well-implemented:
- Properly initialized as nullable to represent "not set"
- Conditionally populated from
ModifyResult- Exposed via clean getter methods
- Follow the same pattern as existing resource properties
Also applies to: 87-95
54-73: LGTM: Modification logic correctly preserves state.The conditional assignment pattern correctly handles state preservation:
$resource->modifiedis only set if$modifyResult->modifiedis true, preventing false from overwriting a previous true state- Encoding properties are only set if not null, preserving any existing values
- The
$stripMetaparameter is correctly propagated to the facadeThis ensures that multiple chained modifications accumulate their effects properly.
tests/Modifier/Applicator/FormatTest.phpt (5)
58-76: LGTM!The test correctly validates that an unsupported MIME type triggers encoding to the default JPG format, and the assertions properly verify the structured result keys.
78-94: LGTM!The test correctly validates format conversion when the path extension differs from the image MIME type, and properly asserts on the structured result.
96-113: LGTM!The test correctly validates that specifying a quality value triggers encoding and includes the quality in the structured result.
115-160: LGTM!Both tests correctly validate JPEG encoding scenarios (standard and progressive), with proper mock setup and assertions on the structured results.
162-186: LGTM!The test correctly validates that an image already in progressive JPEG format (with INTERLACE_JPEG scheme) doesn't trigger re-encoding.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/Modifier/Applicator/StripMetaTest.phpt (2)
23-133: Consider refactoring to reduce test duplication.All four test methods share a nearly identical mock setup pattern (creating Image, PathInfoInterface, ModifierValues, and ConfigInterface mocks). Consider extracting common setup into private helper methods or using a
setUp()method to reduce duplication and improve maintainability.♻️ Example refactoring approach
private function createBaseMocks(): array { return [ 'image' => Mockery::mock(Image::class), 'pathInfo' => Mockery::mock(PathInfoInterface::class), 'modifierValues' => Mockery::mock(ModifierValues::class), 'config' => Mockery::mock(ConfigInterface::class), ]; }Then in each test:
$mocks = $this->createBaseMocks(); $mocks['modifierValues']->shouldReceive('getOptional') ->once() ->with('__stripMeta', false) ->andReturn(false); // ... rest of test
38-38: Test names claim "generator" but assertions accept arrays.All four test methods are named with "EmptyGeneratorShouldBeReturned" or similar, but the assertions use
$result instanceof Traversable ? iterator_to_array($result) : $result, which accepts both generators/iterables and plain arrays. This creates an inconsistency between the test name and what's actually verified.Given the past review comment indicating that the implementation should return a generator (not an array), consider adding explicit type assertions to enforce the expected return type:
Assert::type(Traversable::class, $result); Assert::same([], iterator_to_array($result));This would make the tests fail if the implementation incorrectly returns an array, helping catch the issue mentioned in the previous review.
Also applies to: 62-62, 99-99, 132-132
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/Modifier/Applicator/StripMetaTest.phpt
🔇 Additional comments (3)
tests/Modifier/Applicator/StripMetaTest.phpt (3)
1-22: LGTM: Test class structure and imports.The test class is properly structured with all necessary imports and extends the correct base class.
135-138: LGTM: Proper Mockery cleanup.The tearDown method correctly closes Mockery to prevent mock leakage between tests.
141-141: LGTM: Test execution.Standard Nette Tester pattern for executing the test case.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.