Skip to content

Update file name #1541

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tarunnjoshi wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Update file name #1541

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
141 changes: 141 additions & 0 deletions wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
use Civi\Traits\QrCodeable;
use Civi\InductionService;
use Civi\Api4\Event;
use Civi\Core\Event\GenericHookEvent;

/**
*
Expand Down Expand Up @@ -95,10 +96,150 @@ public static function getSubscribedEvents() {
['validateReceiptFromEmail'],

],
'hook_civicrm_postSave_civicrm_file' => [
['renameAfformUploadedImages', 0],
],

];
}


public static function renameAfformUploadedImages(\Civi\Core\Event\GenericHookEvent $event): void {
error_log('renameAfformUploadedImages: START');
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.


static $inProgress = [];

$dao = $event->getHookValues()[0] ?? NULL;

if (!$dao) {
error_log('renameAfformUploadedImages: dao is NULL, returning');
return;
}

// Basic fields
$id = $dao->id ?? NULL;
$uri = $dao->uri ?? NULL;
$mime = $dao->mime_type ?? NULL;

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));

if (empty($id) || empty($uri) || empty($mime)) {
error_log('renameAfformUploadedImages: missing id/uri/mime, returning');
return;
}

if (strpos($mime, 'image/') !== 0) {
error_log('renameAfformUploadedImages: not an image, returning');
return;
}

if (!empty($inProgress[$id])) {
error_log("renameAfformUploadedImages: inProgress for file_id=$id, returning");
return;
}

$prefix = 'testing-image-coloredcow-';
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

$baseName = basename($uri);

if (strpos($baseName, $prefix) === 0) {
error_log("renameAfformUploadedImages: already renamed ($baseName), returning");
return;
}

$inProgress[$id] = TRUE;

try {
$ext = pathinfo($baseName, PATHINFO_EXTENSION);
$dir = dirname($uri);
$newBaseName = $prefix . $id . ($ext ? ('.' . $ext) : '');
$newUri = ($dir === '.' ? '' : ($dir . '/')) . $newBaseName;

error_log('renameAfformUploadedImages: baseName=' . $baseName);
error_log('renameAfformUploadedImages: ext=' . $ext);
error_log('renameAfformUploadedImages: dir=' . $dir);
error_log('renameAfformUploadedImages: newBaseName=' . $newBaseName);
error_log('renameAfformUploadedImages: newUri=' . $newUri);

// Base path (usually .../uploads/civicrm/)
$basePath = \Civi::paths()->getPath('[civicrm.files]/');
$basePath = rtrim($basePath, '/') . '/';

error_log('renameAfformUploadedImages: basePath=' . $basePath);

// Candidate old paths:
// 1) basePath + uri
// 2) basePath + "custom/" + basename(uri) (common for custom files dir)
$oldPath1 = $basePath . ltrim($uri, '/');
$oldPath2 = $basePath . 'custom/' . $baseName;

// Candidate new paths:
// If we rename inside "custom/", keep it inside custom/ too.
$newPath1 = $basePath . ltrim($newUri, '/');
$newPath2 = $basePath . 'custom/' . $newBaseName;

error_log('renameAfformUploadedImages: oldPath1=' . $oldPath1 . ' exists=' . (file_exists($oldPath1) ? 'YES' : 'NO'));
error_log('renameAfformUploadedImages: oldPath2=' . $oldPath2 . ' exists=' . (file_exists($oldPath2) ? 'YES' : 'NO'));

$renameOk = FALSE;

if (file_exists($oldPath1)) {
error_log('renameAfformUploadedImages: attempting rename oldPath1 -> newPath1');
$renameOk = @rename($oldPath1, $newPath1);
error_log('renameAfformUploadedImages: rename result=' . ($renameOk ? 'OK' : 'FAILED'));
error_log('renameAfformUploadedImages: newPath1 exists=' . (file_exists($newPath1) ? 'YES' : 'NO'));

if ($renameOk) {
// DB uri should match location relative to basePath
// (If you want to preserve subdir, newUri already has it)
civicrm_api4('File', 'update', [
'where' => [['id', '=', (int) $id]],
'values' => ['uri' => $newUri],
'checkPermissions' => FALSE,
]);
error_log('renameAfformUploadedImages: DB updated uri=' . $newUri);
}
}
elseif (file_exists($oldPath2)) {
error_log('renameAfformUploadedImages: attempting rename oldPath2 -> newPath2 (custom/)');
$renameOk = @rename($oldPath2, $newPath2);
error_log('renameAfformUploadedImages: rename result=' . ($renameOk ? 'OK' : 'FAILED'));
error_log('renameAfformUploadedImages: newPath2 exists=' . (file_exists($newPath2) ? 'YES' : 'NO'));

if ($renameOk) {
// IMPORTANT: In this case, the uri should include 'custom/'
$dbUri = 'custom/' . $newBaseName;

civicrm_api4('File', 'update', [
'where' => [['id', '=', (int) $id]],
'values' => ['uri' => $dbUri],
'checkPermissions' => FALSE,
]);
error_log('renameAfformUploadedImages: DB updated uri=' . $dbUri);
}
}
else {
error_log('renameAfformUploadedImages: file not found in either location. NOT updating DB to avoid broken uri.');
error_log('renameAfformUploadedImages: expected locations were:');
error_log(' - ' . $oldPath1);
error_log(' - ' . $oldPath2);
}
}
catch (\Throwable $e) {
error_log('renameAfformUploadedImages: EXCEPTION ' . $e->getMessage());
\Civi::log()->error('File rename failed: ' . $e->getMessage(), [
'file_id' => $id ?? NULL,
'uri' => $uri ?? NULL,
]);
}
finally {
unset($inProgress[$id]);
error_log('renameAfformUploadedImages: END');
}
}


/**
*
*/
Expand Down