feat: allow doctrine/doctrine-bundle 3 #384
Conversation
|
Warning Rate limit exceeded@Chris53897 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 1 minutes and 52 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 (1)
WalkthroughAdds explicit Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/Functional/GeocoderListenerTest.php (1)
74-90: Critical: Final test entities incompatible with Doctrine ORM 3.x lazy ghost proxies.Doctrine ORM 3.x generates lazy ghost proxies for lazy-loaded entities, which requires extending the entity class. Final classes cannot be extended, causing pipeline failures with errors like:
Cannot generate lazy ghost: class "Bazinga\GeocoderBundle\Tests\Functional\Fixtures\Entity\DummyWithStringableGetter" is finalTwo final entity classes in test fixtures are affected:
DummyWithStringableGetter(Entity)StringableAddress(Embeddable)Commenting out the
enable_lazy_ghost_objectsconfiguration does not resolve this incompatibility.Required fixes:
- Remove
finalkeyword fromDummyWithStringableGetterandStringableAddress- Or explicitly disable lazy loading for these entities in test configuration
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
composer.jsonsrc/DependencyInjection/BazingaGeocoderExtension.phpsrc/DependencyInjection/Compiler/AddProvidersPass.phpsrc/DependencyInjection/Compiler/FactoryValidatorPass.phpsrc/DependencyInjection/Compiler/ProfilerPass.phpsrc/Mapping/Driver/AttributeDriver.phptests/Functional/GeocoderListenerTest.phptests/Functional/config/listener.yml
🧰 Additional context used
🧬 Code graph analysis (4)
src/DependencyInjection/Compiler/FactoryValidatorPass.php (5)
src/DependencyInjection/Compiler/AddProvidersPass.php (1)
process(29-42)src/DependencyInjection/Compiler/ProfilerPass.php (1)
process(27-38)tests/Functional/GeocoderListenerTest.php (1)
process(63-68)tests/Functional/CustomTestKernel.php (1)
process(99-104)tests/DependencyInjection/Compiler/FactoryValidatorPassTest.php (5)
FactoryValidatorPassTest(20-87)testProcessDoesntThrowIfDefinitionExists(73-86)testProcessDoesntThrowIfAliasExists(58-71)testProcessThrows(40-56)tearDown(32-38)
src/DependencyInjection/Compiler/AddProvidersPass.php (2)
src/DependencyInjection/Compiler/FactoryValidatorPass.php (1)
process(31-38)src/DependencyInjection/Compiler/ProfilerPass.php (1)
process(27-38)
src/DependencyInjection/Compiler/ProfilerPass.php (4)
src/DependencyInjection/Compiler/AddProvidersPass.php (1)
process(29-42)tests/Functional/GeocoderListenerTest.php (1)
process(63-68)tests/DependencyInjection/Compiler/ProfilerPassTest.php (3)
ProfilerPassTest(23-47)setUp(27-30)testRegistersProviders(32-46)src/BazingaGeocoderBundle.php (1)
build(29-36)
src/Mapping/Driver/AttributeDriver.php (3)
src/Mapping/ClassMetadata.php (1)
ClassMetadata(18-27)tests/Mapping/Driver/AttributeDriverTest.php (1)
testLoadMetadataFromWrongObject(54-60)src/Mapping/Driver/DriverInterface.php (1)
loadMetadataFromObject(21-21)
🪛 GitHub Actions: CI
tests/Functional/GeocoderListenerTest.php
[error] 104-104: Step failed: ./vendor/bin/simple-phpunit -v. PHPUnit error: Cannot generate lazy ghost: class "Bazinga\GeocoderBundle\Tests\Functional\Fixtures\Entity\DummyWithStringableGetter" is final.
[error] 144-144: Step failed: ./vendor/bin/simple-phpunit -v. PHPUnit error: Cannot generate lazy ghost: class "Bazinga\GeocoderBundle\Tests\Functional\Fixtures\Entity\DummyWithStringableGetter" is final.
[error] 184-184: Step failed: ./vendor/bin/simple-phpunit -v. PHPUnit error: Cannot generate lazy ghost: class "Bazinga\GeocoderBundle\Tests\Functional\Fixtures\Entity\DummyWithStringableGetter" is final.
[error] 224-224: Step failed: ./vendor/bin/simple-phpunit -v. PHPUnit error: Cannot generate lazy ghost: class "Bazinga\GeocoderBundle\Tests\Functional\Fixtures\Entity\DummyWithStringableGetter" is final.
[error] 257-257: Step failed: ./vendor/bin/simple-phpunit -v. PHPUnit error: Cannot generate lazy ghost: class "Bazinga\GeocoderBundle\Tests\Functional\Fixtures\Entity\DummyWithStringableGetter" is final.
[error] 290-290: Step failed: ./vendor/bin/simple-phpunit -v. PHPUnit error: Cannot generate lazy ghost: class "Bazinga\GeocoderBundle\Tests\Functional\Fixtures\Entity\DummyWithStringableGetter" is final.
🔇 Additional comments (9)
src/DependencyInjection/BazingaGeocoderExtension.php (1)
45-45: LGTM - Explicit void return type improves type safety.The addition of the explicit
: voidreturn type aligns with modern PHP and Symfony best practices, providing compile-time type enforcement.src/DependencyInjection/Compiler/AddProvidersPass.php (1)
29-29: LGTM - Consistent with CompilerPassInterface expectations.The explicit void return type enhances type safety and aligns with Symfony's compiler pass patterns used throughout this PR.
src/DependencyInjection/Compiler/ProfilerPass.php (1)
27-27: LGTM - Explicit void return type added.Consistent with the type-safety improvements across other compiler passes in this PR.
src/DependencyInjection/Compiler/FactoryValidatorPass.php (1)
31-31: LGTM - Void return type matches pattern.The explicit return type improves type safety consistently with other compiler passes in this PR.
src/Mapping/Driver/AttributeDriver.php (3)
34-34: Good addition of ReflectionException to throws annotation.The
\ReflectionExceptionannotation is accurate sincenew \ReflectionClass()can throw this exception when the class doesn't exist or has invalid syntax.
79-80: Accurate exception annotation added.The
@throws \ReflectionExceptionannotation correctly documents that\ReflectionClassconstructor may throw this exception.
89-95: PHP 8.4 introduces native lazy objects that replace the need for DefaultProxyClassNameResolver.Up until PHP 8.4, Doctrine used code generation to implement lazy loading, by using a clever combination of engine level hacks and code generation. PHP 8.4 introduces lazy objects as a core language feature accessible via ReflectionClass::newLazyGhost(), which Doctrine ORM 3.4 leverages to create proxies with no code-generation anymore. The conditional logic is correct: for PHP versions below 8.4,
DefaultProxyClassNameResolverresolves the proxy class name from the generated proxy class system; for PHP 8.4+, the initializer function is called with the object as its first parameter and is expected to initialize the object state. This architectural shift allows the code to pass the object directly to ReflectionClass instead of routing through a proxy resolver.composer.json (1)
26-26: LGTM - Doctrine Bundle 3.x support added.The broadened constraint appropriately enables compatibility with both doctrine-bundle 2.18+ and 3.x. Both versions are stable and available on Packagist. The minimum version bump from 2.3 to 2.18 is prudent. Note that applications targeting 3.0 will require configuration updates, particularly around the 'enable_native_lazy_objects' setting which moves outside entity managers configuration.
tests/Functional/config/listener.yml (1)
9-11: The lazy ghost object configuration is intentionally disabled to maintain compatibility.The commented options in listener.yml and the disabled
enable_lazy_ghost_objectssetting are deliberate design choices, not workarounds for pipeline failures. The test suite intentionally avoids enabling lazy ghost objects (which don't support final classes), as evidenced by the conditional check in GeocoderListenerTest.php that explicitly does not enable this feature. The related deprecation warnings are already suppressed in baseline-ignore. Tests pass successfully across all CI environments without modifying entity declarations.
|
Im ok with these changes. I accidentally made some test entities as final so they are failing now :/ |
|
I removed the |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/Functional/GeocoderListenerTest.php (1)
69-94: Dynamic version-based configuration approach is sound.The overall approach of using
Composer\InstalledVersionsto dynamically configure ORM options based on installed package versions is appropriate for supporting multiple doctrine-bundle versions. The conditional logic forenable_native_lazy_objects(lines 83-89) demonstrates the correct pattern with proper version guards.Optional refactor: The
$ormVersionvariable is retrieved on line 71 but only used on line 86. Consider moving its declaration closer to its usage point for better readability:$orm = []; // doctrine-bundle if (null !== $doctrineBundleVersion = InstalledVersions::getVersion('doctrine/doctrine-bundle')) { // ... other checks ... if (\PHP_VERSION_ID >= 80400 && version_compare($doctrineBundleVersion, '2.15.0', '>=') && version_compare($doctrineBundleVersion, '3.1.0', '<') && version_compare($ormVersion = InstalledVersions::getVersion('doctrine/orm'), '3.4.0', '>=') ) { $orm['enable_native_lazy_objects'] = true; } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/Functional/GeocoderListenerTest.php
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Functional/GeocoderListenerTest.php (1)
src/DependencyInjection/Configuration.php (1)
Configuration(22-164)
🪛 GitHub Actions: CI
tests/Functional/GeocoderListenerTest.php
[error] 103-103: Unrecognized option "enable_lazy_ghost_objects" under "doctrine.orm.entity_managers.default". Available options are "auto_mapping", "class_metadata_factory_name", "connection", "default_repository_class", "dql", "entity_listener_resolver", "entity_listeners", "fetch_mode_subselect_batch_size", "filters", "hydrators", "identity_generation_preferences", "mappings", "metadata_cache_driver", "naming_strategy", "query_cache_driver", "quote_strategy", "repository_factory", "result_cache_driver", "schema_ignore_classes", "second_level_cache", "typed_field_mapper", "validate_xml_mapping".
[error] 143-143: Unrecognized option "enable_lazy_ghost_objects" under "doctrine.orm.entity_managers.default". Available options are "auto_mapping", "class_metadata_factory_name", "connection", "default_repository_class", "dql", "entity_listener_resolver", "entity_listeners", "fetch_mode_subselect_batch_size", "filters", "hydrators", "identity_generation_preferences", "mappings", "metadata_cache_driver", "naming_strategy", "query_cache_driver", "quote_strategy", "repository_factory", "result_cache_driver", "schema_ignore_classes", "second_level_cache", "typed_field_mapper", "validate_xml_mapping".
[error] 183-183: Unrecognized option "enable_lazy_ghost_objects" under "doctrine.orm.entity_managers.default". Available options are "auto_mapping", "class_metadata_factory_name", "connection", "default_repository_class", "dql", "entity_listener_resolver", "entity_listeners", "fetch_mode_subselect_batch_size", "filters", "hydrators", "identity_generation_preferences", "mappings", "metadata_cache_driver", "naming_strategy", "query_cache_driver", "quote_strategy", "repository_factory", "result_cache_driver", "schema_ignore_classes", "second_level_cache", "typed_field_mapper", "validate_xml_mapping".
[error] 223-223: Unrecognized option "enable_lazy_ghost_objects" under "doctrine.orm.entity_managers.default". Available options are "auto_mapping", "class_metadata_factory_name", "connection", "default_repository_class", "dql", "entity_listener_resolver", "entity_listeners", "fetch_mode_subselect_batch_size", "filters", "hydrators", "identity_generation_preferences", "mappings", "metadata_cache_driver", "naming_strategy", "query_cache_driver", "quote_strategy", "repository_factory", "result_cache_driver", "schema_ignore_classes", "second_level_cache", "typed_field_mapper", "validate_xml_mapping".
[error] 256-256: Unrecognized option "enable_lazy_ghost_objects" under "doctrine.orm.entity_managers.default". Available options are "auto_mapping", "class_metadata_factory_name", "connection", "default_repository_class", "dql", "entity_listener_resolver", "entity_listeners", "fetch_mode_subselect_batch_size", "filters", "hydrators", "identity_generation_preferences", "mappings", "metadata_cache_driver", "naming_strategy", "query_cache_driver", "quote_strategy", "repository_factory", "result_cache_driver", "schema_ignore_classes", "second_level_cache", "typed_field_mapper", "validate_xml_mapping".
🔇 Additional comments (2)
tests/Functional/GeocoderListenerTest.php (2)
22-22: LGTM!The
Composer\InstalledVersionsimport is appropriate for the dynamic version detection logic introduced in the test bootstrap.
80-82: Add explicit doctrine-bundle version guard forenable_lazy_ghost_objects.The
enable_lazy_ghost_objectsoption was introduced in doctrine-bundle 2.8.0. The current check only verifies method existence and Symfony >= 6.1, but doesn't ensure the installed doctrine-bundle version supports this configuration. Add a version constraint to prevent "unrecognized option" errors when testing with doctrine-bundle < 2.8.0:if (method_exists(Configuration::class, 'setLazyGhostObjectEnabled') && Kernel::VERSION_ID >= 60100 && version_compare($doctrineBundleVersion, '2.8.0', '>=')) { $orm['enable_lazy_ghost_objects'] = true; }
|
@norkunas Ready to review This could maybe need a closer look. I am pretty sure that changes in But is it hard to satisfy ci with all of its deprecations warnings and failing ci-runs. |
3 participants
An upgrade will update symfony/dependency-injection (v7.4.2 => v8.0.2):
that leeds to the changes for the return of
voidI guess i will need multiple CI runs to get it green with all the deprecations
Summary by CodeRabbit
Chores
Refactor
Tests
Style
✏️ Tip: You can customize this high-level summary in your review settings.