Add support for doctrine bundle v3 #400
Conversation
|
Warning Rate limit exceeded@norkunas has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 21 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 (13)
WalkthroughRefactors CI matrix to add a dependencies axis and per-matrix installs; updates composer constraints and dev-tool versions (DoctrineBundle v3 compatibility, phpstan v2, etc.); restructures test config loading in tests/Kernel.php with new doctrine/config branches and consolidated framework config; updates baselines/phpstan; changes several delete method return types to non-nullable arrays. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as GitHub Actions
participant Job as Test Job
participant Composer as Composer
CI->>Job: Start job (php, sf, dependencies matrix)
Job->>Composer: Install using matrix.dependency-versions + matrix.composer-options
Composer-->>Job: Dependencies installed
Job->>Job: Run tests and collect coverage
Job-->>CI: Upload coverage artifact (includes matrix.dependencies)
sequenceDiagram
participant Kernel as tests/Kernel::configureContainer
participant Loader as Config loader
Kernel->>Kernel: Load `tests/config/framework.yaml`
Kernel->>Kernel: Check BlacklistSchemaAssetFilter (DoctrineBundle v3)
alt DoctrineBundle v3 present
Kernel->>Loader: Load `tests/config/doctrine.yaml`
else
Kernel->>Kernel: Check LegacyReflectionFields and PHP version
alt PHP >= 8.4 and LegacyReflectionFields present
Kernel->>Loader: Load `tests/config/doctrine_v2.yaml`
else if PHP <= 7
Kernel->>Loader: Load `tests/config/doctrine_php7.yaml`
else
Kernel->>Loader: Load `tests/config/doctrine_old_proxy.yaml`
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
norkunas
force-pushed
the
doctrinebundlev3
branch
2 times, most recently
from
0a4e91a to
9c4efad
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/tests.yml(2 hunks)composer.json(1 hunks)tests/Kernel.php(2 hunks)tests/config/config.yaml(0 hunks)tests/config/config_doctrine_v2.yaml(1 hunks)tests/config/config_old_proxy.yaml(0 hunks)tests/config/doctrine_php7.yaml(0 hunks)tests/config/framework.yaml(1 hunks)
💤 Files with no reviewable changes (3)
- tests/config/doctrine_php7.yaml
- tests/config/config_old_proxy.yaml
- tests/config/config.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Kernel.php (1)
src/DependencyInjection/MeilisearchExtension.php (1)
load(19-64)
🪛 actionlint (1.7.8)
.github/workflows/tests.yml
101-101: property "dependencies" is not defined in object type {deps: string; php-version: number; sf-version: number}
(expression)
🔇 Additional comments (8)
tests/config/framework.yaml (1)
1-4: LGTM!Standard framework configuration for test environment. The settings are appropriate for integration tests.
tests/config/config_doctrine_v2.yaml (1)
1-22: LGTM! Doctrine v2-specific configuration is well-structured.The configuration correctly isolates v2-specific ORM options (
enable_native_lazy_objects,report_fields_where_declared) that are not available or have different semantics in v3.tests/Kernel.php (4)
11-11: LGTM! Appropriate version detection strategy.Using
BlacklistSchemaAssetFilteras a version marker is a pragmatic approach since this class was removed in DoctrineBundle v3.
33-33: LGTM! Framework config externalized correctly.Loading framework configuration from a separate file improves maintainability and aligns with the version-specific config strategy.
35-44: LGTM! Version-aware config loading is well-structured.The conditional logic correctly handles:
- DoctrineBundle v3 (loads config.yaml)
- DoctrineBundle v2 with PHP 8.4+ and LegacyReflectionFields (loads config_doctrine_v2.yaml)
- Older setups (loads config_old_proxy.yaml)
46-53: LGTM! PHP 7 compatibility handled appropriately.The PHP 7 branch correctly configures annotations and loads the PHP 7-specific Doctrine config.
composer.json (1)
23-23: The constraint is correct—no changes needed.The
^2.10 || ^3.0constraint properly handles version resolution across supported PHP versions. DoctrineBundle v2.10 requires PHP ^7.4 or ^8.0, covering all versions your package supports. DoctrineBundle v3 requires PHP ^8.4, so Composer will:
- Install v2.x on PHP 7.4-8.3 (where v3 is unavailable)
- Install v3.x on PHP 8.4+ (where both are available)
This is appropriate package design that supports multiple dependency versions across different PHP versions.
.github/workflows/tests.yml (1)
101-101: No action required—default behavior is safe and correct.The ramsey/composer-install action defaults to "locked" when dependency-versions is empty, installing locked versions equivalent to running composer install. This is the intended behavior for matrix entries without an explicit dependencies property, and no verification or code changes are needed.
Likely an incorrect or invalid review comment.
norkunas
force-pushed
the
doctrinebundlev3
branch
6 times, most recently
from
be47be4 to
2db723c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #400 +/- ##
============================================
- Coverage 88.40% 88.38% -0.02%
============================================
Files 20 20
Lines 888 878 -10
============================================
- Hits 785 776 -9
+ Misses 103 102 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
norkunas
force-pushed
the
doctrinebundlev3
branch
8 times, most recently
from
d3be2cf to
dd84a5f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
composer.json (1)
51-51: Patch‑pin phpunit‑bridge: verify tags and PHP support.Please confirm 6.4.25 and 7.0.10 exist on Packagist and cover PHP 7.4→8.4 in your matrix. If any tag is missing, relax to the minor (e.g., ^6.4 || ^7.0) or bump to the next available patch.
tests/Kernel.php (1)
8-8: Avoid importing a class removed in v3 to keep static analysis quiet.Drop the
use Doctrine\Bundle\DoctrineBundle\Dbal\BlacklistSchemaAssetFilter;and use the FQCN in place to prevent “unknown class” warnings on v3 while preserving runtime detection.- use Doctrine\Bundle\DoctrineBundle\Dbal\BlacklistSchemaAssetFilter; ... - $doctrineBundleV3 = !class_exists(BlacklistSchemaAssetFilter::class); + $doctrineBundleV3 = !class_exists(\Doctrine\Bundle\DoctrineBundle\Dbal\BlacklistSchemaAssetFilter::class);Also applies to: 34-34
.github/workflows/tests.yml (3)
31-36: Set strategy fail‑fast explicitly (env var doesn’t affect matrix).Add strategy.fail-fast to control cancellation behavior; the top-level env key has no effect.
strategy: + fail-fast: false matrix: php-version: ['7.4', '8.1', '8.2', '8.3', '8.4'] sf-version: ['5.4', '6.4', '7.0', '7.1', '7.2', '7.3'] dependencies: ['locked']
63-65: Remove dead exclude.Matrix doesn’t include PHP 8.0; this exclude is redundant and can be dropped.
- - php-version: '8.0' - sf-version: '7.2'
93-96: Gate the sed step to non‑locked runs (and consider composer remove).Editing composer.json while using locked deps is ineffective and may cause validate warnings. Run this only when not locked; optionally switch to composer remove for clarity.
- - name: Remove doctrine/annotations - if: matrix.php-version != '7.4' - run: sed -i '/doctrine\/annotations/d' composer.json + - name: Remove doctrine/annotations + if: matrix.php-version != '7.4' && matrix.dependencies != 'locked' + run: sed -i '/doctrine\/annotations/d' composer.json +# Or: +# run: composer remove -n --no-update doctrine/annotations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/tests.yml(4 hunks).php-cs-fixer.dist.php(1 hunks)composer.json(2 hunks)tests/Kernel.php(2 hunks)tests/baseline-ignore(1 hunks)tests/config/doctrine.yaml(1 hunks)tests/config/doctrine_old_proxy.yaml(0 hunks)tests/config/doctrine_php7.yaml(0 hunks)tests/config/doctrine_v2.yaml(0 hunks)tests/config/framework.yaml(1 hunks)
💤 Files with no reviewable changes (3)
- tests/config/doctrine_v2.yaml
- tests/config/doctrine_old_proxy.yaml
- tests/config/doctrine_php7.yaml
✅ Files skipped from review due to trivial changes (2)
- tests/config/doctrine.yaml
- tests/baseline-ignore
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/config/framework.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Kernel.php (1)
src/DependencyInjection/MeilisearchExtension.php (1)
load(19-64)
🔇 Additional comments (6)
.php-cs-fixer.dist.php (1)
16-16: ****The ruleset name
@PHP8x0Migration:riskyis valid. PHP-CS-Fixer's official rulesets include@PHP8x0Migrationand@PHP8x0Migration:risky, and this ruleset is part of the PHP-CS-Fixer rule set definitions. The 'x' in the middle is standard nomenclature in PHP-CS-Fixer (e.g.,@PHP5x4Migration,@PHP7x0Migration,@PHP8x0Migration,@PHP8x2Migration), not a typo or error. The original review comment's concern about the ruleset being invalid or unconventional was based on incorrect assumptions about the naming pattern.Likely an incorrect or invalid review comment.
composer.json (1)
23-23: DoctrineBundle v3 allowance looks good.Dual constraint keeps 7.4 compatibility and enables v3 on newer PHP. No issues spotted.
tests/Kernel.php (3)
32-43: Config selection by DoctrineBundle/PHP version is clear and correct.Framework first, then doctrine.yaml (v3) or v2/old proxy split for PHP 8+, with sane fallback. Looks good.
Please confirm tests cover all three branches: v3, v2+PHP 8.4 with LegacyReflectionFields, and the old proxy path.
45-52: PHP 7 path is pragmatic.Inline framework tweaks + doctrine_php7.yaml load are fine for legacy runs.
55-62: Conditional ORM prepends are safely gated.The DEFAULT_SCHEME_MAP + “not v3” guard prevents applying legacy settings on v3. LGTM.
.github/workflows/tests.yml (1)
66-77: Matrix expansion and parameterization look good.
- “dependencies” axis + dynamic job name + composer install parameters are consistent.
- Artifact names now unique per dependency mode.
Also applies to: 79-79, 102-104, 112-112
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.github/workflows/tests.yml (2)
15-15: env.fail-fast has no effect; use strategy.fail-fast instead.Set it under the job’s strategy or remove this env var.
-env: - fail-fast: true + # (Optional) If needed, control fail-fast per job: + # strategy: + # fail-fast: false
93-96: Scope the annotations removal to Symfony 7+.Keying off PHP version may remove doctrine/annotations for Symfony 6.4 runs unnecessarily.
- - name: Remove doctrine/annotations - if: matrix.php-version != '7.4' + - name: Remove doctrine/annotations for Symfony 7+ + if: startsWith(matrix.sf-version, '7.') run: sed -i '/doctrine\/annotations/d' composer.jsonIf you want safer edits than sed, consider:
run: composer remove doctrine/annotations --no-update || truetests/Kernel.php (1)
74-86: FixKernel::VERSION_IDto use theHttpKernelaliasInside the local
Kernelclass,Kernel::VERSION_IDresolves to the local class (not Symfony's), but the local class doesn't define this constant. Symfony's HttpKernel defines VERSION_ID as a class constant encoding the version number. Use the importedHttpKernelalias instead:// @phpstan-ignore-next-line - if (Kernel::VERSION_ID >= 60400) { + if (HttpKernel::VERSION_ID >= 60400) { $container->prependExtensionConfig('framework', [ 'handle_all_throwables' => true, 'php_errors' => ['log' => true], ]); } // @phpstan-ignore-next-line - if (Kernel::VERSION_ID >= 70300) { + if (HttpKernel::VERSION_ID >= 70300) { $container->prependExtensionConfig('framework', [ 'property_info' => ['with_constructor_extractor' => false], ]); }
🧹 Nitpick comments (4)
.github/workflows/tests.yml (2)
25-25: Pin Meilisearch image to a tag to avoid surprise breakages.latest can change underneath you. Prefer a major/minor tag or digest.
- image: getmeili/meilisearch:latest + image: getmeili/meilisearch:v1
63-65: Redundant exclude entry.php 8.0 isn’t part of the matrix; this exclude is dead config. Remove for clarity.
- - php-version: '8.0' - sf-version: '7.2'tests/Kernel.php (2)
8-8: Avoid importing a class removed in DoctrineBundle v3; inline FQCN in class_exists.Prevents static analysis noise in v3 contexts.
-use Doctrine\Bundle\DoctrineBundle\Dbal\BlacklistSchemaAssetFilter; +// Avoid importing a class missing in DoctrineBundle v3; reference FQCN inline.And:
- $doctrineBundleV3 = !class_exists(BlacklistSchemaAssetFilter::class); + $doctrineBundleV3 = !class_exists(\Doctrine\Bundle\DoctrineBundle\Dbal\BlacklistSchemaAssetFilter::class);
39-43: Confirm PHP 8.4 + LegacyReflectionFields gating.Double-check that the doctrine_v2.yaml vs doctrine_old_proxy.yaml split strictly needs PHP >= 8.4. If the proxy-path is only for PHP < 8.4 regardless of ORM class presence, consider simplifying to a single class_exists check.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/tests.yml(4 hunks).php-cs-fixer.dist.php(1 hunks)composer.json(2 hunks)tests/Kernel.php(2 hunks)tests/baseline-ignore(1 hunks)tests/config/doctrine.yaml(1 hunks)tests/config/doctrine_old_proxy.yaml(0 hunks)tests/config/doctrine_php7.yaml(0 hunks)tests/config/doctrine_v2.yaml(0 hunks)tests/config/framework.yaml(1 hunks)
💤 Files with no reviewable changes (3)
- tests/config/doctrine_php7.yaml
- tests/config/doctrine_old_proxy.yaml
- tests/config/doctrine_v2.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/config/doctrine.yaml
- composer.json
- .php-cs-fixer.dist.php
🧰 Additional context used
🪛 Gitleaks (8.28.0)
tests/config/framework.yaml
[high] 3-3: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (1)
tests/baseline-ignore (1)
11-11: Baseline entry is fine; remove once v2 path is dropped.The ignore matches the conditional use of report_fields_where_declared for DoctrineBundle v2 only. Plan to delete this baseline when v2 support is removed to avoid masking regressions.
Confirm the message string matches exactly what the bridge emits under your CI locale.
| @@ -0,0 +1,4 @@ | |||
| framework: | |||
| test: true | |||
| secret: 67d829bf61dc5f87a73fd814e2c9f629 | |||
There was a problem hiding this comment.
Avoid key-like test secret to silence secret scanners.
Use a clearly non-secret value or env var to prevent gitleaks noise.
Apply one of:
-framework:
- test: true
- secret: 67d829bf61dc5f87a73fd814e2c9f629
- http_method_override: false
+framework:
+ test: true
+ secret: 'test_secret_value'
+ http_method_override: falseor (if you prefer env):
- secret: 67d829bf61dc5f87a73fd814e2c9f629
+ secret: '%env(string:default::TEST_APP_SECRET)%'Add TEST_APP_SECRET to CI env if using the second option.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| secret: 67d829bf61dc5f87a73fd814e2c9f629 | |
| secret: '%env(string:default::TEST_APP_SECRET)%' |
🧰 Tools
🪛 Gitleaks (8.28.0)
[high] 3-3: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
In tests/config/framework.yaml at line 3 replace the key-like secret value with
a clearly non-secret placeholder or an env var reference to avoid secret
scanners; either set secret: "test-secret" (or similar obviously fake value) or
change to secret: "${TEST_APP_SECRET}" and add TEST_APP_SECRET to CI environment
variables, ensuring no real keys are committed.
dd84a5f to
0bb48ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/config/framework.yaml (1)
1-4: Test configuration is appropriate.The framework test configuration with the hardcoded secret is standard for test environments. The Gitleaks alert is a false positive—this is not a real credential.
Note: A previous review comment already suggested using a more obviously fake value like
'test_secret_value'to avoid triggering secret scanners, which remains a valid optional improvement.
🧹 Nitpick comments (1)
tests/Kernel.php (1)
34-34: Version detection approach is pragmatic but consider adding a comment.The version detection using
!class_exists(BlacklistSchemaAssetFilter::class)is correct—this class exists in DoctrineBundle v2 but was removed in v3. However, the logic is not immediately obvious to future maintainers.Consider adding a clarifying comment:
+// Detect DoctrineBundle v3: BlacklistSchemaAssetFilter was removed in v3 $doctrineBundleV3 = !class_exists(BlacklistSchemaAssetFilter::class);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/tests.yml(4 hunks).php-cs-fixer.dist.php(1 hunks)composer.json(2 hunks)tests/Kernel.php(2 hunks)tests/baseline-ignore(1 hunks)tests/config/doctrine.yaml(1 hunks)tests/config/doctrine_old_proxy.yaml(0 hunks)tests/config/doctrine_php7.yaml(0 hunks)tests/config/doctrine_v2.yaml(0 hunks)tests/config/framework.yaml(1 hunks)
💤 Files with no reviewable changes (3)
- tests/config/doctrine_php7.yaml
- tests/config/doctrine_v2.yaml
- tests/config/doctrine_old_proxy.yaml
✅ Files skipped from review due to trivial changes (1)
- tests/baseline-ignore
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/tests.yml
- composer.json
🧰 Additional context used
🪛 Gitleaks (8.28.0)
tests/config/framework.yaml
[high] 3-3: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (6)
.php-cs-fixer.dist.php (1)
16-16: LGTM! Appropriate rule alias update.The change from
@PHP80Migration:riskyto@PHP8x0Migration:riskybroadens the rule set to cover all PHP 8.x versions, which aligns well with supporting DoctrineBundle v3 across multiple PHP 8.x releases.tests/config/doctrine.yaml (1)
1-19: LGTM! Standard Doctrine v3 test configuration.The configuration correctly sets up Doctrine for testing with:
- SQLite for lightweight testing
- Attribute-based mapping (appropriate for Doctrine v3)
- Proper entity namespace and directory mapping
- Custom type registration for test entities
The structure aligns well with DoctrineBundle v3 requirements.
tests/Kernel.php (4)
8-8: LGTM! Import is safe for version detection.The import of
BlacklistSchemaAssetFilteris used for version detection at line 34. This is safe becauseusestatements don't cause errors if the class doesn't exist—errors only occur when the class is actually instantiated or used in a type hint.
32-32: LGTM! Framework configuration loading added.Loading the framework configuration file is appropriate for the test kernel setup.
36-52: LGTM! Conditional configuration loading is well-structured.The branching logic correctly handles different combinations of PHP versions and Doctrine bundle versions:
- PHP 8.0+ with Doctrine v3 →
doctrine.yaml- PHP 8.0+ with Doctrine v2 (specific cases) →
doctrine_v2.yamlordoctrine_old_proxy.yaml- PHP < 8.0 →
doctrine_php7.yamlwith additional framework annotations configThis approach provides appropriate test configurations for each supported environment.
55-62: LGTM! Condition correctly scoped to DoctrineBundle v2.Adding the
&& !$doctrineBundleV3condition appropriately restricts this ORM configuration to DoctrineBundle v2, where these settings (report_fields_where_declaredandvalidate_xml_mapping) are relevant.
norkunas
force-pushed
the
doctrinebundlev3
branch
6 times, most recently
from
d0ae772 to
dc40bb3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Services/MeilisearchService.php (1)
145-150: Interface mismatch: update SearchService interface.The return type changed from
?arraytoarray, butsrc/SearchService.phpline 44 still declaresdelete(string $className): ?array. Update the interface to match..github/workflows/tests.yml (1)
40-54: Remove dead exclude entries for non-existent matrix values.The static analysis correctly identifies that lines 40, 42, 44, 50, 52, and 54 exclude Symfony versions (7.0, 7.1, 7.2) that don't exist in the base matrix. With
sf-version: ['5.4']as the only base value, these excludes have no effect.Apply this diff to remove the dead exclude entries:
exclude: - php-version: '7.4' sf-version: '6.4' - - php-version: '7.4' - sf-version: '7.0' - - php-version: '7.4' - sf-version: '7.1' - php-version: '7.4' sf-version: '7.2' - - php-version: '7.4' - sf-version: '7.3' - php-version: '8.1' sf-version: '5.4' - - php-version: '8.1' - sf-version: '7.0' - - php-version: '8.1' - sf-version: '7.1' - php-version: '8.1' sf-version: '7.2' - - php-version: '8.1' - sf-version: '7.3' - php-version: '8.2' sf-version: '5.4' - php-version: '8.3' sf-version: '5.4' - php-version: '8.4' sf-version: '5.4'Or remove the entire exclude section since the remaining entries (6.4, 7.2) also don't match any base matrix values.
♻️ Duplicate comments (1)
tests/config/framework.yaml (1)
3-3: Replace generic API key with obviously non-secret placeholder.The hex string is flagged by gitleaks as a Generic API Key. For test configuration, use a clearly fake value to avoid false positives in secret scanners.
Apply this diff:
framework: test: true - secret: 67d829bf61dc5f87a73fd814e2c9f629 + secret: 'test_secret' http_method_override: false
🧹 Nitpick comments (1)
.github/workflows/tests.yml (1)
14-15: Clarify the commented environment variables.The
fail-fastenvironment variable is commented out. If this is intentional for debugging, consider removing it entirely or documenting why it's preserved as a comment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/workflows/tests.yml(6 hunks).php-cs-fixer.dist.php(1 hunks)composer.json(2 hunks)phpstan.dist.neon(1 hunks)src/Engine.php(1 hunks)src/Services/MeilisearchService.php(1 hunks)tests/Kernel.php(2 hunks)tests/baseline-ignore(1 hunks)tests/config/doctrine.yaml(1 hunks)tests/config/doctrine_old_proxy.yaml(0 hunks)tests/config/doctrine_php7.yaml(0 hunks)tests/config/doctrine_v2.yaml(0 hunks)tests/config/framework.yaml(1 hunks)
💤 Files with no reviewable changes (3)
- tests/config/doctrine_php7.yaml
- tests/config/doctrine_old_proxy.yaml
- tests/config/doctrine_v2.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- .php-cs-fixer.dist.php
- tests/baseline-ignore
- tests/Kernel.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/Engine.php (2)
src/Services/MeilisearchService.php (1)
delete(145-150)src/SearchService.php (1)
delete(45-45)
src/Services/MeilisearchService.php (2)
src/SearchService.php (2)
deleteByIndexName(47-47)delete(45-45)src/Engine.php (1)
delete(114-117)
🪛 actionlint (1.7.8)
.github/workflows/tests.yml
40-40: value "7.0" in "exclude" does not match in matrix "sf-version" combinations. possible values are "5.4", "6.4", "7.3"
(matrix)
42-42: value "7.1" in "exclude" does not match in matrix "sf-version" combinations. possible values are "5.4", "6.4", "7.3"
(matrix)
44-44: value "7.2" in "exclude" does not match in matrix "sf-version" combinations. possible values are "5.4", "6.4", "7.3"
(matrix)
54-54: value "7.2" in "exclude" does not match in matrix "sf-version" combinations. possible values are "5.4", "6.4", "7.3"
(matrix)
🪛 GitHub Actions: Tests
src/Services/MeilisearchService.php
[warning] 23-23: TooManyPublicMethods: The class MeilisearchService has 11 public methods. Consider refactoring to keep under 10.
[warning] 23-23: ExcessiveClassComplexity: The class MeilisearchService has an overall complexity of 56 (threshold 50).
[warning] 23-23: CouplingBetweenObjects: The class MeilisearchService has a coupling between objects value of 15. Consider reducing dependencies under 13.
[warning] 44-44: LongVariable: Avoid excessively long variable names like $classToSerializerGroup. Keep under 20 characters.
[warning] 52-52: StaticAccess: Avoid using static access to class '\Symfony\Component\PropertyAccess\PropertyAccess' in method '__construct'.
[warning] 337-337: LongVariable: Avoid excessively long variable names like $searchableEntitiesChunk. Keep under 20 characters.
[warning] 385-385: StaticAccess: Avoid using static access to class '\Doctrine\ORM\Proxy\DefaultProxyClassNameResolver' in method 'resolveClass'.
[warning] 389-389: StaticAccess: Avoid using static access to class '\Doctrine\Common\Util\ClassUtils' in method 'resolveClass'.
🪛 Gitleaks (8.28.0)
tests/config/framework.yaml
[high] 3-3: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (8)
tests/config/doctrine.yaml (1)
1-19: Test Doctrine configuration looks good.The configuration properly sets up SQLite for testing, uses modern Doctrine conventions (attribute-based mapping, underscore_number_aware naming strategy), and correctly registers the custom type. This aligns well with Doctrine Bundle v3 compatibility.
composer.json (3)
23-23: LGTM! Doctrine Bundle v3 support added correctly.The constraint
^2.10 || ^3.0properly adds support for Doctrine Bundle v3 while maintaining backward compatibility with v2.10+.
37-52: Dev dependency updates align with modernization.The PHPStan v2 upgrade and related tooling updates support stricter type checking and align with the Symfony version testing strategy. The removal of
symfony/phpunit-bridge^6.4 support is consistent with the narrowed test matrix.
24-24: Please provide a brief one-line summary per release (v1.0 → v1.16) to help identify the key breaking changes and features, particularly those related to return type declarations and Doctrine Bundle compatibility..github/workflows/tests.yml (3)
34-34: Verify the drastically reduced Symfony version coverage.The matrix now only tests Symfony 5.4 in the base configuration, with commented versions 6.4, 7.0, 7.1, 7.2, 7.3. Coverage for newer Symfony versions is limited to PHP 8.4 in the
includesection. This significantly reduces test coverage compared to the previous configuration.Is this intentional, or should the commented versions be restored?
63-78: Dependencies matrix enhances test coverage across version ranges.The
includeentries effectively test lowest/highest dependency scenarios for key PHP/Symfony combinations, which is valuable for catching compatibility issues with Doctrine Bundle v2 vs. v3 and other dependency variations.
91-92: Removal of composer.lock validation aligns with dependency-variation testing.With the new matrix testing
lowest,locked, andhighestdependency versions, validating the lock file is no longer appropriate since non-locked test runs intentionally diverge from it.phpstan.dist.neon (1)
10-10: ****The code correctly stores only class-strings in
$this->aggregators. Analysis confirms:
- Property type:
list<class-string<Aggregator>>(line 38-40)- Only assignment:
$this->aggregators[] = $index['class'];(line 269)- Source validation:
is_subclass_of($index['class'], Aggregator::class)confirms$index['class']is a valid class-stringarray_unique()on strings works without issueThe PHPStan v2.1 error is a false positive; the suppression is appropriate.
Likely an incorrect or invalid review comment.
norkunas
force-pushed
the
doctrinebundlev3
branch
7 times, most recently
from
2324ecd to
216030a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/config/framework.yaml (1)
3-3: Consider using a clearly non-secret test value.The hardcoded secret triggers security scanners. While acceptable for test configuration, using an obviously fake value like
'test-secret-value'would prevent scanner noise.
🧹 Nitpick comments (1)
.github/workflows/tests.yml (1)
40-54: Remove orphaned exclude entries for non-existent matrix values.The exclude entries for
sf-version: '7.0','7.1', and'7.2'reference values that don't exist in the current matrix (line 34 only includes'5.4'). These are likely left over from when those versions were active in the matrix.Apply this diff to remove the orphaned excludes:
exclude: - php-version: '7.4' sf-version: '6.4' - - php-version: '7.4' - sf-version: '7.0' - - php-version: '7.4' - sf-version: '7.1' - php-version: '7.4' sf-version: '7.2' - php-version: '7.4' sf-version: '7.3' - php-version: '8.1' sf-version: '5.4' - - php-version: '8.1' - sf-version: '7.0' - - php-version: '8.1' - sf-version: '7.1' - php-version: '8.1' sf-version: '7.2' - php-version: '8.1' sf-version: '7.3'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/workflows/tests.yml(6 hunks).php-cs-fixer.dist.php(1 hunks)composer.json(2 hunks)phpstan.dist.neon(1 hunks)src/Engine.php(1 hunks)src/Services/MeilisearchService.php(1 hunks)tests/Kernel.php(2 hunks)tests/baseline-ignore(1 hunks)tests/config/doctrine.yaml(1 hunks)tests/config/doctrine_old_proxy.yaml(0 hunks)tests/config/doctrine_php7.yaml(0 hunks)tests/config/doctrine_v2.yaml(0 hunks)tests/config/framework.yaml(1 hunks)
💤 Files with no reviewable changes (3)
- tests/config/doctrine_php7.yaml
- tests/config/doctrine_v2.yaml
- tests/config/doctrine_old_proxy.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Kernel.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/Engine.php (2)
src/Services/MeilisearchService.php (1)
delete(145-150)src/SearchService.php (1)
delete(45-45)
src/Services/MeilisearchService.php (2)
src/SearchService.php (2)
deleteByIndexName(47-47)delete(45-45)src/Engine.php (1)
delete(114-117)
🪛 actionlint (1.7.8)
.github/workflows/tests.yml
40-40: value "7.0" in "exclude" does not match in matrix "sf-version" combinations. possible values are "5.4", "6.4", "7.3"
(matrix)
42-42: value "7.1" in "exclude" does not match in matrix "sf-version" combinations. possible values are "5.4", "6.4", "7.3"
(matrix)
44-44: value "7.2" in "exclude" does not match in matrix "sf-version" combinations. possible values are "5.4", "6.4", "7.3"
(matrix)
54-54: value "7.2" in "exclude" does not match in matrix "sf-version" combinations. possible values are "5.4", "6.4", "7.3"
(matrix)
🪛 GitHub Actions: Tests
src/Services/MeilisearchService.php
[warning] 23-23: TooManyPublicMethods: The class MeilisearchService has 11 public methods. Consider refactoring MeilisearchService to keep number of public methods under 10.
[warning] 23-23: ExcessiveClassComplexity: The class MeilisearchService has an overall complexity of 56 which is very high. The configured complexity threshold is 50.
[warning] 23-23: CouplingBetweenObjects: The class MeilisearchService has a coupling between objects value of 15. Consider to reduce the number of dependencies under 13.
[warning] 44-44: LongVariable: Avoid excessively long variable names like $classToSerializerGroup. Keep variable name length under 20.
[warning] 52-52: StaticAccess: Avoid using static access to class '\Symfony\Component\PropertyAccess\PropertyAccess' in method '__construct'.
[warning] 337-337: LongVariable: Avoid excessively long variable names like $searchableEntitiesChunk. Keep variable name length under 20.
[warning] 385-385: StaticAccess: Avoid using static access to class '\Doctrine\ORM\Proxy\DefaultProxyClassNameResolver' in method 'resolveClass'.
[warning] 389-389: StaticAccess: Avoid using static access to class '\Doctrine\Common\Util\ClassUtils' in method 'resolveClass'.
🪛 Gitleaks (8.28.0)
tests/config/framework.yaml
[high] 3-3: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (15)
.php-cs-fixer.dist.php (1)
16-16: LGTM! Valid rule set update.The change from
@PHP80Migration:riskyto@PHP8x0Migration:riskyis valid, as@PHP8x0Migration:riskyis an officially supported PHP-CS-Fixer rule set. This broadens the migration rules to cover all PHP 8.x versions (8.0, 8.1, 8.2, etc.) rather than being limited to PHP 8.0 specifically, which aligns with PHP-CS-Fixer's naming convention for version-agnostic rule sets.phpstan.dist.neon (1)
10-10: Suppression is acceptable for PHPStan v2.x upgrade.This ignore pattern addresses a new type-checking constraint introduced by PHPStan v2.x regarding
array_uniqueparameter types. While suppressing the error is appropriate for this PR focused on DoctrineBundle v3 compatibility, consider addressing the underlying type issue in a follow-up if the aggregator array handling can be made more type-safe.src/Engine.php (1)
114-117: LGTM! Return type correctly tightened to non-nullable array.The change from
?arraytoarrayis correct and aligns with the meilisearch-php v1.16 contract fordeleteIndex(). Past reviews confirmed the interface was updated to match.src/Services/MeilisearchService.php (2)
140-143: LGTM! Return type correctly updated to non-nullable array.The change from
?arraytoarrayfordeleteByIndexNameis correct and consistent with the underlying Engine implementation.
145-150: LGTM! Return type correctly updated to non-nullable array.The change from
?arraytoarrayfordeleteis correct and aligns with the Engine's delete method return type.tests/config/doctrine.yaml (1)
1-19: LGTM! Standard Doctrine test configuration.The DBAL and ORM configuration is appropriate for the test environment. Using SQLite for tests and registering the custom
DummyObjectIdTypealigns with standard testing practices.tests/baseline-ignore (1)
11-18: LGTM! Appropriate deprecation suppressions for DoctrineBundle v3 migration.The added patterns correctly suppress expected deprecation warnings during the DoctrineBundle v2 → v3 transition. These include configuration changes, parameter nullability, return type declarations, and Doctrine Collections compatibility notices that are inherent to the upgrade path.
.github/workflows/tests.yml (5)
34-34: Matrix simplification noted.The Symfony version matrix has been intentionally reduced to focus testing. The commented versions suggest this may be expanded in the future.
63-78: LGTM! Dependency matrix strategy is well-structured.The include entries properly test edge cases with
lowestandhighestdependencies across key PHP/Symfony combinations, providing good coverage beyond the locked dependency baseline.
80-80: LGTM! Job name correctly reflects matrix dimensions.The conditional formatting
${{ matrix.dependencies != 'locked' && format(' ({0})', matrix.dependencies) || '' }}cleanly appends the dependency state to the job name only when relevant.
91-92: Validation scope reduced to composer.json only.The validation step no longer checks
composer.lock. Ensure this is intentional, as validating the lock file helps catch inconsistencies between dependencies and their locked versions.
103-104: LGTM! Dependency installation correctly uses matrix values.The
dependency-versionsparameter now dynamically uses the matrix value, enabling the testing of locked, lowest, and highest dependency scenarios.composer.json (3)
23-23: LGTM! DoctrineBundle v3 support successfully added.The constraint
^2.10 || ^3.0achieves the PR's primary objective by adding compatibility with DoctrineBundle v3 while maintaining v2 support.
24-24: LGTM! Meilisearch PHP client updated to v1.16.The significant version bump from
^1.0.0to^1.16supports the non-nullable array return types introduced inEngineandMeilisearchService. This ensures compatibility with the client's current contract.
43-46: LGTM! PHPStan v2.x upgrade coordinated with codebase changes.The PHPStan and related extension upgrades to v2.x align with the suppression added in
phpstan.dist.neonand support the overall dependency modernization effort.
norkunas
force-pushed
the
doctrinebundlev3
branch
9 times, most recently
from
5c4a983 to
ab85715
Compare
|
@Strift @curquiza @brunoocasali CI is green at last here, except codecov.. but with a cost. Had to update I've added lowest/highest dependency runs so we can prove that we properly support doctrine bundle v2/v3 |
norkunas
force-pushed
the
doctrinebundlev3
branch
from
ab85715 to
dc4a894
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/Services/MeilisearchService.php (1)
140-143: Confirm interface is updated to non-nullable array.deleteByIndexName now returns array; ensure SearchService::deleteByIndexName also returns array (not ?array). Re-run a quick check.
#!/bin/bash # Find nullable array return types on delete methods in interfaces/classes. rg -nP --type=php -C2 $'interface\\s+SearchService|class\\s+.*Service' && \ rg -nP --type=php $'function\\s+delete(ByIndexName)?\\s*\\([^)]*\\)\\s*:\\s*\\?array'tests/config/framework.yaml (1)
3-3: Use a clearly non-secret test value to silence secret scanners.The secret value triggers false positives in secret scanning tools. Consider using an obviously fake value like
'test_secret_value'or an environment variable reference.
🧹 Nitpick comments (6)
.github/workflows/tests.yml (3)
25-31: Pin Meilisearch image for reproducibility.latest is non-deterministic; pin a version tag or digest.
- image: getmeili/meilisearch:latest + image: getmeili/meilisearch:v1 # or a specific digest for full reproducibility
31-36: Consider disabling fail-fast on the test matrix.Prevents canceling other axes when one combo fails.
strategy: matrix: php-version: ['7.4', '8.1', '8.2', '8.3', '8.4'] sf-version: ['5.4', '6.4', '7.3', '7.4', '8.0'] dependencies: ['default'] + fail-fast: false
94-101: Validate composer.json after mutating it.Run composer validate again post-sed to fail fast on JSON mistakes.
- name: Remove doctrine/annotations if: matrix.php-version != '7.4' run: sed -i '/doctrine\/annotations/d' composer.json + + - name: Re-validate composer.json + if: matrix.php-version != '7.4' + run: composer validatetests/baseline-ignore (1)
5-18: Scope and time‑box deprecation ignores.These broad patterns can mask real issues. Prefer narrower regex (anchor to specific packages/versions) and add a removal plan (e.g., TODO with target date/version).
Would you like a follow-up PR to split these ignores per package/version and add comments with expiry dates?
composer.json (2)
23-25: Runtime requirement bump: document in CHANGELOG/UPGRADE.Raising meilisearch/meilisearch-php to ^1.16 is a runtime change; add a brief note for users.
I can draft a CHANGELOG entry and UPGRADE note if helpful.
80-81: Add prefer-stable (or limit beta to CI).minimum-stability: beta can pull less stable deps. Consider prefer-stable: true, or constrain beta to CI only.
"scripts": { "lint:fix": "./vendor/bin/php-cs-fixer fix -v --using-cache=no" }, - "minimum-stability": "beta" + "minimum-stability": "beta", + "prefer-stable": true }If beta is only needed to test Symfony 8.0 prereleases, we can avoid committing it by injecting stability via SYMFONY_REQUIRE in CI.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/workflows/tests.yml(4 hunks).php-cs-fixer.dist.php(1 hunks)composer.json(3 hunks)phpstan.dist.neon(1 hunks)src/Engine.php(1 hunks)src/Services/MeilisearchService.php(1 hunks)tests/Kernel.php(2 hunks)tests/baseline-ignore(1 hunks)tests/config/doctrine.yaml(1 hunks)tests/config/doctrine_old_proxy.yaml(0 hunks)tests/config/doctrine_php7.yaml(0 hunks)tests/config/doctrine_v2.yaml(0 hunks)tests/config/framework.yaml(1 hunks)
💤 Files with no reviewable changes (3)
- tests/config/doctrine_old_proxy.yaml
- tests/config/doctrine_php7.yaml
- tests/config/doctrine_v2.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- phpstan.dist.neon
- .php-cs-fixer.dist.php
- tests/config/doctrine.yaml
🧰 Additional context used
🧬 Code graph analysis (2)
src/Services/MeilisearchService.php (2)
src/SearchService.php (2)
deleteByIndexName(47-47)delete(45-45)src/Engine.php (1)
delete(114-117)
src/Engine.php (2)
src/Services/MeilisearchService.php (1)
delete(145-150)src/SearchService.php (1)
delete(45-45)
🪛 Gitleaks (8.28.0)
tests/config/framework.yaml
[high] 3-3: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (8)
.github/workflows/tests.yml (2)
101-108: Matrix-driven dependency-versions wiring looks good.
112-116: Artifact names include dependencies context.Nice touch; helps Codecov merging by dimension.
src/Services/MeilisearchService.php (1)
145-150: Return-type covariance is correct; no issues found.Verification confirms the return type change is valid and safe. MeilisearchService::delete returns non-nullable
array, which is a valid covariant narrowing of the SearchService interface's nullable?arraydeclaration. The implementation delegates to Engine::delete, which also returns non-nullablearray. No callers in the codebase check for null results, so no breaking changes exist.tests/Kernel.php (5)
8-8: LGTM - Appropriate version detection import.Adding
BlacklistSchemaAssetFilterfor version detection is sound, as this class was removed in DoctrineBundle v3.
32-32: LGTM - Framework config loaded first.Loading
framework.yamlat the start establishes base configuration before Doctrine, which is the correct order.
34-34: LGTM - Clean version detection.Using the absence of
BlacklistSchemaAssetFilteris a reliable way to detect DoctrineBundle v3, as this class was removed in that version.
36-52: LGTM - Well-structured config loading logic.The branching logic cleanly separates:
- DoctrineBundle v3 (line 38)
- v2 with PHP 8.4+ supporting legacy reflection (line 40)
- Older proxy configurations (line 42)
- PHP 7 specific setup (lines 45-51)
This provides good maintainability and clear version/environment segregation.
55-62: LGTM - Properly guards v2-specific configuration.The addition of
!$doctrineBundleV3correctly ensures that v2-specific ORM settings (report_fields_where_declared,validate_xml_mapping) are not applied to DoctrineBundle v3, preventing potential conflicts.
| public function delete(string $indexUid): array | ||
| { | ||
| return $this->client->deleteIndex($indexUid); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
LGTM; align docblock and verify call sites.
Signature to array is good. Consider adding @throws ApiException for parity with other methods.
/**
- * Delete an index and its content.
+ * Delete an index and its content.
+ *
+ * @throws ApiException
*/
public function delete(string $indexUid): array
{
return $this->client->deleteIndex($indexUid);
}Quick scan for nullable expectations:
🏁 Script executed:
#!/bin/bash
rg -nP --type=php $'->delete\\(' -C2 && rg -nP --type=php $'function\\s+delete\\s*\\([^)]*\\)\\s*:\\s*\\?array'Length of output: 404
I'll generate corrected shell scripts to verify the call sites and return type expectations for the delete() method.
🏁 Script executed:
#!/bin/bash
# Search for calls to delete() method
echo "=== Searching for delete() method calls ==="
rg -nP --type=php '\->delete\(' -C2
echo ""
echo "=== Searching for nullable array return types ==="
rg -nP --type=php 'function\s+delete\s*\([^)]*\)\s*:\s*\?array' -C2Length of output: 1043
🏁 Script executed:
#!/bin/bash
# Check SearchService.php structure
echo "=== SearchService.php class/interface declaration ==="
rg -nP --type=php '^(class|interface)\s+SearchService' -A5
echo ""
echo "=== Engine.php class declaration and implements ==="
rg -nP --type=php '^class\s+Engine' -A3
echo ""
echo "=== MeilisearchService.php class declaration ==="
rg -nP --type=php '^class\s+MeilisearchService' -A3
echo ""
echo "=== Full delete() signatures in SearchService ==="
cat src/SearchService.php | head -50Length of output: 1629
🏁 Script executed:
#!/bin/bash
# Get full Engine class declaration
echo "=== Engine.php class declaration with full context ==="
head -130 src/Engine.php | tail -40
echo ""
echo "=== Search for Engine class and implements clause ==="
rg -nP --type=php 'class\s+Engine\s*' -A10
echo ""
echo "=== Check if Engine is type-hinted as SearchService anywhere ==="
rg -nP --type=php 'SearchService' --type=php -C3Length of output: 13197
🏁 Script executed:
#!/bin/bash
# Get MeilisearchService delete() method signatures
echo "=== MeilisearchService delete methods ==="
rg -nP --type=php 'public function delete' src/Services/MeilisearchService.php -A3
echo ""
echo "=== Full context of MeilisearchService delete methods ==="
sed -n '136,152p' src/Services/MeilisearchService.phpLength of output: 786
Fix MeilisearchService return types to comply with SearchService contract.
The review comment addresses Engine.php in isolation, but there's a critical type safety violation: MeilisearchService implements SearchService yet returns non-nullable array where the interface defines nullable ?array for both delete() and deleteByIndexName() methods. This violates the Liskov Substitution Principle.
Engine.php changes are acceptable (Engine does not implement SearchService), but MeilisearchService methods at lines 140 and 145 must return ?array to satisfy the SearchService interface contract:
- Line 140:
public function deleteByIndexName(string $indexName): ?array - Line 145:
public function delete(string $className): ?array
🤖 Prompt for AI Agents
In src/MeilisearchService.php around lines 140–145, the method signatures
currently return non-nullable array but must match the SearchService interface
which declares nullable ?array for deleteByIndexName and delete; update the two
method signatures to return ?array (public function deleteByIndexName(string
$indexName): ?array and public function delete(string $className): ?array),
adjust any returns inside those methods to return null where appropriate instead
of an array or throw, and update PHPDoc/comments to reflect the nullable return
type so the implementation complies with the interface and Liskov Substitution
Principle.
e6db257 to
42915a6
Compare
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)
src/Services/MeilisearchService.php (1)
140-149: Update SearchService interface to match implementation return types.The implementation signatures don't match the interface. The
SearchServiceinterface declares bothdelete()anddeleteByIndexName()with?arrayreturn types (lines 45, 47), but theMeilisearchServiceimplementation declares both with non-nullablearrayreturn types (lines 140, 145).The implementation is correct—both methods delegate to
$this->engine->delete(), which returns a non-nullablearray, with no null handling. The interface insrc/SearchService.phpshould be updated to declarearrayinstead of?arrayfor both methods to align with the implementation and reflect the actual contract.
♻️ Duplicate comments (1)
tests/config/framework.yaml (1)
3-3: Past review comment still applies: Use a clearly non-secret test value.The hex string triggers secret scanners. Consider using
'test_secret_value'or an env var reference as suggested in the previous review.
🧹 Nitpick comments (1)
.github/workflows/tests.yml (1)
14-15: Consider removing or documenting the commented env block.The commented-out
fail-fast: truesetting suggests intentional debugging or testing behavior. Either remove it if no longer needed, or add a brief comment explaining why it's preserved.Apply this diff to remove if obsolete:
-#env: -# fail-fast: true -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/workflows/tests.yml(4 hunks).php-cs-fixer.dist.php(1 hunks)composer.json(2 hunks)phpstan.dist.neon(1 hunks)src/Engine.php(1 hunks)src/Services/MeilisearchService.php(1 hunks)tests/Kernel.php(2 hunks)tests/baseline-ignore(1 hunks)tests/config/doctrine.yaml(1 hunks)tests/config/doctrine_old_proxy.yaml(0 hunks)tests/config/doctrine_php7.yaml(0 hunks)tests/config/doctrine_v2.yaml(0 hunks)tests/config/framework.yaml(1 hunks)
💤 Files with no reviewable changes (3)
- tests/config/doctrine_v2.yaml
- tests/config/doctrine_old_proxy.yaml
- tests/config/doctrine_php7.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/config/doctrine.yaml
- .php-cs-fixer.dist.php
- src/Engine.php
- composer.json
🧰 Additional context used
🧬 Code graph analysis (1)
src/Services/MeilisearchService.php (2)
src/SearchService.php (2)
deleteByIndexName(47-47)delete(45-45)src/Engine.php (1)
delete(114-117)
🪛 Gitleaks (8.28.0)
tests/config/framework.yaml
[high] 3-3: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (9)
phpstan.dist.neon (1)
10-10: LGTM! PHPStan 2.x suppression is appropriately targeted.The ignoreErrors pattern correctly suppresses a type strictness issue introduced by the PHPStan v2 upgrade for the Aggregator entity handling.
tests/baseline-ignore (2)
5-6: LGTM! Deprecation patterns consolidated effectively.The regex-based grouping of related Doctrine ORM Configuration deprecations improves maintainability compared to separate entries.
9-18: LGTM! New baseline entries align with v3 compatibility.The additional deprecation suppressions appropriately handle DoctrineBundle 3.0, PHP 8.4, and return-type-related notices expected during the transition period.
tests/Kernel.php (3)
32-34: LGTM! Centralized framework config and clean v3 detection.Loading framework.yaml first centralizes configuration, and the BlacklistSchemaAssetFilter class check is an appropriate way to detect DoctrineBundle v3 (since the class was removed in v3).
36-52: LGTM! Version-aware config loading handles PHP and Doctrine variations.The branching logic correctly:
- Routes PHP 8+ to version-specific Doctrine configs (v3, v2 with PHP 8.4+, or old proxy)
- Configures PHP 7.4 with appropriate framework settings and dedicated config file
The strategy ensures compatibility across the supported version matrix.
55-62: LGTM! Doctrine v2-specific config correctly gated.The
report_fields_where_declaredoption is appropriately applied only when NOT using DoctrineBundle v3, preventing configuration errors with v3 where this option was removed..github/workflows/tests.yml (3)
34-35: LGTM! Comprehensive dependency matrix demonstrates v2/v3 compatibility.The matrix strategy effectively:
- Tests default dependencies across all supported PHP/Symfony combinations
- Adds explicit lowest/highest tests for Symfony 5.4, 6.4, and 7.4
- Properly excludes incompatible PHP/Symfony pairings
- Comments out Symfony 8.0 tests (likely pending stable release)
This demonstrates DoctrineBundle v2 and v3 support across the dependency spectrum.
Also applies to: 60-88
112-117: LGTM! Composer configuration correctly handles beta releases and dependency variations.The conditional
COMPOSER_MINIMUM_STABILITY: betafor Symfony 7.4 and 8.0 appropriately enables testing of pre-release versions, whileCOMPOSER_PREFER_STABLE: 1ensures stable packages are preferred when available. Thedependency-versionsparameter correctly uses the matrix variable.
125-125: LGTM! Coverage artifact names include dependency dimension.The updated naming prevents artifact collisions between lowest/highest/default dependency runs for the same PHP/Symfony combination.
norkunas
force-pushed
the
doctrinebundlev3
branch
from
42915a6 to
4d41277
Compare
1 participant
Pull Request
Related issue
Fixes #399
What does this PR do?
PR checklist
Please check if your PR fulfills the following requirements:
Summary by CodeRabbit