Allow stringable object to be used to geocode address#382
Allow stringable object to be used to geocode address#382norkunas merged 1 commit intogeocoder-php:masterfrom
Conversation
|
Warning Rate limit exceeded@norkunas has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 0 minutes and 19 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
WalkthroughGeocoding now accepts objects implementing Stringable: addresses from getters or embedded fields are cast to string before building GeocodeQuery. Tests and fixtures for a Stringable address type were added. Minor class-name formatting change in AttributeDriver error message. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 0
🧹 Nitpick comments (3)
src/Doctrine/ORM/GeocoderListener.php (1)
95-99: LGTM! Consider edge case with empty Stringable result.The type check and string cast correctly enable Stringable support. Note that
empty($address)won't catch a Stringable object whose__toString()returns an empty string (since objects are truthy). If this edge case matters, you could cast before the empty check:🔎 Optional refinement
- if (empty($address) || (!is_string($address) && !$address instanceof \Stringable)) { + if (!is_string($address) && !$address instanceof \Stringable) { + return; + } + + $addressString = (string) $address; + if ('' === $addressString) { return; } - $results = $this->geocoder->geocodeQuery(GeocodeQuery::create((string) $address)); + $results = $this->geocoder->geocodeQuery(GeocodeQuery::create($addressString));tests/Functional/Fixtures/Entity/DummyWithStringableGetter.php (2)
43-56: Duplicate#[Address]attribute on property and getter.The
#[Address]attribute is applied to both the$addressproperty (line 44) and thegetAddress()method (line 52). This differs from the pattern inDummyWithAddressGetter.phpwhere#[Address]is only on the getter. The duplicate attribute may be redundant or could cause unexpected behavior in the metadata driver.Consider removing
#[Address]from the property since the getter is the intended source:🔎 Proposed fix
#[Embedded] - #[Address] public StringableAddress $address;
30-41: Consider adding type hints for properties.For consistency and improved static analysis, consider adding explicit type hints to
$latitudeand$longitudeproperties (e.g.,?float).🔎 Optional improvement
#[Column] #[Latitude] - public $latitude; + public ?float $latitude = null; #[Column] #[Longitude] - public $longitude; + public ?float $longitude = null;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Doctrine/ORM/GeocoderListener.phptests/Functional/Fixtures/Entity/DummyWithStringableGetter.phptests/Functional/Fixtures/Entity/StringableAddress.phptests/Functional/GeocoderListenerTest.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/Doctrine/ORM/GeocoderListener.php (1)
tests/Mapping/Driver/Fixtures/DummyWithAddressGetter.php (1)
Geocodeable(18-26)
tests/Functional/GeocoderListenerTest.php (1)
tests/Functional/Fixtures/Entity/DummyWithStringableGetter.php (2)
Entity(26-57)setAddress(47-50)
🔇 Additional comments (3)
tests/Functional/Fixtures/Entity/StringableAddress.php (1)
18-31: LGTM!Clean implementation of a Stringable value object. The use of constructor property promotion with Doctrine attributes and proper implementation of the
\Stringableinterface follows PHP 8 best practices.tests/Functional/GeocoderListenerTest.php (2)
182-211: LGTM!The test correctly validates the Stringable address path. It follows the same structure as existing tests and properly verifies that geocoding occurs for entities with Stringable address getters.
20-21: Imports are correctly added.The new fixture imports align with the test additions.
norkunas
force-pushed
the
stringable-getter
branch
2 times, most recently
from
4dfb6c0 to
d999bfd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/Functional/GeocoderListenerTest.php (1)
182-211: Consider adding update scenario for test consistency.The test correctly validates that stringable addresses can be geocoded. However, both
testPersistForPropertyandtestPersistForGetterinclude an additional check: they clone the entity, change the address, flush again, and assert that the coordinates have changed. This verifies that address changes trigger re-geocoding.For consistency and complete coverage, consider adding the same update scenario to this test.
🔎 Suggested addition for update scenario
$em->persist($dummy); $em->flush(); self::assertNotNull($dummy->latitude); self::assertNotNull($dummy->longitude); + + $clone = clone $dummy; + $dummy->setAddress(new StringableAddress('Paris, France')); + + $em->persist($dummy); + $em->flush(); + + self::assertNotEquals($clone->latitude, $dummy->latitude); + self::assertNotEquals($clone->longitude, $dummy->longitude); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Doctrine/ORM/GeocoderListener.phpsrc/Mapping/Driver/AttributeDriver.phptests/Functional/Fixtures/Entity/DummyWithStringableGetter.phptests/Functional/Fixtures/Entity/StringableAddress.phptests/Functional/GeocoderListenerTest.php
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Doctrine/ORM/GeocoderListener.php
- tests/Functional/Fixtures/Entity/DummyWithStringableGetter.php
- tests/Functional/Fixtures/Entity/StringableAddress.php
🧰 Additional context used
🧬 Code graph analysis (2)
tests/Functional/GeocoderListenerTest.php (1)
tests/Functional/Fixtures/Entity/DummyWithStringableGetter.php (2)
Entity(26-56)setAddress(46-49)
src/Mapping/Driver/AttributeDriver.php (1)
src/Mapping/Exception/MappingException.php (1)
MappingException(18-20)
🔇 Additional comments (2)
src/Mapping/Driver/AttributeDriver.php (1)
43-43: LGTM! Modern PHP syntax for class name resolution.The change from
get_class($object)to$object::classis a good modernization. This syntax is more concise and consistent with the changes elsewhere in the codebase.tests/Functional/GeocoderListenerTest.php (1)
20-21: LGTM! Imports are correct and well-organized.The new imports for the stringable address test fixtures are properly ordered and follow the existing conventions.
norkunas
force-pushed
the
stringable-getter
branch
from
d999bfd to
f3f2931
Compare
1 participant
Closes #351
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.