Open
Conversation
- Created Image_lib library to handle EXIF stripping for JPEG, PNG, GIF, and WebP images - Uses GD library to re-encode images without EXIF data - Added EXIF stripping to both company logo upload (Config controller) and item image upload (Items controller) - Handles privacy concern by removing geolocation and device info from uploaded images Fixes #4010
- Renamed strip_exif() to stripEXIF() for PSR-12 compliance - Added configuration options for EXIF stripping (exif_stripping_enabled, exif_fields_to_keep) - Migration to add new config keys with sensible defaults - Updated Config and Items controllers to check config before stripping EXIF - Made EXIF stripping optional via settings, defaulting to disabled for backward compatibility - Allows selective field preservation (Copyright, Orientation by default)
- Add fileeye/pel dependency to composer.json for selective EXIF field removal - Rewrite Image_lib::stripExifJpeg() to use FileEye/pel for precise field manipulation - Add exif_to_pel_tags mapping for supported EXIF fields - Implement removeExifFields() to selectively remove EXIF data based on config - Keep fallback method if library is unavailable or parsing fails - Add language strings for new configuration options - Update migration to include Software in default fields to keep This addresses reviewer concern about preserving copyright and other beneficial metadata while removing privacy-sensitive fields like GPS location.
jekkos
changed the title
objecttothis
previously approved these changes
Mar 8, 2026
objecttothis
previously approved these changes
Mar 9, 2026
jekkos
pushed a commit
that referenced
this pull request
Mar 10, 2026
- Add checkbox to enable/disable EXIF stripping - Add multiselect dropdown for EXIF fields to keep (Make, Model, Orientation, Copyright, Software, DateTime, GPS) - Place UI in General Settings after Image Restrictions section - Add JavaScript to enable/disable multiselect based on checkbox state - Follows existing UI patterns (similar to image_allowed_types multiselect) - Integrates with existing backend migration and Image_lib implementation Closes #4405 - EXIF stripping improvements
objecttothis
previously approved these changes
Mar 11, 2026
jekkos
pushed a commit
that referenced
this pull request
Mar 12, 2026
- Add checkbox to enable/disable EXIF stripping - Add multiselect dropdown for EXIF fields to keep (Make, Model, Orientation, Copyright, Software, DateTime, GPS) - Place UI in General Settings after Image Restrictions section - Add JavaScript to enable/disable multiselect based on checkbox state - Follows existing UI patterns (similar to image_allowed_types multiselect) - Integrates with existing backend migration and Image_lib implementation Closes #4405 - EXIF stripping improvements
📝 WalkthroughWalkthroughAdds a new Image_lib for EXIF stripping, integrates it into company logo and item image upload flows, adds a migration and config/UI to control preserved EXIF fields, and introduces the fileeye/pel composer dependency. Changes
Sequence DiagramsequenceDiagram
actor User
participant Controller as Config/Items Controller
participant FS as FileSystem
participant DB as AppConfig Storage
participant ImageLib as Image_lib
User->>Controller: POST upload image
Controller->>FS: Move uploaded file to uploads/*
Controller->>DB: Read `exif_fields_to_keep`
alt exif_fields_to_keep set
Controller->>ImageLib: stripEXIF(filepath, fields_to_keep)
ImageLib->>FS: Read file & detect MIME
alt JPEG
ImageLib->>ImageLib: Remove EXIF except fields_to_keep
else PNG/GIF/WEBP
ImageLib->>FS: Re-encode image without metadata
end
ImageLib->>FS: Write cleaned file
ImageLib-->>Controller: Return success/failure
end
Controller->>DB: Save image metadata/record
Controller-->>User: Return upload response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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: 5
🧹 Nitpick comments (2)
app/Views/configs/general_config.php (1)
507-516:arguments.calleeis deprecated but matches existing pattern.The use of
arguments.calleeat line 513 is deprecated in ES5 strict mode. However, this follows the existing pattern used forenable_disable_gcaptcha_enableat line 528. Consider refactoring both in a future PR.💡 Modern alternative (for future consideration)
function enable_disable_exif_stripping() { var exif_enabled = $("#exif_stripping_enabled").is(":checked"); $("#exif_fields_to_keep").prop("disabled", !exif_enabled); if (!exif_enabled) { $("#exif_fields_to_keep").selectpicker('refresh'); } } enable_disable_exif_stripping(); $("#exif_stripping_enabled").change(enable_disable_exif_stripping);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/configs/general_config.php` around lines 507 - 516, Replace the IIFE that uses the deprecated arguments.callee with a named function: create a function named enable_disable_exif_stripping that contains the body currently in the IIFE (reading $("#exif_stripping_enabled"), toggling $("#exif_fields_to_keep").prop("disabled"), and calling selectpicker('refresh') when disabled), call it once immediately, and keep the existing $("#exif_stripping_enabled").change(enable_disable_exif_stripping) binding; also apply the same refactor pattern to the similar enable_disable_gcaptcha_enable usage to avoid repeated deprecated usage.app/Controllers/Items.php (1)
795-801: Consider logging EXIF stripping failures.The
stripEXIF()return value is ignored. While silently continuing on failure is reasonable (upload shouldn't fail due to EXIF stripping issues), logging the failure would aid debugging.💡 Suggested improvement
$exif_stripping_enabled = $this->appconfig->get_value('exif_stripping_enabled', '0'); if ($exif_stripping_enabled == '1') { $image_lib = new Image_lib(); $exif_fields_to_keep = array_filter(explode(',', $this->appconfig->get_value('exif_fields_to_keep', 'Copyright,Orientation'))); - $image_lib->stripEXIF(FCPATH . 'uploads/item_pics/' . $file_info['raw_name'] . '.' . $file_info['file_ext'], $exif_fields_to_keep); + $filepath = FCPATH . 'uploads/item_pics/' . $file_info['raw_name'] . '.' . $file_info['file_ext']; + if (!$image_lib->stripEXIF($filepath, $exif_fields_to_keep)) { + log_message('warning', 'EXIF stripping failed for: ' . $filepath); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/Items.php` around lines 795 - 801, Capture and check the return value of Image_lib::stripEXIF (called in the Items controller) and, on failure, emit a log entry (e.g. $this->logger->warning/error) that includes the target file path (FCPATH . 'uploads/item_pics/' . $file_info['raw_name'] . '.' . $file_info['file_ext']), the $exif_fields_to_keep value, and any error message exposed by Image_lib so the upload continues but failures are recorded for debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Controllers/Config.php`:
- Around line 397-398: The code calls implode(',', $this->request->getPost(...))
for multi-select fields which returns null when nothing is selected and causes a
TypeError; update the handling for both 'exif_fields_to_keep' and
'image_allowed_types' so you coerce null to an empty array (e.g. use
(array)$this->request->getPost('exif_fields_to_keep') or
$this->request->getPost('exif_fields_to_keep') ?? []) before calling
implode(',', ...) so implode always receives an array.
In `@app/Database/Migrations/20260306120000_MigrationEXIFStrippingOptions.php`:
- Around line 20-28: The migration defines the default for 'exif_fields_to_keep'
as 'Copyright,Orientation,Software' which conflicts with the fallback defaults
used in the Config and Items controllers (they use 'Copyright,Orientation');
update the fallback/defaults to match: choose the canonical set and make all
three places consistent by changing either the migration value or the fallback
strings in the Config and Items controllers to the same comma-separated list;
verify the config key 'exif_fields_to_keep' and the toggle
'exif_stripping_enabled' are referenced consistently in Config and Items classes
after the change.
- Line 17: Remove the unused $forge variable instantiation in the
MigrationEXIFStrippingOptions migration: delete the line that assigns $forge =
Database::forge() since $forge is never referenced elsewhere in the class;
ensure no other code depends on Database::forge() being called and keep the rest
of the up/down migration logic intact.
In `@app/Libraries/Image_lib.php`:
- Around line 28-32: The allowed_types array in Image_lib.php lists 'image/bmp'
and 'image/tiff' but there are no handlers for them, causing the function to
incorrectly return true for unsupported formats; either remove 'image/bmp' and
'image/tiff' from the allowed_types array (so in_array rejects them) or
implement proper handlers (e.g., add handling branches/functions such as
handleBmp and handleTiff or extend the existing image processing branch in the
same function that currently handles jpeg/png/webp/gif) that perform the same
metadata-stripping/processing as other types and return true on success; also
ensure the final return value reflects actual processing status (return false
when no handler ran) and update/cover with tests.
- Around line 83-86: The code incorrectly calls PelTiff::setIfd(null,
PelTag::GPS_IFD_POINTER) which only replaces IFD0 and won't remove the GPS
sub-IFD; instead retrieve the current IFD0 via $tiff->getIfd(), create a new
PelIfd(PelIfd::IFD0), copy all entries from the original IFD0 except the
PelTag::GPS_INFO_IFD_POINTER entry, copy sub-IFDs skipping the PelIfd::GPS
sub-IFD, preserve the next IFD chain (thumbnail) with setNextIfd, then call
$tiff->setIfd($newIfd0) to replace IFD0 without the GPS data.
---
Nitpick comments:
In `@app/Controllers/Items.php`:
- Around line 795-801: Capture and check the return value of
Image_lib::stripEXIF (called in the Items controller) and, on failure, emit a
log entry (e.g. $this->logger->warning/error) that includes the target file path
(FCPATH . 'uploads/item_pics/' . $file_info['raw_name'] . '.' .
$file_info['file_ext']), the $exif_fields_to_keep value, and any error message
exposed by Image_lib so the upload continues but failures are recorded for
debugging.
In `@app/Views/configs/general_config.php`:
- Around line 507-516: Replace the IIFE that uses the deprecated
arguments.callee with a named function: create a function named
enable_disable_exif_stripping that contains the body currently in the IIFE
(reading $("#exif_stripping_enabled"), toggling
$("#exif_fields_to_keep").prop("disabled"), and calling selectpicker('refresh')
when disabled), call it once immediately, and keep the existing
$("#exif_stripping_enabled").change(enable_disable_exif_stripping) binding; also
apply the same refactor pattern to the similar enable_disable_gcaptcha_enable
usage to avoid repeated deprecated usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 248bc640-0432-4f0a-b693-de9da21498d1
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
app/Controllers/Config.phpapp/Controllers/Items.phpapp/Database/Migrations/20260306120000_MigrationEXIFStrippingOptions.phpapp/Language/en/Config.phpapp/Libraries/Image_lib.phpapp/Views/configs/general_config.phpcomposer.json
app/Database/Migrations/20260306120000_MigrationEXIFStrippingOptions.php
Outdated
Show resolved
Hide resolved
app/Database/Migrations/20260306120000_MigrationEXIFStrippingOptions.php
Show resolved
Hide resolved
app/Libraries/Image_lib.php
Outdated
Comment on lines
+83
to
+86
| $gpsIfd = $tiff->getIfd(PelTag::GPS_IFD_POINTER); | ||
| if ($gpsIfd !== null && !in_array('GPS', $fields_to_keep)) { | ||
| $tiff->setIfd(null, PelTag::GPS_IFD_POINTER); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
fileeye/pel PelTiff setIfd remove GPS IFD
💡 Result:
In PEL, the GPS IFD is stored as a sub-IFD of IFD0 ($ifd0->addSubIfd(...)), and when writing bytes PEL generates the pointer tags from its internal sub-IFD list. PelTiff::setIfd() only lets you replace the first IFD (and it must be PelIfd::IFD0). [1][2]
Because PelIfd has addSubIfd() / getSubIfds() but no public “removeSubIfd”, the usual way to “remove GPS IFD” is to rebuild IFD0 without the GPS sub-IFD, then setIfd():
use lsolesen\pel\PelJpeg;
use lsolesen\pel\PelIfd;
use lsolesen\pel\PelTag;
$jpeg = new PelJpeg($inFile);
$exif = $jpeg->getExif();
$tiff = $exif->getTiff();
$ifd0 = $tiff->getIfd();
// Rebuild IFD0
$newIfd0 = new PelIfd(PelIfd::IFD0);
// copy entries (optionally skip GPS pointer tag if present as an entry)
foreach ($ifd0->getEntries() as $tag => $entry) {
if ($tag == PelTag::GPS_INFO_IFD_POINTER) continue;
$newIfd0->addEntry($entry);
}
// copy sub-IFDs except GPS
foreach ($ifd0->getSubIfds() as $type => $subIfd) {
if ($type === PelIfd::GPS) continue;
$newIfd0->addSubIfd($subIfd);
}
// keep IFD1 chain (thumbnail etc.)
$newIfd0->setNextIfd($ifd0->getNextIfd());
// replace in TIFF
$tiff->setIfd($newIfd0);
file_put_contents($outFile, $jpeg->getBytes());References: PelIfd::addSubIfd() / getSubIfds() / getEntries() / setNextIfd() are documented on PelIfd, and PelTiff::setIfd() requires the new first IFD to be PelIfd::IFD0. [1][2]
Sources:
[1] https://pel.github.io/pel/doc/PEL/PelIfd.html
[2] https://pel.github.io/pel/doc/PEL/PelTiff.html
PelTiff::setIfd() is not the correct API for removing GPS IFD.
The code calls $tiff->setIfd(null, PelTag::GPS_IFD_POINTER), but in PEL, the GPS IFD is a sub-IFD of IFD0, not a standalone IFD. PelTiff::setIfd() only replaces the first IFD (IFD0) and doesn't support tag-based removal. GPS data will not be removed with this approach.
Correct pattern: rebuild IFD0 without the GPS sub-IFD, then call setIfd($newIfd0):
Example fix
use lsolesen\pel\PelIfd;
use lsolesen\pel\PelTag;
$ifd0 = $tiff->getIfd();
$newIfd0 = new PelIfd(PelIfd::IFD0);
// Copy entries, skip GPS pointer tag if present
foreach ($ifd0->getEntries() as $tag => $entry) {
if ($tag == PelTag::GPS_INFO_IFD_POINTER) continue;
$newIfd0->addEntry($entry);
}
// Copy sub-IFDs except GPS
foreach ($ifd0->getSubIfds() as $type => $subIfd) {
if ($type === PelIfd::GPS) continue;
$newIfd0->addSubIfd($subIfd);
}
// Preserve IFD1 chain (thumbnail, etc.)
$newIfd0->setNextIfd($ifd0->getNextIfd());
$tiff->setIfd($newIfd0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Libraries/Image_lib.php` around lines 83 - 86, The code incorrectly calls
PelTiff::setIfd(null, PelTag::GPS_IFD_POINTER) which only replaces IFD0 and
won't remove the GPS sub-IFD; instead retrieve the current IFD0 via
$tiff->getIfd(), create a new PelIfd(PelIfd::IFD0), copy all entries from the
original IFD0 except the PelTag::GPS_INFO_IFD_POINTER entry, copy sub-IFDs
skipping the PelIfd::GPS sub-IFD, preserve the next IFD chain (thumbnail) with
setNextIfd, then call $tiff->setIfd($newIfd0) to replace IFD0 without the GPS
data.
Major changes: - Remove exif_stripping_enabled checkbox, use multiselect only - Feature is enabled when multiselect has selections - Disabled when multiselect is empty - Simplifies UI and logic - Fix PHP implode() null issue - Coerce null to empty array before implode (Config.php:398,399) - Apply fix to both exif_fields_to_keep and image_allowed_types - Fix GPS removal in Image_lib.php - Remove GPS via ifd0.removeEntry() instead of incorrect setIfd(null) - GPS INFO IFD POINTER now properly removed from IFD0 - Fix allowed_types array in Image_lib.php - Remove unsupported 'image/bmp' and 'image/tiff' - Return false instead of true for unhandled formats - Add logging for EXIF stripping failures - Log warnings when stripEXIF returns false - Helps debugging without blocking uploads - Fix migration consistency - Remove exif_stripping_enabled config (no longer needed) - Remove unused $forge variable - Make defaults consistent: Copyright,Orientation,Software - Update language strings - Remove exif_stripping_enabled translations - Clarify exif_fields_to_keep tooltip Addresses all actionable comments from CodeRabbit review
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
app/Libraries/Image_lib.php (1)
154-204: Fallback strips all APP markers indiscriminately.The fallback removes all markers from 0xE0-0xEF (APP0-APP15), which includes not just EXIF (APP1) but also JFIF (APP0), ICC color profiles (APP2), and other metadata. This may affect color accuracy for images with embedded ICC profiles.
This is acceptable as a last-resort fallback, but consider documenting this behavior or logging a more detailed warning when the fallback is triggered.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Libraries/Image_lib.php` around lines 154 - 204, The stripExifFallback in Image_lib.php currently removes all APP0-APP15 markers (APP1..APP15) without notice; update it to emit a clear warning/log when the fallback runs (include function name stripExifFallback and the $filepath) and list which APP markers were removed (e.g., APP0, APP1, APP2) so callers know if ICC profiles (APP2) or JFIF (APP0) were discarded; implement using the project logger or trigger_error, include the filepath and marker names in the message, and ensure the log is only emitted when markers are non-empty and the fallback path is taken.app/Database/Migrations/20260306120000_MigrationEXIFStrippingOptions.php (1)
26-34: Consider usingget()with row check instead ofcountAllResults().
countAllResults()issues aSELECT COUNT(*)query which is slightly heavier than simply fetching the first row to check existence.♻️ Proposed optimization
foreach ($configs as $config) { - $exists = $db->table('app_config') + $existing = $db->table('app_config') ->where('key', $config['key']) - ->countAllResults(); + ->get() + ->getRow(); - if ($exists == 0) { + if ($existing === null) { $db->table('app_config')->insert($config); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Database/Migrations/20260306120000_MigrationEXIFStrippingOptions.php` around lines 26 - 34, The loop currently checks existence using countAllResults() which issues a SELECT COUNT(*) — replace that with a lightweight row fetch: use $db->table('app_config')->where('key', $config['key'])->limit(1)->get() and check the returned row (e.g., getRow() or getRowArray()) before inserting; update the block inside the foreach ($configs as $config) to fetch one row and only call insert($config) when the fetched row is null/empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Controllers/Items.php`:
- Around line 787-795: Check the exif_stripping_enabled toggle before attempting
any EXIF work and invert the empty-list logic so an empty exif_fields_to_keep
means "keep nothing" (i.e. strip all metadata). Specifically, before creating
Image_lib and calling Image_lib->stripEXIF, ensure
$this->appconfig->get_value('exif_stripping_enabled') is truthy; parse
exif_fields_to_keep from $this->appconfig->get_value('exif_fields_to_keep') into
an array even if the config is an empty string (use array_filter but allow an
empty array to mean keep none), and then call stripEXIF($filepath,
$exif_fields_to_keep) when the toggle is enabled regardless of whether the array
is empty; keep references to $file_info and FCPATH when building $filepath and
log a warning if stripEXIF returns false.
In `@app/Database/Migrations/20260306120000_MigrationEXIFStrippingOptions.php`:
- Around line 19-24: The migration currently inserts only the
'exif_fields_to_keep' config; add a second config array entry with 'key' =>
'exif_stripping_enabled' and 'value' => '0' in the $configs variable in the up()
method (so both keys are seeded), and update the down() method (the logic that
deletes seeded configs) to remove both 'exif_fields_to_keep' and
'exif_stripping_enabled' instead of only one; locate these changes in the
MigrationEXIFStrippingOptions class methods up() and down().
In `@app/Libraries/Image_lib.php`:
- Around line 108-122: stripExifPng currently calls imagealphablending($image,
true) which will composite the alpha and corrupt transparency; change it to
imagealphablending($image, false) and keep imagesavealpha($image, true) before
imagepng to preserve the PNG alpha channel, then continue saving with
imagepng($image, $filepath, 9) and imagedestroy($image) as before; ensure these
calls are in the stripExifPng function so transparency is preserved.
- Around line 11-19: The mapping in the $exif_to_pel_tags array incorrectly maps
the 'GPS' EXIF field to PelTag::GPS_OFFSET (a GPS time offset sub-tag) which
conflicts with how GPS IFD is handled in removeExifFields() via
PelTag::GPS_INFO_IFD_POINTER; remove the 'GPS' => PelTag::GPS_OFFSET entry from
$exif_to_pel_tags (or replace it with a clear comment explaining it only applies
to GPS sub-tags) so GPS metadata is only handled by the GPS_INFO_IFD_POINTER
logic in removeExifFields() and avoid the misleading mapping.
- Around line 124-135: stripExifGif currently uses imagecreatefromgif which only
loads the first frame and will flatten animated GIFs; modify stripExifGif to
first detect animation (e.g., read the GIF file contents and search for multiple
frame signatures like the graphic control extension sequence "\x21\xF9\x04" or
the "NETSCAPE2.0" application block) and if multiple frames are found, skip
re-encoding and return early (leave the file untouched or return false/success
per existing convention). If not animated, proceed with the current
imagecreatefromgif -> imagegif -> imagedestroy flow; keep the function name
stripExifGif and the imagecreatefromgif/imagegif calls intact and only add the
pre-check to avoid flattening animations.
---
Nitpick comments:
In `@app/Database/Migrations/20260306120000_MigrationEXIFStrippingOptions.php`:
- Around line 26-34: The loop currently checks existence using countAllResults()
which issues a SELECT COUNT(*) — replace that with a lightweight row fetch: use
$db->table('app_config')->where('key', $config['key'])->limit(1)->get() and
check the returned row (e.g., getRow() or getRowArray()) before inserting;
update the block inside the foreach ($configs as $config) to fetch one row and
only call insert($config) when the fetched row is null/empty.
In `@app/Libraries/Image_lib.php`:
- Around line 154-204: The stripExifFallback in Image_lib.php currently removes
all APP0-APP15 markers (APP1..APP15) without notice; update it to emit a clear
warning/log when the fallback runs (include function name stripExifFallback and
the $filepath) and list which APP markers were removed (e.g., APP0, APP1, APP2)
so callers know if ICC profiles (APP2) or JFIF (APP0) were discarded; implement
using the project logger or trigger_error, include the filepath and marker names
in the message, and ensure the log is only emitted when markers are non-empty
and the fallback path is taken.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2de76b77-5cd0-4b68-af45-da13480efcc8
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
app/Controllers/Config.phpapp/Controllers/Items.phpapp/Database/Migrations/20260306120000_MigrationEXIFStrippingOptions.phpapp/Language/en/Config.phpapp/Libraries/Image_lib.phpapp/Views/configs/general_config.phpcomposer.json
🚧 Files skipped from review as they are similar to previous changes (4)
- composer.json
- app/Views/configs/general_config.php
- app/Language/en/Config.php
- app/Controllers/Config.php
app/Database/Migrations/20260306120000_MigrationEXIFStrippingOptions.php
Show resolved
Hide resolved
Image_lib.php fixes: - Remove incorrect 'GPS' => PelTag::GPS_OFFSET mapping (GPS handled separately) - Fix PNG transparency: change imagealphablending to false - Add animated GIF detection: skip processing for animations - Add logging in stripExifFallback to show which APP markers are removed Migration optimization: - Use get()->getRow() instead of countAllResults() for lighter query - More efficient existence check before insert All changes address CodeRabbit technical review comments while maintaining the simplified multiselect-only UI (no toggle needed).
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/Libraries/Image_lib.php (2)
145-160: Consider animated WebP detection to avoid flattening animations.Similar to GIF, animated WebP files will lose their animation when re-encoded via
imagecreatefromwebp(). Animated WebP files contain multiple frames that GD cannot preserve. Additionally, re-encoding lossy WebP at quality 100 may still cause minor quality degradation.For animated WebP detection, you can check for the
ANIMchunk in the file header.♻️ Optional: Add animated WebP detection
private function stripExifWebp(string $filepath): bool { if (!function_exists('imagecreatefromwebp')) { return false; } + // Check for animated WebP (contains ANIM chunk) + $content = file_get_contents($filepath, false, null, 0, 32); + if ($content !== false && strpos($content, 'ANIM') !== false) { + return true; // Skip animated WebP + } + $image = `@imagecreatefromwebp`($filepath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Libraries/Image_lib.php` around lines 145 - 160, The stripExifWebp method currently re-encodes all WebP files (using imagecreatefromwebp + imagewebp) which will flatten animated WebP frames; update stripExifWebp to first detect animated WebP by scanning the file for an ANIM chunk (check the file header/bytes for "ANIM") and if an ANIM chunk is present, skip re-encoding and return false (or leave the original file intact); only call imagecreatefromwebp/imagewebp for non-animated WebP files, and avoid forced quality=100 re-encoding (preserve original or use a configurable quality) to minimize quality changes.
162-171: Consider validating JPEG structure before parsing.The fallback parser assumes valid JPEG structure but doesn't verify the file starts with the SOI marker (
0xFFD8). While this is mitigated by the earlier MIME type check and PelJpeg validation, adding a quick SOI check would improve robustness for edge cases.♻️ Optional: Add SOI marker validation
private function stripExifFallback(string $filepath): bool { $content = file_get_contents($filepath); if ($content === false) { return false; } + // Validate JPEG starts with SOI marker + if (strlen($content) < 2 || ord($content[0]) !== 0xFF || ord($content[1]) !== 0xD8) { + log_message('warning', "stripExifFallback: Invalid JPEG structure in {$filepath}"); + return false; + } + $markers = []; $offset = 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Libraries/Image_lib.php` around lines 162 - 171, The stripExifFallback function assumes a valid JPEG but doesn't verify the SOI marker; before parsing, check that the file content begins with the JPEG SOI bytes (0xFF 0xD8) and return false if it does not; update stripExifFallback to inspect the first two bytes of $content (or the file via file_get_contents) and bail out early when the SOI marker is missing to avoid parsing non-JPEG data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Libraries/Image_lib.php`:
- Around line 81-83: The code only calls
$ifd0->removeEntry(PelTag::GPS_INFO_IFD_POINTER), which deletes the pointer tag
but leaves the GPS sub-IFD in IFD0's internal $sub array so PEL will recreate
the pointer on serialization; fix this in Image_lib.php by using a
ReflectionProperty on $ifd0->sub (make it accessible, getValue, unset the
PelIfd::GPS index from the array, then setValue back) to remove the GPS sub-IFD
entirely instead of only removing the pointer tag.
---
Nitpick comments:
In `@app/Libraries/Image_lib.php`:
- Around line 145-160: The stripExifWebp method currently re-encodes all WebP
files (using imagecreatefromwebp + imagewebp) which will flatten animated WebP
frames; update stripExifWebp to first detect animated WebP by scanning the file
for an ANIM chunk (check the file header/bytes for "ANIM") and if an ANIM chunk
is present, skip re-encoding and return false (or leave the original file
intact); only call imagecreatefromwebp/imagewebp for non-animated WebP files,
and avoid forced quality=100 re-encoding (preserve original or use a
configurable quality) to minimize quality changes.
- Around line 162-171: The stripExifFallback function assumes a valid JPEG but
doesn't verify the SOI marker; before parsing, check that the file content
begins with the JPEG SOI bytes (0xFF 0xD8) and return false if it does not;
update stripExifFallback to inspect the first two bytes of $content (or the file
via file_get_contents) and bail out early when the SOI marker is missing to
avoid parsing non-JPEG data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2c4d9fa3-e287-4384-ba6b-93f5568d5e79
📒 Files selected for processing (2)
app/Database/Migrations/20260306120000_MigrationEXIFStrippingOptions.phpapp/Libraries/Image_lib.php
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Database/Migrations/20260306120000_MigrationEXIFStrippingOptions.php
Comment on lines
+81
to
+83
| if (!in_array('GPS', $fields_to_keep)) { | ||
| $ifd0->removeEntry(PelTag::GPS_INFO_IFD_POINTER); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's a way to verify GPS removal or if there are any tests
rg -n 'GPS|gps' app/Libraries/Image_lib.php -A 2 -B 2
# Look for any test files that might verify this behavior
fd -e php -g '*test*' | xargs -I {} rg -l 'Image_lib|stripEXIF|GPS' {} 2>/dev/null || echo "No test files found"Repository: opensourcepos/opensourcepos
Length of output: 286
🌐 Web query:
fileeye/pel removeEntry GPS sub-IFD removal
💡 Result:
In fileeye/pel (PEL), GPS metadata lives in the GPS sub-IFD (type PelIfd::GPS). You can delete entries with the ArrayAccess “unset” API (offsetUnset) and you can attach a sub-IFD with addSubIfd(), but there is no public “remove sub-IFD” method. (pel.github.io)
Remove GPS entries (keeps an empty GPS IFD)
$gps = $ifd0->getSubIfd(PelIfd::GPS);
if ($gps) {
foreach ($gps->getEntries() as $tag => $_entry) {
unset($gps[$tag]); // PelIfd::offsetUnset()
}
}unset($ifd[$tag]) is the supported way to delete an entry. (pel.github.io)
Remove the GPS sub-IFD entirely (so it won’t be written back)
When serializing, PEL writes sub-IFDs based on the parent’s internal $sub list (and generates pointer entries like GPS_INFO_IFD_POINTER from that list). So deleting only the pointer tag is not sufficient if the sub-IFD still exists. (pel.github.io)
Practical workaround: remove the GPS element from the private $sub array via reflection:
$rp = new ReflectionProperty($ifd0, 'sub');
$rp->setAccessible(true);
$sub = $rp->getValue($ifd0);
unset($sub[PelIfd::GPS]);
$rp->setValue($ifd0, $sub);If you want, paste your current snippet (how you’re calling removeEntry / unset) and I’ll tailor this to your exact object chain (PelJpeg → PelExif → PelTiff → PelIfd).
Citations:
- 1: https://pel.github.io/pel/doc/PEL/PelIfd.html
- 2: https://pel.github.io/pel/doc/__filesource/fsource_PEL__srcPelIfd.php.html
- 3: https://pel.github.io/pel/doc/__filesource/fsource_PEL__srcPelIfd.php.html
GPS data is not being removed; only the pointer tag is deleted.
In PEL, GPS metadata is stored as a sub-IFD in IFD0's internal $sub array. Calling removeEntry(PelTag::GPS_INFO_IFD_POINTER) removes the pointer tag but leaves the GPS sub-IFD intact. When PEL serializes the image, it regenerates the GPS pointer tag from the internal $sub array, re-adding GPS data to the output.
To properly remove GPS, remove the GPS sub-IFD from IFD0's internal list:
$rp = new ReflectionProperty($ifd0, 'sub');
$rp->setAccessible(true);
$sub = $rp->getValue($ifd0);
unset($sub[PelIfd::GPS]);
$rp->setValue($ifd0, $sub);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Libraries/Image_lib.php` around lines 81 - 83, The code only calls
$ifd0->removeEntry(PelTag::GPS_INFO_IFD_POINTER), which deletes the pointer tag
but leaves the GPS sub-IFD in IFD0's internal $sub array so PEL will recreate
the pointer on serialization; fix this in Image_lib.php by using a
ReflectionProperty on $ifd0->sub (make it accessible, getValue, unset the
PelIfd::GPS index from the array, then setValue back) to remove the GPS sub-IFD
entirely instead of only removing the pointer tag.
Comment on lines
+130
to
+132
| if (strpos($content, "\x21\xF9\x04") !== false || strpos($content, 'NETSCAPE2.0') !== false) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Animated GIF detection is overly broad and will skip processing most static GIFs.
The Graphic Control Extension (\x21\xF9\x04) is present in virtually all GIF files, not just animated ones. Static GIFs with transparency or any delay timing also contain this marker. This check will incorrectly classify most static GIFs as animated and skip EXIF stripping.
A more reliable approach is to count Image Descriptor blocks (0x2C), as animated GIFs have multiple frames (multiple image descriptors).
🐛 Proposed fix for accurate animated GIF detection
private function stripExifGif(string $filepath): bool
{
$content = file_get_contents($filepath);
if ($content === false) {
return false;
}
- if (strpos($content, "\x21\xF9\x04") !== false || strpos($content, 'NETSCAPE2.0') !== false) {
+ // Check for NETSCAPE loop extension (strong indicator of animation)
+ if (strpos($content, 'NETSCAPE2.0') !== false) {
+ return true;
+ }
+
+ // Count Image Descriptor blocks (0x2C) - multiple frames indicate animation
+ if (substr_count($content, "\x2C") > 1) {
return true;
}
$image = `@imagecreatefromgif`($filepath);
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
2 participants
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
This PR addresses all review comments from #4394:
✅ Completed Changes
PSR-12 Compliance: Renamed
strip_exif()tostripEXIF()(app/Libraries/Image_lib.php:7)Configuration Options: Added two new configuration keys:
exif_stripping_enabled: Toggle to enable/disable EXIF stripping (defaults to '0' for backward compatibility)exif_fields_to_keep: Comma-separated list of EXIF fields to preserve (defaults to 'Copyright,Orientation')Conditional Stripping: Updated Config and Items controllers to only strip EXIF when enabled:
Migration: Created migration to add new config keys to existing installations
📝 Design Decisions
Following the reviewer's "Good, Better, Best" framework:
🔒 Security Benefits
🔧 Testing
Closes #4394 review comments
Summary by CodeRabbit
New Features
Localization
Database