Conversation
WalkthroughA new event handler Changes
Sequence DiagramsequenceDiagram
participant Hook as CiviCRM Hook System
participant Handler as renameAfformUploadedImages
participant FileSystem as File System
participant DB as CiviCRM API4
participant Logger as Logger
Hook->>Handler: hook_civicrm_postSave_civicrm_file (event)
Handler->>Handler: Validate MIME type & extract file data
Handler->>Handler: Check inProgress guard
Handler->>Handler: Construct old & new URI paths
Handler->>FileSystem: Check if file exists in base path
alt File found in base path
FileSystem-->>Handler: File exists
Handler->>FileSystem: `@rename`(oldPath, newPath)
FileSystem-->>Handler: Rename success
Handler->>DB: File.update(uri: newUri)
DB-->>Handler: DB updated
else File found in custom/ path
Handler->>FileSystem: Check custom/ directory
FileSystem-->>Handler: File exists
Handler->>FileSystem: `@rename`(oldPath, newUri_custom)
Handler->>DB: File.update(uri: custom_newUri)
DB-->>Handler: DB updated
else File not found
Handler->>Logger: Log skip (file not found)
end
Handler->>Logger: Log completion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 2
🤖 Fix all issues with AI agents
In `@wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php`:
- Line 108: The method renameAfformUploadedImages contains 20+ raw error_log
calls that clutter production logs; replace non-essential debug statements with
either removal or conditional structured logging using CiviCRM's logger (e.g.,
\Civi::log()->debug(...)) and retain only meaningful error-level logs as
\Civi::log()->error(...) when failures occur; update calls inside
renameAfformUploadedImages (and related debug blocks) to use the structured
logger and proper log levels, or remove them entirely so only true error
conditions remain logged.
- Line 143: The hardcoded test prefix 'testing-image-coloredcow-' in
CollectionCampService should be replaced with a configurable constant or
configuration lookup; replace the direct $prefix assignment with a reference to
a class constant (e.g. self::IMAGE_PREFIX) or a config method (e.g.
$this->config->get('collection_image_prefix')) and add that constant
(IMAGE_PREFIX) to the CollectionCampService class or wire the config key so
production values can be set without changing code.
🧹 Nitpick comments (1)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (1)
189-189: Error suppression with@renamehides potential issues.Using
@to suppress errors makes debugging difficult. Consider logging the actual error if rename fails.♻️ Suggested improvement
- $renameOk = `@rename`($oldPath1, $newPath1); - error_log('renameAfformUploadedImages: rename result=' . ($renameOk ? 'OK' : 'FAILED')); + $renameOk = rename($oldPath1, $newPath1); + if (!$renameOk) { + \Civi::log()->warning('File rename failed', [ + 'from' => $oldPath1, + 'to' => $newPath1, + 'error' => error_get_last()['message'] ?? 'unknown', + ]); + }Also applies to: 206-206
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: tarunnjoshi
Repo: ColoredCow/goonj PR: 1230
File: wp-content/civi-extensions/civirazorpay/cli/update-recurring-contact-info.php:115-117
Timestamp: 2025-05-27T07:31:44.131Z
Learning: For one-time scripts in the Goonj project, tarunnjoshi prefers to keep hardcoded values rather than creating shared constants or configuration files, prioritizing simplicity over strict adherence to DRY principle to avoid over-engineering temporary code.
📚 Learning: 2024-10-11T09:53:14.458Z
Learnt from: belwalshubham
Repo: ColoredCow/goonj PR: 284
File: wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php:520-520
Timestamp: 2024-10-11T09:53:14.458Z
Learning: In `wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php`, it's acceptable that the `reGenerateCollectionCampQr` method calls `generateQrCode` with only the `$collectionCampId` parameter, as QR codes are generated when the Dropping Center is authorized. We'll implement changes if any issues arise in the future.
Applied to files:
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php
🔇 Additional comments (7)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (7)
25-25: LGTM!Import added correctly for
GenericHookEventwhich is needed for the new hook handler.
99-101: LGTM!Event subscription correctly wired to the CiviCRM file post-save hook.
110-110: Re-entrancy guard implementation is appropriate.The static
$inProgressarray correctly prevents duplicate processing when the database update triggers the same hook. Thefinallyblock ensures cleanup even on exceptions.Also applies to: 138-141, 151-151, 236-238
112-131: Input validation is well-implemented.The method properly validates the DAO object, checks for required fields (id, uri, mime_type), and correctly filters for image MIME types only.
196-200: Database updates use proper API4 pattern.The
civicrm_api4calls correctly update the File record withcheckPermissionsdisabled, which is appropriate for system-level operations.Also applies to: 214-218
229-235: Exception handling is adequate.The catch block logs the error using CiviCRM's logger with relevant context. This is the correct approach for production error handling.
154-157: Path reconstruction logic is already handling edge cases appropriately.The empty extension case (line 156) is correctly handled by the ternary operator—when
$extis empty, no period is prepended. The case wheredirname($uri)returns'.'(file without a directory component) is also explicitly handled on line 157 with the conditional check.However, neither the filename-only URI case nor special characters in directory names require special handling here since the code operates on the URI string directly. For actual filesystem operations downstream (lines 174–180), ensure any path components are properly escaped if they're user-controlled or come from untrusted sources. The current logging (lines 159–163) helps catch unexpected path formats during development.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
|
||
|
|
||
| public static function renameAfformUploadedImages(\Civi\Core\Event\GenericHookEvent $event): void { | ||
| error_log('renameAfformUploadedImages: START'); |
There was a problem hiding this comment.
Excessive error_log calls throughout the method.
The method contains 20+ error_log calls which will pollute server logs in production. These debug statements should either be removed entirely or converted to use CiviCRM's structured logging with appropriate log levels (e.g., \Civi::log()->debug()).
🔧 Suggested approach
Replace verbose error_log calls with conditional debug logging:
public static function renameAfformUploadedImages(\Civi\Core\Event\GenericHookEvent $event): void {
- error_log('renameAfformUploadedImages: START');
+ \Civi::log()->debug('renameAfformUploadedImages: START');
// ... validation logic ...
- error_log('renameAfformUploadedImages: dao.id=' . print_r($id, TRUE));
- error_log('renameAfformUploadedImages: dao.uri=' . print_r($uri, TRUE));
- error_log('renameAfformUploadedImages: dao.mime=' . print_r($mime, TRUE));
+ \Civi::log()->debug('renameAfformUploadedImages: processing file', [
+ 'id' => $id,
+ 'uri' => $uri,
+ 'mime' => $mime,
+ ]);Better yet, remove most of these logs entirely and keep only essential ones for error scenarios. The current implementation logs every step which is excessive for production.
Also applies to: 124-126, 129-130, 134-135, 139-140, 147-148, 159-163, 169-169, 182-183, 188-191, 205-208, 219-219, 223-227, 230-231, 238-238
🤖 Prompt for AI Agents
In `@wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php` at
line 108, The method renameAfformUploadedImages contains 20+ raw error_log calls
that clutter production logs; replace non-essential debug statements with either
removal or conditional structured logging using CiviCRM's logger (e.g.,
\Civi::log()->debug(...)) and retain only meaningful error-level logs as
\Civi::log()->error(...) when failures occur; update calls inside
renameAfformUploadedImages (and related debug blocks) to use the structured
logger and proper log levels, or remove them entirely so only true error
conditions remain logged.
| return; | ||
| } | ||
|
|
||
| $prefix = 'testing-image-coloredcow-'; |
There was a problem hiding this comment.
Hardcoded test prefix should be updated for production.
The prefix 'testing-image-coloredcow-' appears to be a development/test value. For production, consider using a more appropriate prefix or making it configurable via a class constant.
🔧 Suggested fix
+ const IMAGE_FILE_PREFIX = 'goonj-afform-';
+
public static function renameAfformUploadedImages(\Civi\Core\Event\GenericHookEvent $event): void {
// ...
- $prefix = 'testing-image-coloredcow-';
+ $prefix = self::IMAGE_FILE_PREFIX;🤖 Prompt for AI Agents
In `@wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php` at
line 143, The hardcoded test prefix 'testing-image-coloredcow-' in
CollectionCampService should be replaced with a configurable constant or
configuration lookup; replace the direct $prefix assignment with a reference to
a class constant (e.g. self::IMAGE_PREFIX) or a config method (e.g.
$this->config->get('collection_image_prefix')) and add that constant
(IMAGE_PREFIX) to the CollectionCampService class or wire the config key so
production values can be set without changing code.
1 participant
Update file name
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.