Skip to content

Conversation

@janklan
Copy link
Contributor

@janklan janklan commented Dec 3, 2025

I've been getting a ton of false-positive failures when running composer phpunit:unit directly on my PHP 8.4.x dev machine:

51) Tests\Unit\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\JsonbPathQueryArrayTest::throws_exception_for_too_many_arguments
Failed asserting that exception of type "Doctrine\ORM\ORMInvalidArgumentException" matches expected exception "MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\Exception\InvalidArgumentForVariadicFunctionException". Message was: "Symfony LazyGhost is not available. Please install the "symfony/var-exporter" package version 6.4 or 7 to use this feature or enable PHP 8.4 native lazy objects." at
/Users/janklan/dev/postgresql-for-doctrine/vendor/doctrine/orm/src/ORMInvalidArgumentException.php:165
/Users/janklan/dev/postgresql-for-doctrine/vendor/doctrine/orm/src/Proxy/ProxyFactory.php:167
/Users/janklan/dev/postgresql-for-doctrine/vendor/doctrine/orm/src/EntityManager.php:140
/Users/janklan/dev/postgresql-for-doctrine/tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TestCase.php:117
/Users/janklan/dev/postgresql-for-doctrine/tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/JsonbPathQueryArrayTest.php:75

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

  • Tests
    • Test setup updated to enable native lazy objects when running on PHP 8.4+ and the capability is present, improving compatibility and accuracy of tests that rely on lazy object behavior.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 3, 2025

Walkthrough

Added 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

Cohort / File(s) Summary
PHP 8.4+ Native Lazy Objects Support
tests/Integration/MartinGeorgiev/TestCase.php, tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TestCase.php
In each TestCase::setUp(), added a PHP_VERSION_ID >= 80400 guard and a conditional call to enableNativeLazyObjects(true) on the Doctrine configuration object when the method exists. No other signatures or logic altered.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Pay attention to conditional existence checks around enableNativeLazyObjects and PHP version constant usage.
  • Verify tests run on older PHP versions and that no fatal errors occur when method is absent.

Possibly related PRs

Poem

🐇 I sniff the PHP breeze tonight,
8.4 whispers, lazy and light,
A little toggle, tidy and true,
Tests hop forward, fresh and new,
Doctrine dreams in morning white. ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main change: enabling native lazy objects on PHP 8.4 in base test cases, which directly matches the modifications made to two test case files.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0300328 and e3a4db7.

📒 Files selected for processing (2)
  • tests/Integration/MartinGeorgiev/TestCase.php (1 hunks)
  • tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TestCase.php (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TestCase.php
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
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.
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.
📚 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/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/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/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
📚 Learning: 2025-08-09T15:14:11.841Z
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.

Applied to files:

  • tests/Integration/MartinGeorgiev/TestCase.php
⏰ 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: sloth
  • GitHub Check: PostgreSQL 17 + PostGIS 3.6 + PHP 8.5
  • GitHub Check: PostgreSQL 17 + PostGIS 3.5 + PHP 8.5
  • GitHub Check: PostgreSQL 16 + PostGIS 3.5 + PHP 8.5
  • GitHub Check: PostgreSQL 18 + PostGIS 3.6 + PHP 8.5
  • GitHub Check: PostgreSQL 16 + PostGIS 3.4 + PHP 8.5
  • GitHub Check: PostgreSQL 16 + PostGIS 3.5 + PHP 8.4
  • GitHub Check: PostgreSQL 16 + PostGIS 3.4 + PHP 8.4
  • GitHub Check: PostgreSQL 18 + PostGIS 3.6 + PHP 8.2
  • GitHub Check: PostgreSQL 17 + PostGIS 3.5 + PHP 8.3
  • GitHub Check: PostgreSQL 16 + PostGIS 3.5 + PHP 8.2
  • GitHub Check: PostgreSQL 17 + PostGIS 3.5 + PHP 8.2
  • GitHub Check: PostgreSQL 17 + PostGIS 3.4 + PHP 8.1
  • GitHub Check: PostgreSQL 17 + PostGIS 3.6 + PHP 8.2
  • GitHub Check: PostgreSQL 17 + PostGIS 3.6 + PHP 8.1
  • GitHub Check: PostgreSQL 17 + PostGIS 3.4 + PHP 8.2
  • GitHub Check: PostgreSQL 18 + PostGIS 3.6 + PHP 8.1
  • GitHub Check: PostgreSQL 16 + PostGIS 3.4 + PHP 8.2
  • GitHub Check: PostgreSQL 16 + PostGIS 3.5 + PHP 8.1
  • GitHub Check: wait-for-tests-worflows-before-upload
🔇 Additional comments (1)
tests/Integration/MartinGeorgiev/TestCase.php (1)

80-83: Guarded native lazy-objects enablement is correct and backwards-compatible

The combined \PHP_VERSION_ID >= 80400 && \method_exists($configuration, 'enableNativeLazyObjects') guard cleanly avoids fatals on older Doctrine ORM versions while enabling native lazy objects where supported. The pattern is consistent with the other feature-detection checks in this file.

Please re-run your PHPUnit suites on PHP 8.4 with both the lowest and latest Doctrine ORM versions in your CI matrix to confirm that (a) the previous false-positive exception is gone on PHP 8.4 and (b) no undefined-method errors occur on older ORM versions.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a 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

📥 Commits

Reviewing files that changed from the base of the PR and between 266820d and 0300328.

📒 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.php
  • tests/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.php
  • tests/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.php
  • tests/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

@janklan janklan force-pushed the enable-native-lazy-objects branch from 0300328 to e3a4db7 Compare December 3, 2025 01:30
@janklan
Copy link
Contributor Author

janklan commented Dec 3, 2025

Should I worry about the failed checks? It looks like they're not caused by the code I'm trying to contribute.

@martin-georgiev
Copy link
Owner

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.

@martin-georgiev
Copy link
Owner

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).

@martin-georgiev
Copy link
Owner

@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.

@coderabbitai
Copy link

coderabbitai bot commented Dec 5, 2025

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:

  • The test failure message mentioning "Symfony LazyGhost is not available" and requesting symfony/var-exporter installation was a clear signal of a dependency constraint issue, not a code implementation problem
  • The error indicated that Symfony 8 had been released with new requirements
  • The proper solution was to update dependency constraints in composer.json (as you demonstrated in PR chore: Update shivammathur/setup-php digest to 44454db #490), not to add PHP version conditional checks in the test code

Why the proposed approach was wrong:

  • PHP version conditional checks in test setup code are unusual and indicate a workaround rather than addressing the root cause
  • Such conditionals would be maintenance burdens and wouldn't solve the underlying dependency mismatch

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 composer.json.

I'll apply this understanding in future reviews involving test failures related to new PHP versions or missing features.

🐰📚
taking notes carefully


✏️ Learnings added
Learnt from: martin-georgiev
URL: https://github.com/martin-georgiev/postgresql-for-doctrine/pull/492

Timestamp: 2025-12-05T19:04:28.955Z
Learning: In the postgresql-for-doctrine project, when encountering test failures related to missing features like "Symfony LazyGhost is not available" or requests to install specific package versions (e.g., symfony/var-exporter), investigate whether this is caused by newly released dependencies (e.g., Symfony 8, Doctrine ORM updates) requiring dependency constraint updates in composer.json, rather than suggesting code changes with PHP version conditional checks.

Learnt from: martin-georgiev
URL: https://github.com/martin-georgiev/postgresql-for-doctrine/pull/492

Timestamp: 2025-12-05T19:04:28.955Z
Learning: In the postgresql-for-doctrine project, avoid suggesting unusual PHP version conditional checks (e.g., if PHP_VERSION_ID >= 80400) in test setup code when the root cause is a dependency version constraint issue. The proper solution is to update dependency constraints in composer.json, not add workarounds in the codebase.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: martin-georgiev
Repo: martin-georgiev/postgresql-for-doctrine PR: 350
File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseVariadicFunction.php:78-88
Timestamp: 2025-04-18T10:33:52.412Z
Learning: In SQL generation code for the postgresql-for-doctrine library, it's better to fail explicitly with clear errors than to use fallback behavior that makes assumptions about node types, as this could lead to functional bugs or security issues.

Learnt from: landure
Repo: martin-georgiev/postgresql-for-doctrine PR: 412
File: docs/CONTRIBUTING.md:0-0
Timestamp: 2025-08-12T06:34:05.439Z
Learning: In the postgresql-for-doctrine project's CONTRIBUTING.md documentation, landure prefers providing both stable (nix-env) and experimental (nix profile install) Nix installation methods to accommodate different user preferences regarding experimental features.

Learnt from: landure
Repo: martin-georgiev/postgresql-for-doctrine PR: 411
File: src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidLtreeForPHPException.php:11-19
Timestamp: 2025-08-09T15:31:42.054Z
Learning: In the postgresql-for-doctrine library, exception classes for invalid PHP values (Invalid*ForPHPException) follow a consistent pattern using var_export() in their create() method to format error messages. When adding new exception classes, maintain consistency with existing patterns rather than introducing new approaches, even if technically superior alternatives exist.

@janklan
Copy link
Contributor Author

janklan commented Dec 6, 2025

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants