fix(voice-channel): improve channel deletion and state handling logic#212
Closed
fix(voice-channel): improve channel deletion and state handling logic#212
Conversation
📝 WalkthroughWalkthroughThis pull request refactors voice channel caching across multiple modules to store channel data as plain associative arrays instead of VoiceChannelDTO objects in cache. New helper methods are introduced to normalize and convert between arrays and DTOs during cache operations. The VoiceChannelDTO factory method is updated to properly handle both CarbonInterface instances and string timestamps for the lastJoinedAt field. DeleteVoiceChannelAction and HandleStateChannelAction are modified to implement DTO-driven cache loading and saving. Tests are updated to reconstruct DTOs from cached array entries. An environment variable CACHE_STORE=redis is added to .env.example. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 5
🧹 Nitpick comments (3)
app-modules/bot-discord/src/Actions/VoiceChannel/DeleteVoiceChannelAction.php (2)
78-114: ⚡ Quick win
delete()does double work and double-normalization.
normalizeChannel()is invoked twice per remaining entry (once inarray_filter, once inarray_map), and the inline mapping repeatsdtosToArrays(). If you keep cache writes insidedelete(), simplify to a single pass; otherwise prefer moving the cache write to the caller (see theexecute()comment).♻️ Single-pass version
- $filtered = array_values(array_filter($channels, function ($channel) use ($channelId): bool { - $dto = $this->normalizeChannel($channel); - - return !$dto instanceof VoiceChannelDTO || $dto->channelId !== $channelId; - })); - - $filteredArrays = array_map(function ($channel): ?array { - $dto = $this->normalizeChannel($channel); - - if (!$dto instanceof VoiceChannelDTO) { - return null; - } - - return [ - 'guildId' => $dto->guildId, - 'channelId' => $dto->channelId, - 'ownerId' => $dto->ownerId, - 'usersCount' => $dto->usersCount, - 'users' => $dto->users, - 'lastJoinedAt' => $dto->lastJoinedAt?->toIso8601String(), - ]; - }, $filtered); - $filteredArrays = array_filter($filteredArrays); - $filteredArrays = array_values($filteredArrays); - - cache()->tags(['voice_channels'])->put('active_voice_channels_keys', $filteredArrays); + $remaining = []; + foreach ($channels as $channel) { + $dto = $this->normalizeChannel($channel); + if (!$dto instanceof VoiceChannelDTO || $dto->channelId === $channelId) { + continue; + } + $remaining[] = $dto->toArray(); + } + + cache()->tags(['voice_channels'])->put('active_voice_channels_keys', $remaining);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app-modules/bot-discord/src/Actions/VoiceChannel/DeleteVoiceChannelAction.php` around lines 78 - 114, The delete() method currently calls normalizeChannel() twice per item and reimplements DTO-to-array mapping; fix by doing a single pass: iterate the cached 'active_voice_channels_keys' once, call normalizeChannel($channel) only once per item, skip the entry whose VoiceChannelDTO->channelId matches $channelId, collect remaining VoiceChannelDTOs (or their array forms) and then write the filtered result back to cache; alternatively, remove cache write from delete() and have execute() handle cache persistence (use the existing dtosToArrays() helper if available to avoid reimplementing the array mapping).
45-55: ⚡ Quick winDe-duplicate with a DTO
toArray()helper.
dtosToArrays(), the inline mapper indelete()(lines 94–109),HandleStateChannelAction::saveChannels(), andDynamicVoiceCommand::dtoToArray()all repeat the same DTO → array shape. If aVoiceChannelDTO::toArray()is added (see the comment on the DTO file), this becomes:- return array_map(fn (VoiceChannelDTO $dto) => [ - 'guildId' => $dto->guildId, - 'channelId' => $dto->channelId, - 'ownerId' => $dto->ownerId, - 'usersCount' => $dto->usersCount, - 'users' => $dto->users, - 'lastJoinedAt' => $dto->lastJoinedAt?->toIso8601String(), - ], $dtos); + return array_map(fn (VoiceChannelDTO $dto) => $dto->toArray(), $dtos);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app-modules/bot-discord/src/Actions/VoiceChannel/DeleteVoiceChannelAction.php` around lines 45 - 55, Add a VoiceChannelDTO::toArray() method that returns the shared array shape (guildId, channelId, ownerId, usersCount, users, lastJoinedAt => nullable ISO8601 string) and replace the local mappers: remove DeleteVoiceChannelAction::dtosToArrays(), the inline mapper in DeleteVoiceChannelAction::delete(), HandleStateChannelAction::saveChannels() mapping, and DynamicVoiceCommand::dtoToArray() to call $dto->toArray() instead so all conversions use the single DTO::toArray() implementation and preserve the lastJoinedAt?->toIso8601String() behavior.app-modules/bot-discord/src/Actions/VoiceChannel/HandleStateChannelAction.php (1)
126-138: ⚡ Quick winReplace inline mapping with
VoiceChannelDTO::toArray().Same array shape repeats in
DynamicVoiceCommandandDeleteVoiceChannelAction. OnceVoiceChannelDTO::toArray()is added, this becomes a one-liner and the four locations stay in sync automatically.- $arrays = array_map(fn (VoiceChannelDTO $dto) => [ - 'guildId' => $dto->guildId, - 'channelId' => $dto->channelId, - 'ownerId' => $dto->ownerId, - 'usersCount' => $dto->usersCount, - 'users' => $dto->users, - 'lastJoinedAt' => $dto->lastJoinedAt?->toIso8601String(), - ], $channels); + $arrays = array_map(fn (VoiceChannelDTO $dto) => $dto->toArray(), $channels);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app-modules/bot-discord/src/Actions/VoiceChannel/HandleStateChannelAction.php` around lines 126 - 138, The repeated inline mapping in saveChannels should be replaced by a single DTO serializer: add a public VoiceChannelDTO::toArray() method that returns the array with keys guildId, channelId, ownerId, usersCount, users, lastJoinedAt (ensure lastJoinedAt is converted to ISO8601 or null), then change saveChannels to map each VoiceChannelDTO to its toArray() (e.g., array_map(fn(VoiceChannelDTO $dto) => $dto->toArray(), $channels)) and update the other duplicate sites (DynamicVoiceCommand and DeleteVoiceChannelAction) to use VoiceChannelDTO::toArray() as well so the shape is single-sourced.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app-modules/bot-discord/src/Actions/VoiceChannel/DeleteVoiceChannelAction.php`:
- Around line 13-39: The loop in execute() can re-add channels deleted by
delete() because validChannels is populated before calling delete() and delete()
also mutates the cache; fix by removing cache read/write from
DeleteVoiceChannelAction::delete() (delete() should only remove the Discord
channel) and ensure execute() does a single cache write at the end using
cache()->tags(['voice_channels'])->put('active_voice_channels_keys',
$this->dtosToArrays($validChannels));; to avoid resurrecting just-deleted
channels either (A) move the $validChannels[] = $dto; so it happens only when
you do NOT call delete(), or (B) if you prefer current order, have delete()
return a boolean and skip adding to $validChannels when delete() removed
it—apply this change around normalizeChannel, VoiceChannelDTO checks, delete(),
and dtosToArrays usage so only one final cache write occurs.
In
`@app-modules/bot-discord/src/Actions/VoiceChannel/HandleStateChannelAction.php`:
- Around line 103-121: loadChannels() currently maps entries to VoiceChannelDTO
but returns nulls for malformed entries and ignores legacy stdClass data,
causing TypeErrors downstream in LeftChannelAction/JoiningChannelAction and
saveChannels(); update loadChannels() to reuse
DeleteVoiceChannelAction::normalizeChannel() (or its normalization logic) to
accept arrays and stdClass, convert/normalize each cached entry, filter out any
null/invalid results, and return only concrete VoiceChannelDTO instances so
callers like LeftChannelAction, JoiningChannelAction, and saveChannels() always
receive an array<VoiceChannelDTO>.
In `@app-modules/bot-discord/src/DTO/VoiceChannelDTO.php`:
- Around line 27-34: The code reads $data['lastJoinedAt'] directly which throws
when the key is missing; change the access to use a null-coalesced local value
(e.g., $value = $data['lastJoinedAt'] ?? null) and then call filled($value) and
the subsequent checks against CarbonInterface and is_string on that local $value
before assigning $lastJoinedAt and parsing with Date::parse; update the block
around $lastJoinedAt so it never indexes $data directly without the ?? guard.
- Around line 25-44: Add a toArray(): array method to VoiceChannelDTO to mirror
make(), returning the same keys (guildId, channelId, ownerId, usersCount, users,
lastJoinedAt) and serializing lastJoinedAt as an ISO8601 string or null (e.g.,
$this->lastJoinedAt?->toIso8601String()). Keep the field names and formats
identical to make() expectations, then replace the duplicated serialization
logic in DynamicVoiceCommand::dtoToArray,
HandleStateChannelAction::saveChannels, and
DeleteVoiceChannelAction::dtosToArrays/delete to call VoiceChannelDTO->toArray()
instead.
In `@app-modules/bot-discord/src/SlashCommands/DynamicVoiceCommand.php`:
- Around line 153-163: The dtoToArray method returns an untyped array shape
causing PHPStan failures and duplicates logic present in
HandleStateChannelAction::saveChannels and
DeleteVoiceChannelAction::dtosToArrays; replace usages of dtoToArray (e.g. where
channels are built at line ~84) with VoiceChannelDTO::toArray() and remove the
dtoToArray private method, or add a toArray() implementation on VoiceChannelDTO
and update callers to use VoiceChannelDTO::toArray() so array shapes are
centralized and PHPStan types are satisfied.
---
Nitpick comments:
In
`@app-modules/bot-discord/src/Actions/VoiceChannel/DeleteVoiceChannelAction.php`:
- Around line 78-114: The delete() method currently calls normalizeChannel()
twice per item and reimplements DTO-to-array mapping; fix by doing a single
pass: iterate the cached 'active_voice_channels_keys' once, call
normalizeChannel($channel) only once per item, skip the entry whose
VoiceChannelDTO->channelId matches $channelId, collect remaining
VoiceChannelDTOs (or their array forms) and then write the filtered result back
to cache; alternatively, remove cache write from delete() and have execute()
handle cache persistence (use the existing dtosToArrays() helper if available to
avoid reimplementing the array mapping).
- Around line 45-55: Add a VoiceChannelDTO::toArray() method that returns the
shared array shape (guildId, channelId, ownerId, usersCount, users, lastJoinedAt
=> nullable ISO8601 string) and replace the local mappers: remove
DeleteVoiceChannelAction::dtosToArrays(), the inline mapper in
DeleteVoiceChannelAction::delete(), HandleStateChannelAction::saveChannels()
mapping, and DynamicVoiceCommand::dtoToArray() to call $dto->toArray() instead
so all conversions use the single DTO::toArray() implementation and preserve the
lastJoinedAt?->toIso8601String() behavior.
In
`@app-modules/bot-discord/src/Actions/VoiceChannel/HandleStateChannelAction.php`:
- Around line 126-138: The repeated inline mapping in saveChannels should be
replaced by a single DTO serializer: add a public VoiceChannelDTO::toArray()
method that returns the array with keys guildId, channelId, ownerId, usersCount,
users, lastJoinedAt (ensure lastJoinedAt is converted to ISO8601 or null), then
change saveChannels to map each VoiceChannelDTO to its toArray() (e.g.,
array_map(fn(VoiceChannelDTO $dto) => $dto->toArray(), $channels)) and update
the other duplicate sites (DynamicVoiceCommand and DeleteVoiceChannelAction) to
use VoiceChannelDTO::toArray() as well so the shape is single-sourced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 6ab79b48-fed2-44e3-b13d-51f1642fa013
📒 Files selected for processing (6)
.env.exampleapp-modules/bot-discord/src/Actions/VoiceChannel/DeleteVoiceChannelAction.phpapp-modules/bot-discord/src/Actions/VoiceChannel/HandleStateChannelAction.phpapp-modules/bot-discord/src/DTO/VoiceChannelDTO.phpapp-modules/bot-discord/src/SlashCommands/DynamicVoiceCommand.phpapp-modules/bot-discord/tests/Feature/Actions/VoiceChannel/HandleStateChannelActionTest.php
Comment on lines
13
to
+39
| public function execute(Discord $discord): void | ||
| { | ||
| $channels = cache()->tags(['voice_channels'])->get('active_voice_channels_keys', []); | ||
| foreach ($channels as $index => $channel) { | ||
| /** @var VoiceChannelDTO $channel */ | ||
| if ($channel->isEmpty() && $channel->isLongTermEmpty()) { | ||
| $this->delete($channel->guildId, $channel->channelId, $index, $discord); | ||
|
|
||
| $validChannels = []; | ||
| $hasInvalid = false; | ||
|
|
||
| foreach ($channels as $channel) { | ||
| $dto = $this->normalizeChannel($channel); | ||
|
|
||
| if (!$dto instanceof VoiceChannelDTO) { | ||
| $hasInvalid = true; | ||
|
|
||
| continue; | ||
| } | ||
|
|
||
| $validChannels[] = $dto; | ||
|
|
||
| if ($dto->isEmpty() && $dto->isLongTermEmpty()) { | ||
| $this->delete($dto->guildId, $dto->channelId, $discord); | ||
| } | ||
| } | ||
|
|
||
| if ($hasInvalid) { | ||
| cache()->tags(['voice_channels'])->put('active_voice_channels_keys', $this->dtosToArrays($validChannels)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Cache write can resurrect just-deleted channels.
When $hasInvalid is true and at least one channel passes the isEmpty() && isLongTermEmpty() check:
delete()(line 32) reads the cache, filters out that channel, and writes back the filtered list.- After the loop, line 37 unconditionally writes
$validChannels— which was populated before the delete and still contains the deleted channel — overwriting step 1.
So channels that should have been removed will reappear in active_voice_channels_keys. The fix is to perform a single, post-loop write with the final desired state, and drop the cache mutation inside delete().
🐛 Proposed fix (single cache write, delete() only handles Discord)
public function execute(Discord $discord): void
{
$channels = cache()->tags(['voice_channels'])->get('active_voice_channels_keys', []);
- $validChannels = [];
- $hasInvalid = false;
+ $remainingChannels = [];
+ $mutated = false;
foreach ($channels as $channel) {
$dto = $this->normalizeChannel($channel);
if (!$dto instanceof VoiceChannelDTO) {
- $hasInvalid = true;
-
+ $mutated = true;
continue;
}
- $validChannels[] = $dto;
-
if ($dto->isEmpty() && $dto->isLongTermEmpty()) {
$this->delete($dto->guildId, $dto->channelId, $discord);
+ $mutated = true;
+ continue;
}
+
+ $remainingChannels[] = $dto;
}
- if ($hasInvalid) {
- cache()->tags(['voice_channels'])->put('active_voice_channels_keys', $this->dtosToArrays($validChannels));
+ if ($mutated) {
+ cache()->tags(['voice_channels'])->put('active_voice_channels_keys', $this->dtosToArrays($remainingChannels));
}
}…and have delete() only remove the Discord channel (drop its own cache read/write).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app-modules/bot-discord/src/Actions/VoiceChannel/DeleteVoiceChannelAction.php`
around lines 13 - 39, The loop in execute() can re-add channels deleted by
delete() because validChannels is populated before calling delete() and delete()
also mutates the cache; fix by removing cache read/write from
DeleteVoiceChannelAction::delete() (delete() should only remove the Discord
channel) and ensure execute() does a single cache write at the end using
cache()->tags(['voice_channels'])->put('active_voice_channels_keys',
$this->dtosToArrays($validChannels));; to avoid resurrecting just-deleted
channels either (A) move the $validChannels[] = $dto; so it happens only when
you do NOT call delete(), or (B) if you prefer current order, have delete()
return a boolean and skip adding to $validChannels when delete() removed
it—apply this change around normalizeChannel, VoiceChannelDTO checks, delete(),
and dtosToArrays usage so only one final cache write occurs.
Comment on lines
+103
to
+121
| /** | ||
| * @return array<VoiceChannelDTO> | ||
| */ | ||
| private function loadChannels(): array | ||
| { | ||
| $channels = cache()->tags(['voice_channels'])->get('active_voice_channels_keys', []); | ||
|
|
||
| return array_map(function ($channel): ?VoiceChannelDTO { | ||
| if ($channel instanceof VoiceChannelDTO) { | ||
| return $channel; | ||
| } | ||
|
|
||
| if (is_array($channel) && isset($channel['guildId'], $channel['channelId'], $channel['ownerId'])) { | ||
| return VoiceChannelDTO::make($channel); | ||
| } | ||
|
|
||
| return null; | ||
| }, $channels); | ||
| } |
There was a problem hiding this comment.
loadChannels() can return null entries and skips stdClass legacy data.
Two robustness gaps:
array_maphere returnsnullfor any malformed entry, but the result is typedarray<VoiceChannelDTO>and is fed straight intoLeftChannelAction/JoiningChannelActionandsaveChannels()— whose arrow function type-hintsVoiceChannelDTOand will throw aTypeErroron the firstnull.DeleteVoiceChannelAction::normalizeChannel()(lines 57–76) handlesstdClassto recover from the exact serialization issue this PR fixes, butloadChannels()only handlesarray. A pre-existing cache populated by the old code path (still present until it expires) would silently drop those entries here.
Both can be addressed by reusing the same normalization and filtering nulls.
🛡️ Proposed fix
/**
* `@return` array<VoiceChannelDTO>
*/
private function loadChannels(): array
{
$channels = cache()->tags(['voice_channels'])->get('active_voice_channels_keys', []);
- return array_map(function ($channel): ?VoiceChannelDTO {
+ $mapped = array_map(function ($channel): ?VoiceChannelDTO {
if ($channel instanceof VoiceChannelDTO) {
return $channel;
}
- if (is_array($channel) && isset($channel['guildId'], $channel['channelId'], $channel['ownerId'])) {
- return VoiceChannelDTO::make($channel);
+ $data = match (true) {
+ is_array($channel) => $channel,
+ $channel instanceof \stdClass => (array) $channel,
+ default => null,
+ };
+
+ if ($data !== null && isset($data['guildId'], $data['channelId'], $data['ownerId'])) {
+ return VoiceChannelDTO::make($data);
}
return null;
}, $channels);
+
+ return array_values(array_filter($mapped));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * @return array<VoiceChannelDTO> | |
| */ | |
| private function loadChannels(): array | |
| { | |
| $channels = cache()->tags(['voice_channels'])->get('active_voice_channels_keys', []); | |
| return array_map(function ($channel): ?VoiceChannelDTO { | |
| if ($channel instanceof VoiceChannelDTO) { | |
| return $channel; | |
| } | |
| if (is_array($channel) && isset($channel['guildId'], $channel['channelId'], $channel['ownerId'])) { | |
| return VoiceChannelDTO::make($channel); | |
| } | |
| return null; | |
| }, $channels); | |
| } | |
| /** | |
| * `@return` array<VoiceChannelDTO> | |
| */ | |
| private function loadChannels(): array | |
| { | |
| $channels = cache()->tags(['voice_channels'])->get('active_voice_channels_keys', []); | |
| $mapped = array_map(function ($channel): ?VoiceChannelDTO { | |
| if ($channel instanceof VoiceChannelDTO) { | |
| return $channel; | |
| } | |
| $data = match (true) { | |
| is_array($channel) => $channel, | |
| $channel instanceof \stdClass => (array) $channel, | |
| default => null, | |
| }; | |
| if ($data !== null && isset($data['guildId'], $data['channelId'], $data['ownerId'])) { | |
| return VoiceChannelDTO::make($data); | |
| } | |
| return null; | |
| }, $channels); | |
| return array_values(array_filter($mapped)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app-modules/bot-discord/src/Actions/VoiceChannel/HandleStateChannelAction.php`
around lines 103 - 121, loadChannels() currently maps entries to VoiceChannelDTO
but returns nulls for malformed entries and ignores legacy stdClass data,
causing TypeErrors downstream in LeftChannelAction/JoiningChannelAction and
saveChannels(); update loadChannels() to reuse
DeleteVoiceChannelAction::normalizeChannel() (or its normalization logic) to
accept arrays and stdClass, convert/normalize each cached entry, filter out any
null/invalid results, and return only concrete VoiceChannelDTO instances so
callers like LeftChannelAction, JoiningChannelAction, and saveChannels() always
receive an array<VoiceChannelDTO>.
Comment on lines
25
to
44
| public static function make(array $data): self | ||
| { | ||
| $lastJoinedAt = null; | ||
| if (filled($data['lastJoinedAt'])) { | ||
| if ($data['lastJoinedAt'] instanceof CarbonInterface) { | ||
| $lastJoinedAt = $data['lastJoinedAt']; | ||
| } elseif (is_string($data['lastJoinedAt'])) { | ||
| $lastJoinedAt = Date::parse($data['lastJoinedAt']); | ||
| } | ||
| } | ||
|
|
||
| return new self( | ||
| guildId: $data['guildId'], | ||
| channelId: $data['channelId'], | ||
| ownerId: $data['ownerId'], | ||
| usersCount: $data['usersCount'], | ||
| users: $data['users'], | ||
| lastJoinedAt: $data['lastJoinedAt'] ?? null, | ||
| lastJoinedAt: $lastJoinedAt, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Consider centralizing DTO ↔ array conversion here.
The reverse mapping (DTO → array) is now duplicated in three places: DynamicVoiceCommand::dtoToArray, HandleStateChannelAction::saveChannels, and DeleteVoiceChannelAction::dtosToArrays/delete. Adding a toArray(): array method on VoiceChannelDTO (paired with the existing make()) keeps the serialization contract in one place and prevents drift in field names/formats (e.g., lastJoinedAt?->toIso8601String()).
♻️ Proposed addition
public function isLongTermEmpty(): bool
{
return abs(now()->diffInSeconds($this->lastJoinedAt)) >= 20;
}
+
+ /**
+ * `@return` array<string, mixed>
+ */
+ public function toArray(): array
+ {
+ return [
+ 'guildId' => $this->guildId,
+ 'channelId' => $this->channelId,
+ 'ownerId' => $this->ownerId,
+ 'usersCount' => $this->usersCount,
+ 'users' => $this->users,
+ 'lastJoinedAt' => $this->lastJoinedAt?->toIso8601String(),
+ ];
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public static function make(array $data): self | |
| { | |
| $lastJoinedAt = null; | |
| if (filled($data['lastJoinedAt'])) { | |
| if ($data['lastJoinedAt'] instanceof CarbonInterface) { | |
| $lastJoinedAt = $data['lastJoinedAt']; | |
| } elseif (is_string($data['lastJoinedAt'])) { | |
| $lastJoinedAt = Date::parse($data['lastJoinedAt']); | |
| } | |
| } | |
| return new self( | |
| guildId: $data['guildId'], | |
| channelId: $data['channelId'], | |
| ownerId: $data['ownerId'], | |
| usersCount: $data['usersCount'], | |
| users: $data['users'], | |
| lastJoinedAt: $data['lastJoinedAt'] ?? null, | |
| lastJoinedAt: $lastJoinedAt, | |
| ); | |
| } | |
| public static function make(array $data): self | |
| { | |
| $lastJoinedAt = null; | |
| if (filled($data['lastJoinedAt'])) { | |
| if ($data['lastJoinedAt'] instanceof CarbonInterface) { | |
| $lastJoinedAt = $data['lastJoinedAt']; | |
| } elseif (is_string($data['lastJoinedAt'])) { | |
| $lastJoinedAt = Date::parse($data['lastJoinedAt']); | |
| } | |
| } | |
| return new self( | |
| guildId: $data['guildId'], | |
| channelId: $data['channelId'], | |
| ownerId: $data['ownerId'], | |
| usersCount: $data['usersCount'], | |
| users: $data['users'], | |
| lastJoinedAt: $lastJoinedAt, | |
| ); | |
| } | |
| /** | |
| * `@return` array<string, mixed> | |
| */ | |
| public function toArray(): array | |
| { | |
| return [ | |
| 'guildId' => $this->guildId, | |
| 'channelId' => $this->channelId, | |
| 'ownerId' => $this->ownerId, | |
| 'usersCount' => $this->usersCount, | |
| 'users' => $this->users, | |
| 'lastJoinedAt' => $this->lastJoinedAt?->toIso8601String(), | |
| ]; | |
| } |
🧰 Tools
🪛 GitHub Actions: Continuous Integration
[error] 28-28: Undefined array key "lastJoinedAt" in VoiceChannelDTO::make().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app-modules/bot-discord/src/DTO/VoiceChannelDTO.php` around lines 25 - 44,
Add a toArray(): array method to VoiceChannelDTO to mirror make(), returning the
same keys (guildId, channelId, ownerId, usersCount, users, lastJoinedAt) and
serializing lastJoinedAt as an ISO8601 string or null (e.g.,
$this->lastJoinedAt?->toIso8601String()). Keep the field names and formats
identical to make() expectations, then replace the duplicated serialization
logic in DynamicVoiceCommand::dtoToArray,
HandleStateChannelAction::saveChannels, and
DeleteVoiceChannelAction::dtosToArrays/delete to call VoiceChannelDTO->toArray()
instead.
Comment on lines
+27
to
+34
| $lastJoinedAt = null; | ||
| if (filled($data['lastJoinedAt'])) { | ||
| if ($data['lastJoinedAt'] instanceof CarbonInterface) { | ||
| $lastJoinedAt = $data['lastJoinedAt']; | ||
| } elseif (is_string($data['lastJoinedAt'])) { | ||
| $lastJoinedAt = Date::parse($data['lastJoinedAt']); | ||
| } | ||
| } |
There was a problem hiding this comment.
Guard against undefined lastJoinedAt key (pipeline failure).
CI is failing with Undefined array key "lastJoinedAt" because filled($data['lastJoinedAt']) reads the key directly. Several callers (e.g., the test beforeEach that builds entries without lastJoinedAt, lines 14–27 of HandleStateChannelActionTest.php) omit it. Use null‑coalescing on the access.
🐛 Proposed fix
- $lastJoinedAt = null;
- if (filled($data['lastJoinedAt'])) {
- if ($data['lastJoinedAt'] instanceof CarbonInterface) {
- $lastJoinedAt = $data['lastJoinedAt'];
- } elseif (is_string($data['lastJoinedAt'])) {
- $lastJoinedAt = Date::parse($data['lastJoinedAt']);
- }
- }
+ $lastJoinedAt = null;
+ $rawLastJoinedAt = $data['lastJoinedAt'] ?? null;
+ if (filled($rawLastJoinedAt)) {
+ if ($rawLastJoinedAt instanceof CarbonInterface) {
+ $lastJoinedAt = $rawLastJoinedAt;
+ } elseif (is_string($rawLastJoinedAt)) {
+ $lastJoinedAt = Date::parse($rawLastJoinedAt);
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| $lastJoinedAt = null; | |
| if (filled($data['lastJoinedAt'])) { | |
| if ($data['lastJoinedAt'] instanceof CarbonInterface) { | |
| $lastJoinedAt = $data['lastJoinedAt']; | |
| } elseif (is_string($data['lastJoinedAt'])) { | |
| $lastJoinedAt = Date::parse($data['lastJoinedAt']); | |
| } | |
| } | |
| $lastJoinedAt = null; | |
| $rawLastJoinedAt = $data['lastJoinedAt'] ?? null; | |
| if (filled($rawLastJoinedAt)) { | |
| if ($rawLastJoinedAt instanceof CarbonInterface) { | |
| $lastJoinedAt = $rawLastJoinedAt; | |
| } elseif (is_string($rawLastJoinedAt)) { | |
| $lastJoinedAt = Date::parse($rawLastJoinedAt); | |
| } | |
| } |
🧰 Tools
🪛 GitHub Actions: Continuous Integration
[error] 28-28: Undefined array key "lastJoinedAt" in VoiceChannelDTO::make().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app-modules/bot-discord/src/DTO/VoiceChannelDTO.php` around lines 27 - 34,
The code reads $data['lastJoinedAt'] directly which throws when the key is
missing; change the access to use a null-coalesced local value (e.g., $value =
$data['lastJoinedAt'] ?? null) and then call filled($value) and the subsequent
checks against CarbonInterface and is_string on that local $value before
assigning $lastJoinedAt and parsing with Date::parse; update the block around
$lastJoinedAt so it never indexes $data directly without the ?? guard.
Comment on lines
+153
to
+163
| private function dtoToArray(VoiceChannelDTO $dto): array | ||
| { | ||
| return [ | ||
| 'guildId' => $dto->guildId, | ||
| 'channelId' => $dto->channelId, | ||
| 'ownerId' => $dto->ownerId, | ||
| 'usersCount' => $dto->usersCount, | ||
| 'users' => $dto->users, | ||
| 'lastJoinedAt' => $dto->lastJoinedAt?->toIso8601String(), | ||
| ]; | ||
| } |
There was a problem hiding this comment.
Add @return value type and reuse a DTO helper.
PHPStan is failing on this method because the array shape is unspecified. Beyond that, this duplicates the same conversion present in HandleStateChannelAction::saveChannels() and DeleteVoiceChannelAction::dtosToArrays(). Adding VoiceChannelDTO::toArray() (see the DTO file comment) lets you delete this helper entirely.
🛠️ Minimal fix for PHPStan
- private function dtoToArray(VoiceChannelDTO $dto): array
+ /**
+ * `@return` array<string, mixed>
+ */
+ private function dtoToArray(VoiceChannelDTO $dto): array
{
return [
'guildId' => $dto->guildId,
'channelId' => $dto->channelId,
'ownerId' => $dto->ownerId,
'usersCount' => $dto->usersCount,
'users' => $dto->users,
'lastJoinedAt' => $dto->lastJoinedAt?->toIso8601String(),
];
}Preferred: replace the call at line 84 with $channels[] = $channelDto->toArray(); and remove dtoToArray() once VoiceChannelDTO::toArray() exists.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private function dtoToArray(VoiceChannelDTO $dto): array | |
| { | |
| return [ | |
| 'guildId' => $dto->guildId, | |
| 'channelId' => $dto->channelId, | |
| 'ownerId' => $dto->ownerId, | |
| 'usersCount' => $dto->usersCount, | |
| 'users' => $dto->users, | |
| 'lastJoinedAt' => $dto->lastJoinedAt?->toIso8601String(), | |
| ]; | |
| } | |
| /** | |
| * `@return` array<string, mixed> | |
| */ | |
| private function dtoToArray(VoiceChannelDTO $dto): array | |
| { | |
| return [ | |
| 'guildId' => $dto->guildId, | |
| 'channelId' => $dto->channelId, | |
| 'ownerId' => $dto->ownerId, | |
| 'usersCount' => $dto->usersCount, | |
| 'users' => $dto->users, | |
| 'lastJoinedAt' => $dto->lastJoinedAt?->toIso8601String(), | |
| ]; | |
| } |
🧰 Tools
🪛 GitHub Check: Perform Phpstan Check / Run
[failure] 153-153:
Method He4rt\BotDiscord\SlashCommands\DynamicVoiceCommand::dtoToArray() return type has no value type specified in iterable type array.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app-modules/bot-discord/src/SlashCommands/DynamicVoiceCommand.php` around
lines 153 - 163, The dtoToArray method returns an untyped array shape causing
PHPStan failures and duplicates logic present in
HandleStateChannelAction::saveChannels and
DeleteVoiceChannelAction::dtosToArrays; replace usages of dtoToArray (e.g. where
channels are built at line ~84) with VoiceChannelDTO::toArray() and remove the
dtoToArray private method, or add a toArray() implementation on VoiceChannelDTO
and update callers to use VoiceChannelDTO::toArray() so array shapes are
centralized and PHPStan types are satisfied.
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
1 participant
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
Fixes voice channel deletion not working due to cache serialization issues and improves the overall voice channel state handling logic.
Changes
Root Cause
The database cache driver (and others) serialize PHP objects in a way that loses class metadata. When objects were retrieved from cache, they came back as __PHP_Incomplete_Class or stdClass, causing method calls to fail.
Testing
Description
The PR fixes voice channel deletion failures caused by cache serialization issues. When cache drivers (like databases) serialize PHP objects, class metadata is lost, causing method calls to fail. The solution stores VoiceChannel data as arrays instead of objects, ensuring data remains usable after deserialization and reconstruction through VoiceChannelDTO.
References
Dependencies & Requirements
CACHE_STORE=redisadded to.env.exampleto document requirement for cache tagging supportContributor Summary
Changes Summary
CACHE_STORE=redisenvironment variable documentationmake()factory to accept both CarbonInterface and ISO8601 string dates forlastJoinedAtloadChannels()andsaveChannels()methods to convert between cached arrays and VoiceChannelDTO objectsnormalizeChannel()to handle deserialized data; refactored deletion process to validate, normalize, and rebuild cache from DTOsdtoToArray()helper to store VoiceChannelDTO as arrays in cache