-
-
Notifications
You must be signed in to change notification settings - Fork 56
(test) Enable native lazy objects on PHP 8.4 in base test cases #492
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughAdded PHP-version-guarded calls in test setup to enable Doctrine's native lazy objects when running on PHP 8.4+; the change invokes enableNativeLazyObjects(true) on the configuration inside setUp() for two test TestCase classes. 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 (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (6)📓 Common learnings📚 Learning: 2025-05-23T11:11:57.951ZApplied to files:
📚 Learning: 2025-03-29T03:31:17.114ZApplied to files:
📚 Learning: 2025-09-01T18:48:28.508ZApplied to files:
📚 Learning: 2025-04-23T20:20:39.026ZApplied to files:
📚 Learning: 2025-08-09T15:14:11.841ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
🔇 Additional comments (1)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/Integration/MartinGeorgiev/TestCase.php(1 hunks)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TestCase.php(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: landure
Repo: martin-georgiev/postgresql-for-doctrine PR: 411
File: tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/DBALTypesTest.php:21-27
Timestamp: 2025-08-09T15:14:11.841Z
Learning: PHP silently ignores unknown attributes in versions that don't recognize them. The #[\Override] attribute introduced in PHP 8.3 can be safely used in code targeting PHP 8.1+ as it will be ignored without errors in 8.1/8.2 and provide override validation in 8.3+. This allows forward compatibility without breaking older PHP versions.
📚 Learning: 2025-05-23T11:11:57.951Z
Learnt from: martin-georgiev
Repo: martin-georgiev/postgresql-for-doctrine PR: 383
File: tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RadiansTest.php:1-9
Timestamp: 2025-05-23T11:11:57.951Z
Learning: Tests in the `Tests\Unit\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\Unit\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), rather than PHPUnit's TestCase directly, and therefore don't need an explicit import.
Applied to files:
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TestCase.phptests/Integration/MartinGeorgiev/TestCase.php
📚 Learning: 2025-03-29T03:31:17.114Z
Learnt from: martin-georgiev
Repo: martin-georgiev/postgresql-for-doctrine PR: 318
File: tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAggTest.php:1-9
Timestamp: 2025-03-29T03:31:17.114Z
Learning: Tests in the `Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), rather than PHPUnit's TestCase, and therefore don't need an explicit import.
Applied to files:
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TestCase.phptests/Integration/MartinGeorgiev/TestCase.php
📚 Learning: 2025-09-01T18:48:28.508Z
Learnt from: martin-georgiev
Repo: martin-georgiev/postgresql-for-doctrine PR: 434
File: tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/PostGIS/ST_CrossesTest.php:12-31
Timestamp: 2025-09-01T18:48:28.508Z
Learning: When analyzing unit test files in this codebase, always verify the actual file structure and existing patterns before suggesting changes. The ContainsGeometries fixture exists at ./fixtures/MartinGeorgiev/Doctrine/Entity/ContainsGeometries.php and the PostGIS unit tests use the namespace Tests\Unit\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\PostGIS consistently.
Applied to files:
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TestCase.phptests/Integration/MartinGeorgiev/TestCase.php
📚 Learning: 2025-04-23T20:20:39.026Z
Learnt from: martin-georgiev
Repo: martin-georgiev/postgresql-for-doctrine PR: 0
File: :0-0
Timestamp: 2025-04-23T20:20:39.026Z
Learning: In this project, unit tests should avoid using reflection (ReflectionMethod) to test protected methods directly, and instead use fixtures to test through public interfaces, even if it means slightly lower test coverage.
Applied to files:
tests/Integration/MartinGeorgiev/TestCase.php
🪛 GitHub Check: PHP 8.1 + Doctrine ORM 2.14 + Doctrine Lexer 2.1
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TestCase.php
[failure] 32-32:
Call to an undefined method Doctrine\ORM\Configuration::enableNativeLazyObjects().
tests/Integration/MartinGeorgiev/TestCase.php
[failure] 81-81:
Call to an undefined method Doctrine\ORM\Configuration::enableNativeLazyObjects().
🪛 GitHub Check: PHP 8.1 + Doctrine ORM 2.14 + Doctrine Lexer latest
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TestCase.php
[failure] 32-32:
Call to an undefined method Doctrine\ORM\Configuration::enableNativeLazyObjects().
tests/Integration/MartinGeorgiev/TestCase.php
[failure] 81-81:
Call to an undefined method Doctrine\ORM\Configuration::enableNativeLazyObjects().
🪛 GitHub Check: PHP 8.1 + Doctrine ORM 2.18 + Doctrine Lexer 2.1
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TestCase.php
[failure] 32-32:
Call to an undefined method Doctrine\ORM\Configuration::enableNativeLazyObjects().
tests/Integration/MartinGeorgiev/TestCase.php
[failure] 81-81:
Call to an undefined method Doctrine\ORM\Configuration::enableNativeLazyObjects().
🪛 GitHub Check: PHP 8.1 + Doctrine ORM 2.18 + Doctrine Lexer 3.0
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TestCase.php
[failure] 32-32:
Call to an undefined method Doctrine\ORM\Configuration::enableNativeLazyObjects().
tests/Integration/MartinGeorgiev/TestCase.php
[failure] 81-81:
Call to an undefined method Doctrine\ORM\Configuration::enableNativeLazyObjects().
🪛 GitHub Check: PHP 8.1 + Doctrine ORM 2.18 + Doctrine Lexer latest
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TestCase.php
[failure] 32-32:
Call to an undefined method Doctrine\ORM\Configuration::enableNativeLazyObjects().
tests/Integration/MartinGeorgiev/TestCase.php
[failure] 81-81:
Call to an undefined method Doctrine\ORM\Configuration::enableNativeLazyObjects().
🪛 GitHub Check: PHP 8.1 + Doctrine ORM latest + Doctrine Lexer 2.1
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TestCase.php
[failure] 32-32:
Call to an undefined method Doctrine\ORM\Configuration::enableNativeLazyObjects().
tests/Integration/MartinGeorgiev/TestCase.php
[failure] 81-81:
Call to an undefined method Doctrine\ORM\Configuration::enableNativeLazyObjects().
🪛 GitHub Check: PHP 8.2 + Doctrine ORM 2.18 + Doctrine Lexer 3.0
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TestCase.php
[failure] 32-32:
Call to an undefined method Doctrine\ORM\Configuration::enableNativeLazyObjects().
tests/Integration/MartinGeorgiev/TestCase.php
[failure] 81-81:
Call to an undefined method Doctrine\ORM\Configuration::enableNativeLazyObjects().
🪛 GitHub Check: PHP 8.2 + Doctrine ORM latest + Doctrine Lexer 2.1
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TestCase.php
[failure] 32-32:
Call to an undefined method Doctrine\ORM\Configuration::enableNativeLazyObjects().
tests/Integration/MartinGeorgiev/TestCase.php
[failure] 81-81:
Call to an undefined method Doctrine\ORM\Configuration::enableNativeLazyObjects().
🪛 GitHub Check: PHP 8.3 + Doctrine ORM 2.18 + Doctrine Lexer 3.0
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TestCase.php
[failure] 32-32:
Call to an undefined method Doctrine\ORM\Configuration::enableNativeLazyObjects().
tests/Integration/MartinGeorgiev/TestCase.php
[failure] 81-81:
Call to an undefined method Doctrine\ORM\Configuration::enableNativeLazyObjects().
🪛 GitHub Check: PHP 8.3 + Doctrine ORM latest + Doctrine Lexer 2.1
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TestCase.php
[failure] 32-32:
Call to an undefined method Doctrine\ORM\Configuration::enableNativeLazyObjects().
tests/Integration/MartinGeorgiev/TestCase.php
[failure] 81-81:
Call to an undefined method Doctrine\ORM\Configuration::enableNativeLazyObjects().
🪛 GitHub Check: PHP 8.4 + Doctrine ORM 2.14 + Doctrine Lexer 2.1
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TestCase.php
[failure] 32-32:
Call to an undefined method Doctrine\ORM\Configuration::enableNativeLazyObjects().
tests/Integration/MartinGeorgiev/TestCase.php
[failure] 81-81:
Call to an undefined method Doctrine\ORM\Configuration::enableNativeLazyObjects().
🪛 GitHub Check: PHP 8.4 + Doctrine ORM 2.14 + Doctrine Lexer latest
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TestCase.php
[failure] 32-32:
Call to an undefined method Doctrine\ORM\Configuration::enableNativeLazyObjects().
tests/Integration/MartinGeorgiev/TestCase.php
[failure] 81-81:
Call to an undefined method Doctrine\ORM\Configuration::enableNativeLazyObjects().
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: PostgreSQL 17 + PostGIS 3.5 + PHP 8.4
- GitHub Check: PostgreSQL 17 + PostGIS 3.6 + PHP 8.2
- GitHub Check: PostgreSQL 17 + PostGIS 3.4 + PHP 8.5
- GitHub Check: PostgreSQL 18 + PostGIS 3.6 + PHP 8.3
- GitHub Check: PostgreSQL 17 + PostGIS 3.5 + PHP 8.5
- GitHub Check: PostgreSQL 17 + PostGIS 3.4 + PHP 8.4
- GitHub Check: PostgreSQL 17 + PostGIS 3.4 + PHP 8.3
- GitHub Check: PostgreSQL 16 + PostGIS 3.4 + PHP 8.2
- GitHub Check: PostgreSQL 16 + PostGIS 3.5 + PHP 8.3
- GitHub Check: PostgreSQL 17 + PostGIS 3.5 + PHP 8.2
- GitHub Check: PostgreSQL 16 + PostGIS 3.4 + PHP 8.3
- GitHub Check: PostgreSQL 16 + PostGIS 3.4 + PHP 8.1
- GitHub Check: PostgreSQL 16 + PostGIS 3.5 + PHP 8.1
- GitHub Check: PostgreSQL 18 + PostGIS 3.6 + PHP 8.2
- GitHub Check: PostgreSQL 16 + PostGIS 3.5 + PHP 8.2
- GitHub Check: PostgreSQL 17 + PostGIS 3.6 + PHP 8.1
- GitHub Check: PostgreSQL 18 + PostGIS 3.6 + PHP 8.1
- GitHub Check: PostgreSQL 17 + PostGIS 3.5 + PHP 8.1
- GitHub Check: PostgreSQL 17 + PostGIS 3.4 + PHP 8.1
- GitHub Check: wait-for-tests-worflows-before-upload
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TestCase.php
Outdated
Show resolved
Hide resolved
0300328 to
e3a4db7
Compare
|
Should I worry about the failed checks? It looks like they're not caused by the code I'm trying to contribute. |
|
Can you please raise a standard issue for the problem you're having? I'm not sure you should continue with the current approach, but I'm missing some context details to help you here. |
|
I think I know why you raised this PR. However, the implementation was indeed going in the wrong direction - this shouldn't have been a code change but rather an update on the dependency constraints (as it is linked to the newly released Symfony 8). |
|
@CodeRabbit - Learn from this comment for the future, as you should have deep researched this. The root case was caused by a newly released dependency and shouldn't have had the unusual PHP version conditional check. |
|
Tip For best results, initiate chat on the files or code changes. @martin-georgiev, thank you for this guidance. I understand the important distinction here: What I should have recognized:
Why the proposed approach was wrong:
I should have investigated the dependency tree and recent releases of Doctrine ORM and Symfony components to identify that this was a dependency version constraint issue requiring updates to I'll apply this understanding in future reviews involving test failures related to new PHP versions or missing features. 🐰📚 ✏️ Learnings added
🧠 Learnings used |
|
I think PR 490 fixes a different problem; PHPStan incompatibility doesn't share much with Doctrine's assumption that native objects can/should be used at a specific version of PHP when var-exporter is at a newer version. Installing var-exporter 8 broke no tests, so the package is not incompatible with it per se. I think once PHPStan catches up, unless you decide to keep var-exporter <8 forever, the change I'm proposing (or something similar) will be necessary to support both current and legacy versions. With that said, it's your project, and 490 also makes Doctrine's problem go away for now, so... meh. Thanks! |
I've been getting a ton of false-positive failures when running
composer phpunit:unitdirectly on my PHP 8.4.x dev machine:This small change enables the native lazy objects on PHP 8.4+ and fixes the problem for me.
If it's not the right way to fix the problem, please advise.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.