Conversation
… 8.1+ compatibility - Protect json_decode() from null json parameter (default to '') - Protect round() from null num parameter (default to 0) - Protect explode() from null string parameter (default to '') - Affected: Helper, Api, GoogleDriveQuota, Policy, Nova resources, Models - Behavior unchanged: null treated as appropriate default value per function - All tests passing
…precation Fix: add null coalescing to json_decode, round, explode calls for PHP…
There was a problem hiding this comment.
Pull request overview
This PR adds null coalescing operators (??) throughout the codebase to prevent potential null pointer errors and improve defensive programming practices. The changes also include removal of some PHPDoc @return annotations.
Key changes:
- Added null coalescing operators to
json_decode(),explode(), and arithmetic operations across multiple files - Removed
@returndocumentation tags from several methods in the Policy helper class - Applied defensive null checks to prevent errors when processing user input and external data
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| app/Nova/TindakLanjut.php | Added null coalescing to penanggung_jawab request parameter before json_decode |
| app/Nova/RapatInternal.php | Added null coalescing to peserta request parameter before json_decode |
| app/Models/ShareLink.php | Added null coalescing to search parameter before explode |
| app/Models/Dipa.php | Added null coalescing to search parameter before explode |
| app/Helpers/Policy.php | Removed @return documentation tags and added null coalescing to explode calls in allowedFor and notAllowedFor methods |
| app/Helpers/Helper.php | Added null coalescing to various operations including base64_decode, explode, and arithmetic calculations |
| app/Helpers/GoogleDriveQuota.php | Added redundant null coalescing to limit and usage variables in quota calculations |
| app/Helpers/Api.php | Added null coalescing to composer command output before json_decode |
| app/Console/Commands/SimpedeRestore.php | Added null coalescing to bytes variable in formatBytes method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public function notAllowedFor(string $roles = 'all'): self | ||
| { | ||
| $this->allowed = $roles !== 'all' && self::hasAccess(array_diff(array_keys(Helper::ROLE), explode(',', $roles)), session('role')); | ||
| $this->allowed = $roles !== 'all' && self::hasAccess(array_diff(array_keys(Helper::ROLE), explode(',', $roles ?? '')), session('role')); |
There was a problem hiding this comment.
The null coalescing operator is unnecessary here. The function parameter $roles already has a default value of 'all' defined in the method signature. If a value is passed, it will be a string, and if not passed, it defaults to 'all'. Adding ?? '' suggests $roles could be null, but the default parameter ensures it won't be null.
| ...json_decode( | ||
| base64_decode($queries[$filterUri], true), | ||
| base64_decode($queries[$filterUri], true) ?? '', | ||
| true |
There was a problem hiding this comment.
Incorrect null coalescing placement. The null coalescing operator should be applied to the result of base64_decode before passing it to json_decode, but the current placement wraps the entire base64_decode call. Since base64_decode can return false on failure (when the second parameter is true), the operator should check that result. However, passing an empty string to json_decode will cause json_decode to fail and return null, which will then cause array_merge to fail with a non-array argument. Consider checking if base64_decode returns false and handling that case appropriately, or removing the strict parameter from base64_decode.
| ...json_decode( | ||
| base64_decode($filter, true), | ||
| base64_decode($filter, true) ?? '', | ||
| true |
There was a problem hiding this comment.
Incorrect null coalescing placement. The null coalescing operator should be applied to the result of base64_decode before passing it to json_decode, but the current placement wraps the entire base64_decode call. Since base64_decode can return false on failure (when the second parameter is true), the operator should check that result. However, passing an empty string to json_decode will cause json_decode to fail and return null, which will then cause array_merge to fail with a non-array argument. Consider checking if base64_decode returns false and handling that case appropriately, or removing the strict parameter from base64_decode.
| @@ -1477,7 +1477,7 @@ public static function formatMitra($mitra) | |||
| $item['jabatan'] = 'Mitra Statistik'; | |||
| $item['volume'] = $item['volume_realisasi']; | |||
| $item['bruto'] = $item['volume_realisasi'] * $item['harga_satuan']; | |||
There was a problem hiding this comment.
Inconsistent null handling between bruto and pajak calculations. Line 1479 calculates bruto using $item['volume_realisasi'] and $item['harga_satuan'] without null coalescing, but line 1480 adds null coalescing for the same values in the pajak calculation. This inconsistency means that if these values are null, bruto will fail or produce unexpected results while pajak will default to 0. Either both calculations should use null coalescing, or neither should (depending on whether nulls are expected in the data).
| $item['bruto'] = $item['volume_realisasi'] * $item['harga_satuan']; | |
| $item['bruto'] = ($item['volume_realisasi'] ?? 0) * ($item['harga_satuan'] ?? 0); |
| @@ -1580,7 +1580,7 @@ public static function formatPegawai($pegawai, $tanggal_spj) | |||
| $item['kode_bank_id'] = optional($pegawai)->kode_bank_id; | |||
| $item['golongan'] = optional(self::getDataPegawaiByUserId($item['user_id'], $tanggal_spj))->golongan; | |||
| $item['bruto'] = $item['volume'] * $item['harga_satuan']; | |||
There was a problem hiding this comment.
Inconsistent null handling between bruto and pajak calculations. Line 1582 calculates bruto using $item['volume'] and $item['harga_satuan'] without null coalescing, but line 1583 adds null coalescing for the same values in the pajak calculation. This inconsistency means that if these values are null, bruto will fail or produce unexpected results while pajak will default to 0. Either both calculations should use null coalescing, or neither should (depending on whether nulls are expected in the data).
| $item['bruto'] = $item['volume'] * $item['harga_satuan']; | |
| $item['bruto'] = ($item['volume'] ?? 0) * ($item['harga_satuan'] ?? 0); |
| $total = $limit ? round(($limit ?? 0) / (1024 ** 3), 2) : 0; | ||
| $used = $usage ? round(($usage ?? 0) / (1024 ** 3), 2) : 0; |
There was a problem hiding this comment.
Redundant null coalescing operators. The variables $limit and $usage are already defaulted to 0 on lines 32-33 if they don't exist in the array. Adding ?? 0 here is unnecessary since these variables will always have a value (either from the array or the default 0). This makes the null coalescing check redundant and adds unnecessary complexity.
| $total = $limit ? round(($limit ?? 0) / (1024 ** 3), 2) : 0; | |
| $used = $usage ? round(($usage ?? 0) / (1024 ** 3), 2) : 0; | |
| $total = $limit ? round($limit / (1024 ** 3), 2) : 0; | |
| $used = $usage ? round($usage / (1024 ** 3), 2) : 0; |
| public function allowedFor(string $roles = 'all'): self | ||
| { | ||
| $this->allowed = $roles === 'all' || self::hasAccess(explode(',', $roles), session('role')); | ||
| $this->allowed = $roles === 'all' || self::hasAccess(explode(',', $roles ?? ''), session('role')); |
There was a problem hiding this comment.
The null coalescing operator is unnecessary here. The function parameters $roles already have default values of 'all' defined in the method signature. If a value is passed, it will be a string, and if not passed, it defaults to 'all'. Adding ?? '' suggests $roles could be null, but the default parameter ensures it won't be null.
1 participant
No description provided.