Auto register doctrine services, and support multiple metadata drivers #391
Conversation
|
Warning Rate limit exceeded@norkunas has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 14 minutes and 11 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 (23)
WalkthroughThis PR introduces attribute-based geocoding metadata extraction for PHP 8+, adds a ChainDriver to aggregate multiple metadata drivers, refactors the Doctrine ORM listener to use ServiceLocator for dynamic provider lookup, and updates Symfony DI configuration to wire these components. The Geocodeable attribute now requires an explicit provider parameter. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant ChainDriver as ChainDriver<br/>(DriverInterface)
participant AttributeDriver as AttributeDriver<br/>(DriverInterface)
participant Reflector as PHP Reflection
participant Metadata as ClassMetadata
Client->>ChainDriver: loadMetadataFromObject(object)
ChainDriver->>AttributeDriver: loadMetadataFromObject(object)
AttributeDriver->>Reflector: Scan class for Geocodeable attribute
alt Geocodeable found
Reflector-->>AttributeDriver: Geocodeable attribute + provider
AttributeDriver->>Reflector: Scan properties for Latitude/Longitude/Address
Reflector-->>AttributeDriver: Properties list
AttributeDriver->>Reflector: Scan public methods for Address attribute
Reflector-->>AttributeDriver: Methods list
AttributeDriver->>Metadata: new ClassMetadata(provider, ..., properties, methods)
Metadata-->>AttributeDriver: populated metadata
AttributeDriver-->>ChainDriver: ClassMetadata
ChainDriver-->>Client: ClassMetadata
else Geocodeable not found
AttributeDriver-->>ChainDriver: MappingException
ChainDriver->>ChainDriver: Try next driver
alt No drivers succeed
ChainDriver-->>Client: MappingException
end
end
sequenceDiagram
actor DoctrineORM as Doctrine ORM
participant Listener as GeocodeEntityListener
participant LocatorSvc as ServiceLocator<br/>(providers)
participant Driver as DriverInterface<br/>(ChainDriver)
participant Provider as Geocoder Provider
DoctrineORM->>Listener: onFlush(LifecycleEventArgs)
Listener->>Driver: loadMetadataFromObject(entity)
Driver-->>Listener: ClassMetadata (with provider name)
Listener->>LocatorSvc: get(providerId from metadata)
alt Provider exists
LocatorSvc-->>Listener: Provider instance
Listener->>Provider: geocodeQuery(address)
Provider-->>Listener: results
Listener->>Listener: Set entity latitude/longitude
else Provider not found
LocatorSvc-->>Listener: RuntimeException
Listener-->>DoctrineORM: RuntimeException
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 7
🧹 Nitpick comments (1)
src/DependencyInjection/BazingaGeocoderExtension.php (1)
58-64: LGTM! Proper conditional ORM loading with defensive checks.The conditional loading logic correctly verifies DoctrineBundle is available before loading ORM services and provides a clear error message if ORM is enabled without the required bundle.
Minor style refinement: Remove unnecessary leading backslash
The leading backslash in
\array_key_existsis unnecessary within a namespaced context:- if (\array_key_exists('DoctrineBundle', $container->getParameter('kernel.bundles'))) { + if (array_key_exists('DoctrineBundle', $container->getParameter('kernel.bundles'))) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
Mapping/Driver/AttributeDriver.phpResources/config/doctrine.ymlResources/config/services.ymlResources/doc/doctrine.mdconfig/orm.phpconfig/services.phpphpstan-baseline.phpsrc/DependencyInjection/BazingaGeocoderExtension.phpsrc/DependencyInjection/Configuration.phpsrc/Doctrine/ORM/GeocodeEntityListener.phpsrc/Mapping/Attributes/Geocodeable.phpsrc/Mapping/ClassMetadata.phpsrc/Mapping/Driver/AttributeDriver.phpsrc/Mapping/Driver/ChainDriver.phpsrc/Mapping/Driver/DriverInterface.phptests/Functional/Fixtures/Entity/DummyWithEmptyProperty.phptests/Functional/Fixtures/Entity/DummyWithGetter.phptests/Functional/Fixtures/Entity/DummyWithInvalidGetter.phptests/Functional/Fixtures/Entity/DummyWithProperty.phptests/Functional/Fixtures/Entity/DummyWithStringableGetter.phptests/Functional/GeocodeEntityListenerTest.phptests/Functional/config/listener.ymltests/Functional/config/listener_php8.ymltests/Mapping/Driver/AttributeDriverTest.phptests/Mapping/Driver/Fixtures/Dummy.phptests/Mapping/Driver/Fixtures/DummyWithAddressGetter.php
💤 Files with no reviewable changes (1)
- tests/Functional/config/listener_php8.yml
🧰 Additional context used
🧬 Code graph analysis (14)
src/Mapping/Attributes/Geocodeable.php (4)
src/DependencyInjection/Configuration.php (1)
__construct(24-27)src/Doctrine/ORM/GeocodeEntityListener.php (1)
__construct(33-37)src/Mapping/ClassMetadata.php (1)
__construct(23-30)src/Mapping/Driver/ChainDriver.php (1)
__construct(26-29)
tests/Functional/Fixtures/Entity/DummyWithStringableGetter.php (2)
tests/Mapping/Driver/Fixtures/Dummy.php (1)
Geocodeable(20-31)tests/Mapping/Driver/Fixtures/DummyWithAddressGetter.php (1)
Geocodeable(18-26)
src/Mapping/Driver/ChainDriver.php (5)
src/Mapping/ClassMetadata.php (2)
ClassMetadata(18-31)__construct(23-30)src/Mapping/Exception/MappingException.php (1)
MappingException(18-20)Mapping/Driver/AttributeDriver.php (2)
isGeocodeable(25-34)loadMetadataFromObject(39-82)src/Mapping/Driver/AttributeDriver.php (2)
isGeocodeable(26-31)loadMetadataFromObject(36-71)src/Mapping/Driver/DriverInterface.php (2)
isGeocodeable(20-20)loadMetadataFromObject(25-25)
tests/Mapping/Driver/Fixtures/Dummy.php (1)
tests/Mapping/Driver/Fixtures/DummyWithAddressGetter.php (1)
Geocodeable(18-26)
tests/Functional/Fixtures/Entity/DummyWithEmptyProperty.php (2)
tests/Mapping/Driver/Fixtures/Dummy.php (1)
Geocodeable(20-31)tests/Mapping/Driver/Fixtures/DummyWithAddressGetter.php (1)
Geocodeable(18-26)
src/Mapping/Driver/DriverInterface.php (4)
src/Mapping/Exception/MappingException.php (1)
MappingException(18-20)Mapping/Driver/AttributeDriver.php (1)
isGeocodeable(25-34)src/Mapping/Driver/AttributeDriver.php (1)
isGeocodeable(26-31)src/Mapping/Driver/ChainDriver.php (1)
isGeocodeable(31-40)
tests/Functional/Fixtures/Entity/DummyWithProperty.php (2)
tests/Mapping/Driver/Fixtures/Dummy.php (1)
Geocodeable(20-31)tests/Mapping/Driver/Fixtures/DummyWithAddressGetter.php (1)
Geocodeable(18-26)
tests/Functional/Fixtures/Entity/DummyWithGetter.php (2)
tests/Mapping/Driver/Fixtures/Dummy.php (1)
Geocodeable(20-31)tests/Mapping/Driver/Fixtures/DummyWithAddressGetter.php (1)
Geocodeable(18-26)
tests/Mapping/Driver/Fixtures/DummyWithAddressGetter.php (1)
tests/Mapping/Driver/Fixtures/Dummy.php (1)
Geocodeable(20-31)
tests/Functional/Fixtures/Entity/DummyWithInvalidGetter.php (2)
tests/Mapping/Driver/Fixtures/Dummy.php (1)
Geocodeable(20-31)tests/Mapping/Driver/Fixtures/DummyWithAddressGetter.php (1)
Geocodeable(18-26)
config/services.php (3)
Mapping/Driver/AttributeDriver.php (1)
AttributeDriver(23-83)src/Mapping/Driver/AttributeDriver.php (1)
AttributeDriver(24-97)src/Mapping/Driver/ChainDriver.php (1)
ChainDriver(21-54)
config/orm.php (1)
src/Doctrine/ORM/GeocodeEntityListener.php (1)
GeocodeEntityListener(27-123)
src/Doctrine/ORM/GeocodeEntityListener.php (3)
src/Mapping/Attributes/Geocodeable.php (1)
__construct(24-27)src/Mapping/ClassMetadata.php (1)
__construct(23-30)src/Mapping/Driver/ChainDriver.php (1)
__construct(26-29)
src/Mapping/ClassMetadata.php (2)
src/Doctrine/ORM/GeocodeEntityListener.php (1)
__construct(33-37)src/Mapping/Attributes/Geocodeable.php (1)
__construct(24-27)
🪛 LanguageTool
Resources/doc/doctrine.md
[style] ~8-~8: Often, this adverbial phrase is redundant. Consider using an alternative.
Context: ...is the feature you been always wanted. First of all, update your entity: ```php use Bazin...
(FIRST_OF_ALL)
🔇 Additional comments (25)
tests/Functional/Fixtures/Entity/DummyWithInvalidGetter.php (1)
26-26: LGTM! Consistent provider configuration.The addition of
provider: 'acme'to the Geocodeable attribute aligns with the PR's objective to make the provider parameter explicit and matches the configuration intests/Functional/config/listener.yml.tests/Mapping/Driver/Fixtures/DummyWithAddressGetter.php (1)
18-18: LGTM! Attribute update is consistent.The provider parameter addition is correct and consistent with the broader attribute-driven provider support introduced in this PR.
tests/Functional/config/listener.yml (2)
12-18: LGTM! Doctrine ORM mappings properly configured.The attribute-based mapping configuration correctly points to the test fixtures directory and sets appropriate prefix and alias values for the functional tests.
23-30: LGTM! ORM integration and provider properly configured.The configuration enables ORM integration and sets up the 'acme' provider with appropriate Nominatim options for functional testing.
tests/Mapping/Driver/Fixtures/Dummy.php (1)
20-20: LGTM! Consistent attribute update.The provider parameter addition is correct and aligns with the attribute-driven provider support.
tests/Mapping/Driver/AttributeDriverTest.php (2)
38-38: LGTM! Essential test coverage for provider extraction.The assertion correctly verifies that the AttributeDriver extracts the provider value from the Geocodeable attribute and populates the ClassMetadata accordingly.
51-51: LGTM! Comprehensive provider verification.The assertion ensures provider extraction works correctly for entities with address getters, complementing the property-based test coverage.
config/orm.php (1)
1-21: LGTM! Proper service configuration for ORM integration.The GeocodeEntityListener service is correctly configured with:
- A service locator for dynamic provider lookup via tagged providers
- A DriverInterface reference for metadata loading
- Proper Doctrine event listener registration for the onFlush event
This aligns with the PR's objective to support dynamic provider lookup using ServiceLocator.
tests/Functional/Fixtures/Entity/DummyWithStringableGetter.php (1)
27-27: LGTM! Final fixture updated consistently.The provider parameter addition completes the consistent update pattern across all test fixtures, ensuring the Stringable getter scenario is also covered with provider-aware metadata.
src/Mapping/Driver/AttributeDriver.php (1)
46-46: LGTM! Provider extraction is correctly implemented.The provider is properly extracted from the Geocodeable attribute instance and passed to ClassMetadata construction. The bounds check at line 42 ensures $attributes[0] exists before access.
tests/Functional/Fixtures/Entity/DummyWithEmptyProperty.php (1)
26-26: LGTM! Fixture updated for required provider parameter.The Geocodeable attribute now includes the provider parameter, aligning with the new attribute signature and test expectations.
tests/Functional/Fixtures/Entity/DummyWithGetter.php (1)
26-26: LGTM! Fixture updated consistently.The provider parameter addition is consistent with other test fixtures and supports the new provider-based geocoding metadata system.
phpstan-baseline.php (3)
132-137: PHPStan baseline entry added for array_key_exists.This entry suppresses a type checking issue where
array_key_existsreceives a parameter that PHPStan cannot prove is an array. Ensure this is intentional and the code properly validates the input at runtime.
160-160: Path references updated for renamed listener.The baseline correctly reflects the rename from GeocoderListener.php to GeocodeEntityListener.php.
Also applies to: 166-167
168-172: Remove this false positive PHPStan baseline entry.The
$providerparameter is correctly passed to theClassMetadataconstructor via named argument unpacking. Line 46 builds the$argsarray with'provider' => $attributes[0]->newInstance()->provider, and line 70 unpacks it withnew ClassMetadata(...$args). PHP 8.1 (the project's minimum version) fully supports spreading associative arrays with string keys as named arguments, so this pattern is valid and the parameter is not missing. This baseline entry should be removed.src/DependencyInjection/Configuration.php (1)
69-81: LGTM! ORM configuration node properly structured.The new
ormconfiguration node follows Symfony best practices with proper normalization of boolean shortcuts (true/false/null) and clear documentation. The default value offalseensures the ORM listener is opt-in.tests/Functional/Fixtures/Entity/DummyWithProperty.php (1)
26-26: LGTM! Fixture consistently updated.The provider parameter is added consistently with other test fixtures, supporting the provider-aware geocoding workflow.
tests/Functional/GeocodeEntityListenerTest.php (1)
38-38: LGTM! Test class renamed for consistency.The test class name now matches the renamed listener class (GeocodeEntityListener), improving code clarity and maintainability.
src/Mapping/Driver/DriverInterface.php (1)
16-16: LGTM! Exception contract properly documented.The
@throws MappingExceptionannotation correctly documents the exception contract forloadMetadataFromObject, aligning with implementations like AttributeDriver that throw this exception when metadata cannot be loaded.Also applies to: 22-24
src/Mapping/Driver/ChainDriver.php (1)
23-53: LGTM! Clean chain of responsibility implementation.The implementation correctly delegates to multiple drivers, short-circuits on success, and handles failures gracefully with proper exception propagation.
Resources/config/services.yml (1)
42-52: LGTM! Correct metadata driver wiring.The ChainDriver correctly aggregates tagged drivers without tagging itself, and the AttributeDriver is properly tagged. The alias from DriverInterface to ChainDriver enables dependency injection for the driver chain.
src/Doctrine/ORM/GeocodeEntityListener.php (2)
29-37: LGTM! Clean refactor to ServiceLocator for dynamic provider resolution.The constructor now accepts a
ServiceLocator<Provider>instead of a singleProvider, enabling dynamic provider lookup based on entity metadata. This aligns well with the provider-aware metadata model introduced in this PR.
79-111: LGTM! Proper validation and error handling for provider lookup.The method correctly builds a service ID from the metadata provider (line 98), validates that the provider exists in the ServiceLocator (lines 100-102), and provides a clear error message if the provider is invalid. The implementation is robust and maintainable.
src/Mapping/Attributes/Geocodeable.php (1)
21-27: All existing usages of theGeocodeableattribute have been properly updated with the requiredproviderparameter. No code remains that uses#[Geocodeable]without arguments.src/Mapping/ClassMetadata.php (1)
20-30: Breaking change: provider parameter now required.The constructor now requires
provideras the first parameter. This breaking change is properly implemented—all instantiations in the active codebase (src/Mapping/Driver/AttributeDriver.php) provide the provider parameter, and the readonly property with constructor promotion ensures type safety.Note: The old file at
Mapping/Driver/AttributeDriver.phpis dead code (PSR-4 autoloader only loads from src/) and should be removed.
norkunas
force-pushed
the
auto-orm-listener
branch
3 times, most recently
from
c8f64a4 to
bc48423
Compare
norkunas
force-pushed
the
auto-orm-listener
branch
from
bc48423 to
c8350b8
Compare
2 participants
Closes #277
Summary by CodeRabbit
Release Notes
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.