Replace json_encode with Json::encode - #621
foremtehan wants to merge 1 commit into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: hypervel/components/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
PR Summary by QodoRoute collection serialization through Eloquent's JSON encoder
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo
1. Standalone collections cannot serialize
|
| public function toJson(int $options = 0): string | ||
| { | ||
| return json_encode($this->jsonSerialize(), $options | JSON_THROW_ON_ERROR); | ||
| return Json::encode($this->jsonSerialize(), $options | JSON_THROW_ON_ERROR); |
There was a problem hiding this comment.
1. Standalone collections cannot serialize 🐞 Bug ≡ Correctness
EnumeratesValues::toJson() now calls Hypervel\Database\Eloquent\Casts\Json, although the collections package neither requires nor autoloads the database package. Calling toJson() in a standalone hypervel/collections installation therefore reaches an unavailable class and terminates with a class-not-found error.
Agent Prompt
## Issue description
Collection JSON serialization now depends on an Eloquent class from `hypervel/database`, which is not a dependency of the standalone collections package and itself depends on collections.
## Fix Focus Areas
- src/collections/src/Traits/EnumeratesValues.php[17-17]
- src/collections/src/Traits/EnumeratesValues.php[967-969]
## Recommended Fix
Remove the database `Json` import and restore direct `json_encode` with `JSON_THROW_ON_ERROR`, preserving collection serialization without introducing a circular package dependency.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| public function toJson(int $options = 0): string | ||
| { | ||
| return json_encode($this->jsonSerialize(), $options | JSON_THROW_ON_ERROR); | ||
| return Json::encode($this->jsonSerialize(), $options | JSON_THROW_ON_ERROR); |
There was a problem hiding this comment.
2. Model encoders corrupt collections 🐞 Bug ≡ Correctness
EnumeratesValues::toJson() now delegates to the Eloquent cast encoder, whose process-wide callback can return any mixed value instead of native JSON. When an application configures that supported callback for model attributes, every collection starts returning the callback's arbitrary output or throws a return-type error if it returns false.
Agent Prompt
## Issue description
Collection serialization now shares Eloquent's configurable global encoder, allowing model-specific encoding configuration to alter or break every collection's `toJson()` result.
## Fix Focus Areas
- src/collections/src/Traits/EnumeratesValues.php[17-17]
- src/collections/src/Traits/EnumeratesValues.php[967-969]
## Recommended Fix
Remove the Eloquent cast encoder from collection serialization and use native `json_encode` with the supplied options plus `JSON_THROW_ON_ERROR`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
| use Hypervel\Support\Collection; | ||
| use Hypervel\Support\Enumerable; | ||
| use Hypervel\Support\HigherOrderCollectionProxy; | ||
| use Hypervel\Database\Eloquent\Casts\Json; |
There was a problem hiding this comment.
Collections now require database
If hypervel/collections is installed without hypervel/database, the new Json call cannot load its class: collections does not require the database package, and the database package owns that class. Calling toJson(), toPrettyJson(), or casting a collection to a string now fails with a class-not-found error instead of producing JSON.
| public function toJson(int $options = 0): string | ||
| { | ||
| return json_encode($this->jsonSerialize(), $options | JSON_THROW_ON_ERROR); | ||
| return Json::encode($this->jsonSerialize(), $options | JSON_THROW_ON_ERROR); |
There was a problem hiding this comment.
Cast encoder can break collections
If an application configures Eloquent’s JSON cast encoder and it returns false to signal an encoding failure, this call passes that result into toJson(): string. Collection serialization then throws a TypeError instead of retaining its previous JSON-string or JsonException behavior. The configured encoder also applies to collections even though it was set for Eloquent casts.
|
@foremtehan Thanks for taking the time to submit this PR. Hypervel closely tracks Laravel’s public APIs and behavior. Laravel’s collections use Without a custom encoder, this adds a method call and callback check before calling the same native JSON encoder, with no benefit to the default behavior. For a general feature like configurable collection serialization, please propose it to Laravel first. If accepted upstream, we can consider porting it. Hypervel-specific improvements, such as coroutine safety and Swoole integration, remain welcome here. |
This is kind of poc, Is is make sense to use Json cast whenever
json_encodeused in codebase?