Add support for doctrine bundle v3 #400
Conversation
WalkthroughAdds a CI Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(242,247,255,0.9)
participant CI as GitHub Actions
participant Job as Matrix Job
participant Composer as Composer
CI->>Job: Start (php-version, symfony, matrix.dependencies)
Job->>Composer: Install using matrix.dependency-versions + matrix.composer-options
Composer-->>Job: Dependencies installed
Job->>Job: Run tests, collect coverage
Job-->>CI: Upload coverage artifact (includes matrix.dependencies)
end
sequenceDiagram
rect rgba(242,247,255,0.9)
participant Kernel as tests/Kernel::configureContainer
participant Loader as Config loader
Kernel->>Kernel: Load `tests/config/framework.yaml`
Kernel->>Kernel: Check for 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 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
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 (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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.38% 88.38%
=========================================
Files 20 20
Lines 878 878
=========================================
Hits 776 776
Misses 102 102 ☔ 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
norkunas
force-pushed
the
doctrinebundlev3
branch
8 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.
norkunas
force-pushed
the
doctrinebundlev3
branch
5 times, most recently
from
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
Strift
left a comment
Strift
left a comment
There was a problem hiding this comment.
Thanks for this PR. I only have one small question. The rest is good for me.
norkunas
dismissed
Strift’s stale review
The merge-base changed after approval.
norkunas
force-pushed
the
doctrinebundlev3
branch
from
4d41277 to
c6f70ef
Compare
Thanks, i've rebased to fix git conflict, so will merge later if CI will be ok |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/config/framework.yaml (1)
3-3: Use a clearly non-secret test value to silence security scanners.The hex string
67d829bf61dc5f87a73fd814e2c9f629triggers secret scanners (Gitleaks flagged this as a potential API key). While this appears to be a test value, using an obviously fake placeholder prevents false positives in security tooling.Apply this diff:
framework: test: true - secret: 67d829bf61dc5f87a73fd814e2c9f629 + secret: 'test_secret_not_for_production' http_method_override: falseAlternatively, use an environment variable:
- secret: 67d829bf61dc5f87a73fd814e2c9f629 + secret: '%env(default::APP_SECRET)%'
🧹 Nitpick comments (2)
src/Engine.php (1)
114-117: Return type change is valid; consider adding @throws for consistency.The change from
?arraytoarrayis appropriate sincedeleteIndex()appears to never returnnull. For consistency with other methods (likeclear()at line 102), consider adding@throws ApiExceptionto the docblock.Apply this diff to add the annotation:
/** * Delete an index and its content. + * + * @throws ApiException */ public function delete(string $indexUid): array.github/workflows/tests.yml (1)
14-15: Remove or document the commented env block.The commented-out
fail-fastenvironment variable should either be removed if no longer needed, or uncommented with a clear explanation of its purpose.Apply this diff to remove it:
-#env: -# fail-fast: true -
📜 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_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)
- tests/config/doctrine.yaml
- .php-cs-fixer.dist.php
- phpstan.dist.neon
🧰 Additional context used
🧬 Code graph analysis (3)
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)
tests/Kernel.php (1)
src/DependencyInjection/MeilisearchExtension.php (1)
load(19-64)
🪛 Gitleaks (8.29.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 (11)
src/Services/MeilisearchService.php (1)
140-150: Return type narrowing is valid and consistent with Engine changes.The change from
?arraytoarrayfor both methods is technically correct—PHP allows covariant return types where implementations can be more specific than their interface. SinceEngine::delete()now returnsarray, these methods correctly reflect that guarantee. This approach also aligns with PHPStan's preference for non-nullable types whennullis never returned, reducing false positives in static analysis.composer.json (2)
23-24: Verify backward compatibility with meilisearch-php ^1.16 minimum.The jump from
^1.0to^1.16drops support for 15 minor versions. While the PR description explains this change enables newer MeiliSearch endpoints required by tests, ensure that:
- No users are locked on older meilisearch-php versions that are incompatible with this bundle
- The minimum version
1.16.0is thoroughly tested with the lowest dependency matrix run- CHANGELOG or UPGRADE notes document this breaking change
The doctrine-bundle constraint
^2.10 || ^3.0correctly enables v3 support while maintaining v2 compatibility.
37-52: LGTM: Dev dependency updates align with expanded test matrix.The dev dependency updates (phpstan 2.x, php-cs-fixer 3.88, phpunit-bridge 7.3, etc.) appropriately support testing against newer PHP and Symfony versions while maintaining compatibility with the expanded CI matrix.
tests/baseline-ignore (1)
1-18: LGTM: Deprecation baseline properly handles Doctrine v2/v3 transition.The added deprecation patterns appropriately suppress expected warnings during the DoctrineBundle v2/v3 transition period:
- Doctrine ORM proxy/lazy object deprecations (lines 5-7)
- DoctrineBundle 3.0 config deprecations (lines 8-9)
- PHP 8.x return type compatibility notices (lines 13-18)
These suppressions are reasonable for maintaining clean test output while supporting both bundle versions.
.github/workflows/tests.yml (3)
33-35: LGTM: Matrix expansion properly tests DoctrineBundle v2 and v3.The added
dependenciesmatrix dimension withlowestandhighestcombinations ensures proper testing of both DoctrineBundle v2 (likely in lowest) and v3 (in highest), addressing the PR objective.Also applies to: 63-78
91-91: Appropriate validation change for Flex-based testing.Validating only
composer.json(notcomposer.lock) is correct for a matrix that uses Symfony Flex to dynamically resolve different version combinations.
103-104: LGTM: Dependency handling and artifact naming correctly use matrix variable.The changes properly:
- Pass
${{ matrix.dependencies }}to the composer install action (line 104)- Include the dependencies value in artifact names (line 112)
This ensures each matrix combination is tested with the correct dependency set and produces uniquely named coverage artifacts.
Also applies to: 112-112
tests/Kernel.php (4)
32-32: Good refactoring: Framework config centralized.Loading
framework.yamlseparately (line 32) properly centralizes framework configuration that was previously scattered across multiple config files, improving maintainability.
36-52: LGTM: Conditional config loading properly handles PHP and Doctrine version combinations.The refactored configuration loading correctly:
- Detects DoctrineBundle v3 and loads appropriate config (line 37-38)
- Falls back to v2-specific configs based on PHP version and ORM features (lines 39-42)
- Handles PHP 7 with separate framework prepends and config (lines 45-51)
This ensures compatibility across the supported PHP 7.4-8.4 and DoctrineBundle 2.10-3.0 version ranges.
55-62: LGTM: Correctly guards v2-only configuration.The
!$doctrineBundleV3condition (line 55) appropriately prevents applying thereport_fields_where_declaredconfiguration to DoctrineBundle v3, where this option no longer exists (as noted in the deprecation baseline).
34-34: Detection logic verified as correct.BlacklistSchemaAssetFilter was removed in DoctrineBundle v3.0 as a documented breaking change. The
!class_exists(BlacklistSchemaAssetFilter::class)approach reliably distinguishes versions: the class exists in v2 (returns false), and is absent in v3 (returns true). The detection is used consistently throughout the file for conditional configuration loading, making it a maintainable and appropriate mechanism for the version check.
norkunas
force-pushed
the
doctrinebundlev3
branch
from
c6f70ef to
78968db
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/config/framework.yaml (1)
1-4: Secret scanner noise remains unaddressed.The secret value on Line 3 continues to trigger Gitleaks warnings. While this is test configuration, addressing the past review comment would reduce CI noise.
🧹 Nitpick comments (1)
src/Engine.php (1)
114-117: Return type change is correct.The narrowing from
?arraytoarrayaccurately reflects thatdeleteIndex()never returns null—it either returns an array or throws anApiException. This aligns with the discussion in previous review comments.Optional: Consider documenting the exception.
For completeness, you could add
@throws ApiExceptionto the docblock:/** * Delete an index and its content. + * + * @throws ApiException */ public function delete(string $indexUid): array
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/workflows/tests.yml(5 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 (4)
- .php-cs-fixer.dist.php
- phpstan.dist.neon
- tests/config/doctrine.yaml
- src/Services/MeilisearchService.php
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: norkunas
Repo: meilisearch/meilisearch-symfony PR: 391
File: src/Command/MeilisearchImportCommand.php:117-120
Timestamp: 2025-08-05T04:46:01.119Z
Learning: In meilisearch-symfony project, the maintainer norkunas prefers to keep PRs focused and defer error handling improvements for settings updates to separate PRs, particularly when it involves decisions about partial import behavior when some indexes fail.
🧬 Code graph analysis (2)
src/Engine.php (2)
src/Services/MeilisearchService.php (1)
delete(145-150)src/SearchService.php (1)
delete(45-45)
tests/Kernel.php (1)
src/DependencyInjection/MeilisearchExtension.php (1)
load(19-64)
🪛 Gitleaks (8.29.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)
composer.json (2)
23-24: LGTM! Core dependency updates align with PR objectives.The broadened DoctrineBundle constraint and the meilisearch-php version bump are well-justified and support the PR's goal of adding DoctrineBundle v3 compatibility.
37-52: No action required — symfony/phpunit-bridge 7.3 is compatible with Symfony 5.4.The PHPUnit Bridge is designed to work across maintained Symfony releases and doesn't force a matching Symfony major version. The constraint update is safe.
tests/Kernel.php (3)
32-34: LGTM! Clean version detection and config centralization.Loading
framework.yamlfirst centralizes framework configuration, and detecting DoctrineBundle v3 via the absence ofBlacklistSchemaAssetFilteris a reliable approach.
36-52: LGTM! Conditional config loading handles version combinations well.The branching logic cleanly handles DoctrineBundle v3, v2 with different proxy mechanisms, and PHP 7 requirements. The version detection through class existence checks is reliable.
55-62: LGTM! Guards v2-specific ORM config from v3.Adding
!$doctrineBundleV3to the condition correctly prevents applying DoctrineBundle v2-specific ORM configuration when using v3..github/workflows/tests.yml (2)
30-75: LGTM! Matrix expansion enables comprehensive dependency testing.The new
dependenciesdimension with strategiclowestandhighestcombinations effectively tests DoctrineBundle v2 and v3 compatibility across key PHP and Symfony versions.
88-89: Change is appropriate for this PHP library.The repository intentionally excludes
composer.lockvia.gitignore, so validating onlycomposer.jsonis the correct approach. Lock files are not committed in PHP libraries, making the simplified validation step consistent with project structure.tests/baseline-ignore (1)
1-18: LGTM! Baseline updates align with dependency upgrades.The deprecation suppressions appropriately handle known issues during the transition to DoctrineBundle v3, PHPStan 2.x, and newer Symfony versions. These patterns will reduce test noise while the ecosystem catches up.
2 participants
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