Refactor Event class and improve architecture with caching and logging#58
Conversation
- Add Platform enum and ChatTarget DTO - Add in-memory caching for Event and Setting models - Replace error_log() with PSR-3 LoggerInterface in Validator - Fix EventTrait bug (was assigning string to Event object) - Fix all implicit nullable parameter deprecations - Add retry logic with exponential backoff for Telegram API - Replace unsafe extract() in ConfigHelper with closure scope - Use match expressions, readonly properties, #[NoDiscard] - Centralize Telegram API calls in Webhook (eliminate 4x duplication) - Add strict type comparisons throughout - Fix test paths (storage/json/tgn/ → config/jsons/) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the Event class and several core components to improve the architecture with:
- In-memory caching for file-based configuration (JSON event configs and settings)
- Centralized retry logic with exponential backoff for Telegram API calls
- PSR-3 logging integration via
psr/logreplacingerror_log()calls - A new
ChatTargetDTO andPlatformenum introducing structured data modeling - Relocation of default JSON config files from
storage/json/tgn/toconfig/jsons/
Changes:
- Introduced in-memory caching (
$dirty,lastLoadedFile) to bothEventandSettingmodels, and replacedsaveEventConfig()/saveSettingsToFile()with a unifiedpersist()+loadFromFile()pattern - Extracted duplicate Telegram API call logic into a single
callTelegramApi()inWebhook.phpandsendWithRetry()inNotification.php, both with exponential backoff for HTTP 429 rate limiting - Added
ChatTargetDTO,Platformenum, and PSR-3 logger support inValidator, replacing raw string parsing anderror_logcalls
Reviewed changes
Copilot reviewed 23 out of 26 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
src/Models/Event.php |
Adds caching (dirty/lastLoadedFile), replaces getter/setter for platformFile with PHP 8.4 property hook, extracts loadFromFile()/persist() |
src/Models/Setting.php |
Adds caching, extracts loadFromFile()/persist(), adds reloadSettings() |
src/Webhook.php |
Extracts repeated Telegram API calls into callTelegramApi() with retry logic; adds DI for Client |
src/Structures/Notification.php |
Adds sendWithRetry() with retry backoff; improves setPayload() null safety; adds #[\NoDiscard] |
src/Objects/Validator.php |
Injects PSR-3 LoggerInterface, replaces error_log with structured debug log |
src/Notifier.php |
Replaces NotificationConstant parsing with ChatTarget DTO; adds getChatTargets() |
src/DTOs/ChatTarget.php |
New DTO for parsing chat ID + thread ID strings |
src/Enums/Platform.php |
New enum representing supported platforms with helper methods |
src/Constants/EventConstant.php |
Adds platformEnum() bridge method to Platform::from() |
src/Structures/TelegramBot.php |
Improves isOwner() and isNotifyChat() type safety |
src/Trait/EventTrait.php |
Switches to direct property assignment for platformFile, minor cleanups |
src/Trait/EventSettingTrait.php |
Converts if-elseif to match, fixes nullable parameter annotation |
src/Helpers/ConfigHelper.php |
Isolates template rendering in a closure; changes require_once to require |
config/tg-notifier.php |
Updates default JSON file paths from storage/json/tgn/ to config/jsons/ |
config/jsons/*.json |
New canonical location for default event/settings JSON config files |
tests/*.php |
Updates path references from old to new config locations |
composer.json |
Adds psr/log: ^3.0 dependency |
src/Interfaces/Structures/NotificationInterface.php |
Updates nullable parameter annotations |
src/Interfaces/Structures/AppInterface.php |
Updates nullable parameter annotations |
src/Structures/App.php |
Updates nullable parameter annotations |
src/Exceptions/InvalidViewTemplateException.php |
Updates nullable parameter annotation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $json = file_get_contents($this->settingFile); | ||
|
|
||
| if (!empty($json)) { | ||
| $this->settings = json_decode($json, true) ?? []; | ||
| } |
There was a problem hiding this comment.
In Setting::loadFromFile(), if file_get_contents() returns content but the JSON is empty (e.g., the file exists but contains only whitespace), $this->settings is not updated and retains its previous value. This means after a reloadSettings() call on a corrupted or empty file, stale data could persist. The same issue exists in Event::loadFromFile(). The $this->settings (or $this->eventConfig) should be reset to [] unconditionally at the start of loadFromFile(), or specifically when the JSON content is empty after reading a valid file.
|
|
||
| use CSlant\TelegramGitNotifier\Constants\EventConstant; | ||
| use CSlant\TelegramGitNotifier\Constants\NotificationConstant; | ||
| use CSlant\TelegramGitNotifier\DTOs\ChatTarget; |
There was a problem hiding this comment.
After removing the NotificationConstant import from Notifier.php and replacing the parsing logic with ChatTarget, the NotificationConstant class appears to be unused across the entire codebase. The separator constants (;, :, ,) are now hardcoded in ChatTarget::fromString() and ChatTarget::parseMultiple(). The NotificationConstant class should be removed to avoid dead code, or the ChatTarget methods should use the constants to maintain a single source of truth for the format.
| public function getChatTargets(?string $chatIds = null): array | ||
| { | ||
| $raw = $chatIds ?? config('telegram-git-notifier.bot.notify_chat_ids'); | ||
|
|
||
| return ChatTarget::parseMultiple($raw ?? ''); | ||
| } |
There was a problem hiding this comment.
The new getChatTargets() method has no test coverage, while the similar parseNotifyChatIds() is tested in NotifierTest.php. Given that the project has established tests for related methods, getChatTargets() should also be tested to verify that it correctly parses chat IDs into ChatTarget DTOs.
| private function sendWithRetry(string $url, array $options, int $maxRetries = 3): bool | ||
| { | ||
| $attempt = 0; | ||
|
|
||
| while (true) { | ||
| try { | ||
| $response = $this->client->request('POST', $url, $options); | ||
|
|
||
| $body = (string) $response->getBody(); | ||
| if ($response->getStatusCode() === 200) { | ||
| return true; | ||
| } | ||
|
|
||
| throw SendNotificationException::create($body); | ||
| } catch (GuzzleException $e) { | ||
| throw SendNotificationException::create($e->getMessage()); | ||
| $body = (string) $response->getBody(); | ||
|
|
||
| // Telegram returns 429 for rate limiting | ||
| if ($response->getStatusCode() === 429 && $attempt < $maxRetries) { | ||
| $retryAfter = json_decode($body, true)['parameters']['retry_after'] ?? (2 ** $attempt); | ||
| usleep((int) ($retryAfter * 1_000_000)); | ||
| $attempt++; | ||
|
|
||
| continue; | ||
| } | ||
|
|
||
| throw SendNotificationException::create($body); | ||
| } catch (GuzzleException $e) { | ||
| if ($attempt < $maxRetries && str_contains($e->getMessage(), '429')) { | ||
| usleep(2 ** $attempt * 1_000_000); | ||
| $attempt++; | ||
|
|
||
| continue; | ||
| } | ||
|
|
||
| throw SendNotificationException::create($e->getMessage()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The new retry logic in sendWithRetry() (exponential backoff, Telegram API retry_after header handling) is not covered by any tests. Since the project has established test patterns and this is new behaviour involving both the HTTP 429 status path and GuzzleException path, it would benefit from test coverage to guard against regressions.
|
|
||
| /** | ||
| * @return array<string, string> | ||
| */ | ||
| public static function webhookEventHeaders(): array | ||
| { | ||
| return array_combine( | ||
| array_map(fn (self $p) => $p->value, self::cases()), | ||
| array_map(fn (self $p) => $p->webhookEventHeader(), self::cases()), | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * @return array<string, string> | ||
| */ | ||
| public static function eventSeparators(): array | ||
| { | ||
| return array_combine( | ||
| array_map(fn (self $p) => $p->value, self::cases()), | ||
| array_map(fn (self $p) => $p->eventSeparator(), self::cases()), | ||
| ); | ||
| } | ||
|
|
||
| public static function default(): self | ||
| { | ||
| return self::GitHub; | ||
| } |
There was a problem hiding this comment.
The Platform enum introduces methods webhookEventHeaders(), eventSeparators(), and default() that are never called anywhere in the codebase. Additionally, the data they return (webhook event headers and event separators) duplicates what's already in EventConstant::WEBHOOK_EVENT_HEADER and EventConstant::PLATFORM_EVENT_SEPARATOR. This creates a maintenance burden since both places need to stay in sync when new platforms are added. If the enum is the canonical source of truth, the constants should be removed and replaced with enum usages. If constants are canonical, the static methods on the enum are redundant.
| /** | |
| * @return array<string, string> | |
| */ | |
| public static function webhookEventHeaders(): array | |
| { | |
| return array_combine( | |
| array_map(fn (self $p) => $p->value, self::cases()), | |
| array_map(fn (self $p) => $p->webhookEventHeader(), self::cases()), | |
| ); | |
| } | |
| /** | |
| * @return array<string, string> | |
| */ | |
| public static function eventSeparators(): array | |
| { | |
| return array_combine( | |
| array_map(fn (self $p) => $p->value, self::cases()), | |
| array_map(fn (self $p) => $p->eventSeparator(), self::cases()), | |
| ); | |
| } | |
| public static function default(): self | |
| { | |
| return self::GitHub; | |
| } |
| if (empty($this->platformFile) || !file_exists($this->platformFile)) { | ||
| $this->eventConfig = []; | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| $json = file_get_contents($this->platformFile); | ||
|
|
||
| if (!empty($json)) { | ||
| $this->eventConfig = json_decode($json, true) ?? []; |
There was a problem hiding this comment.
Same as the issue in Setting::loadFromFile(): if the platform JSON file exists but its content is empty or whitespace, $this->eventConfig is not reset to [] and retains its previous value. Calling reloadEventConfig() on a corrupted or emptied file would silently keep stale event configuration. The property should be reset before attempting to parse the file's contents.
| if (empty($this->platformFile) || !file_exists($this->platformFile)) { | |
| $this->eventConfig = []; | |
| return; | |
| } | |
| $json = file_get_contents($this->platformFile); | |
| if (!empty($json)) { | |
| $this->eventConfig = json_decode($json, true) ?? []; | |
| // Always reset to avoid keeping stale configuration if the file is empty or invalid. | |
| $this->eventConfig = []; | |
| if (empty($this->platformFile) || !file_exists($this->platformFile)) { | |
| $this->lastLoadedFile = ''; | |
| $this->dirty = false; | |
| return; | |
| } | |
| $json = file_get_contents($this->platformFile); | |
| $json = is_string($json) ? trim($json) : ''; | |
| if ($json !== '') { | |
| $decoded = json_decode($json, true); | |
| if (is_array($decoded)) { | |
| $this->eventConfig = $decoded; | |
| } |
| public function getChatTargets(?string $chatIds = null): array | ||
| { | ||
| $raw = $chatIds ?? config('telegram-git-notifier.bot.notify_chat_ids'); | ||
|
|
||
| return ChatTarget::parseMultiple($raw ?? ''); | ||
| } |
There was a problem hiding this comment.
The new public getChatTargets() method is not declared in NotificationInterface, while the related parseNotifyChatIds() method is. Since Notifier implements NotificationInterface, callers programming to the interface won't have access to getChatTargets(). Either add getChatTargets() to NotificationInterface or, if it is only intended as a helper on the concrete class, mark it clearly and document it accordingly.
| @@ -57,16 +55,24 @@ public function getTemplateData(string $partialPath, array $data = []): bool|str | |||
| return ''; | |||
| } | |||
|
|
|||
| $content = ''; | |||
| ob_start(); | |||
|
|
|||
| try { | |||
| extract($data, EXTR_SKIP); | |||
| require_once $viewPathFile; | |||
| $content = ob_get_clean(); | |||
| $content = (static function (string $__viewFile, array $__data): string { | |||
| extract($__data, EXTR_SKIP); | |||
| ob_start(); | |||
| require $__viewFile; | |||
|
|
|||
There was a problem hiding this comment.
getTemplateData() builds a filesystem path from the partialPath argument and then requires it, but the library composes partialPath from webhook event and action values that ultimately come from HTTP headers and JSON payload (Notification::setMessage() → tgn_view() → this method) without any sanitization. An attacker who can send requests to the webhook endpoint can craft event/action strings containing ../ segments so that, after str_replace('.', '/', $partialPath), the resulting $viewPathFile traverses out of the intended views directory and includes arbitrary PHP files under the configured base path. To mitigate this, normalize and strictly validate partialPath (e.g., allowlisted event/action names and rejection of .. or path separators) before building $viewPathFile, and avoid using raw user-controlled data in paths passed to require.
- Replace all empty() calls with explicit === '' / !== null checks - Add is_array() guard in ConfigHelper::execConfig() traversal - Handle preg_replace() null return in helpers - Fix setCallbackContentMessage() using isset() instead of truthy check - Make ConfigHelper::config readonly and private - Add return type to tgn_view() Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add proper array type hints to interfaces and trait implementations - Remove dead catch clauses in ConfigHelper (Throwable covers all) - Remove unnecessary nullsafe operator in ActionEventTrait - Guard nullable array key in EventTrait with fallback - Fix config paths to match actual JSON file locations - Remove unused imports Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Move DEFAULT_PLATFORM, WEBHOOK_EVENT_HEADER, event separators, and PLATFORM_EVENT_SEPARATOR into Platform enum methods - Add Platform::DEFAULT const for use in default parameter values - Remove platformEnum() static helper (use Platform::from() directly) - EventConstant now only contains callback serialization constants - SettingConstant uses hardcoded separator strings (const context) - All 16 tests passing Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This pull request introduces several significant improvements and refactorings across the codebase, focusing on configuration management, platform abstraction, and type safety. The most important changes include the introduction of new DTOs and Enums for platform and chat targeting, improved config and settings file handling with in-memory caching, and enhanced method signatures for better nullability and type consistency.
Platform Abstraction and Type Safety:
Platformenum insrc/Enums/Platform.phpto centralize platform-specific logic, such as event separators and webhook headers. This is now used throughout the codebase for better type safety and maintainability. [1] [2] [3] [4]EventConstantand related logic to leverage the newPlatformenum, replacing string-based platform handling. [1] [2]Configuration and Settings Management:
EventandSettingmodels to use in-memory caching for JSON config files, reducing unnecessary disk reads and improving performance. Added explicitreloadmethods and adirtyflag to manage persistence. [1] [2] [3] [4]config/tg-notifier.phpto use new locations underconfig/jsons/, improving organization and clarity.DTOs and Data Handling:
ChatTargetDTO insrc/DTOs/ChatTarget.phpto encapsulate chat and thread targeting logic, including parsing from string formats.Interface and Method Signature Improvements:
AppInterface,NotificationInterface, and various constructors). [1] [2] [3] [4] [5] [6] [7]Config Helper and Template Rendering:
ConfigHelper's template rendering to use output buffering within a closure for better error handling and variable isolation. Improved documentation and error propagation for template failures. [1] [2]These changes collectively improve the maintainability, reliability, and clarity of the codebase, especially around configuration, platform abstraction, and type safety.