Support casting from WKT representation #149
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThe Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Cast/GeometryCast.php (1)
52-57: Excellent solution for distinguishing WKB from WKT formats.The
ctype_xdigit()approach is clever and should reliably distinguish between:
- WKB data (hexadecimal strings from database)
- WKT data (text format like "POINT(1 2)" after serialization)
This directly addresses the core issue where serialization clears the cast cache and WKT gets interpreted as WKB.
Consider adding a comment to document this detection logic:
+ // Detect format: WKB data is hex-encoded, WKT is human-readable text if (ctype_xdigit($value)) { $geometry = $this->wkbParser->parse($value); } else { $geometry = $this->wktParser->parse($value); }tests/Models/LocationTest.php (1)
68-86: Excellent test coverage for the serialization bug fix.This test directly validates the core issue described in the PR:
- Model serialization clears the cast cache
- Geometry attribute access should still work correctly
- Both in-memory and fresh retrieval scenarios are tested
The test effectively ensures that the conditional WKB/WKT parsing logic in
GeometryCasthandles the cache clearing scenario properly.Consider a minor optimization by combining model creation and saving:
- $location = Location::make([ + $location = Location::create([ 'name' => 'Test Location', 'location' => Point::makeGeodetic(51.087, 8.76), ]); - $location->save();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Cast/GeometryCast.php(3 hunks)tests/Models/LocationTest.php(1 hunks)
🧰 Additional context used
🪛 PHPMD (2.15.0)
src/Cast/GeometryCast.php
33-33: Avoid using static access to class '\Illuminate\Support\Facades\App' in method '__construct'. (Clean Code Rules)
(StaticAccess)
34-34: Avoid using static access to class '\Illuminate\Support\Facades\App' in method '__construct'. (Clean Code Rules)
(StaticAccess)
55-57: The method get uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them. (Clean Code Rules)
(ElseExpression)
🔇 Additional comments (3)
src/Cast/GeometryCast.php (3)
9-9: LGTM: WKTParser import added correctly.The import is properly placed and follows the existing pattern.
25-25: LGTM: WKTParser property declaration.Property is correctly typed and follows the existing pattern.
34-34: LGTM: WKTParser instantiation.Using
App::make()is the standard Laravel pattern for dependency injection in this context.
|
Will review this on the weekend :) thanks for your effort! |
saibotk
left a comment
saibotk
left a comment
There was a problem hiding this comment.
Thanks!
Amazing contribution and sorry for the delay!
2 participants
When an Eloquent model gets serialized, it clears the class cast cache, see here. This causes issues with how casting currently works in this library.
Here, a Geometry type attribute is cast from the model representation to the WKT representation, when set. But when fetching from the DB, it casts from WKB to the model representation.
When the class cast cache is populated, this is not a problem. After being set, the cached model representation will be fetched. However, without the cache, when fetching the attribute, it needs to be cast, which it can't because it expects WKB, but gets WKT, resulting in:
Bad endian byte value 83. at src/IO/Parser/WKB/WKBParser.php:125 121▕ 122▕ $byteOrder = match ($endianValue) { 123▕ 0 => ByteOrder::bigEndian, 124▕ 1 => ByteOrder::littleEndian, ➜ 125▕ default => throw new RuntimeException(sprintf('Bad endian byte value %s.', json_encode($endianValue))), 126▕ }; 127▕ 128▕ $this->scanner->setByteOrder($byteOrder); 129▕ } 1 src/IO/Parser/WKB/WKBParser.php:125 2 src/IO/Parser/WKB/WKBParser.php:38This situation presents itself when using queueable anonymous event listeners on the model.
This change fixes this issue.
In the meantime, it could be solved by doing
$model->refresh()to fetch the WKB representation from the database.Summary by CodeRabbit
New Features
Tests
Bug Fixes