Add support for Symfony 8 #419
Conversation
WalkthroughBumps base PHP image to php:8.5-fpm, broadens Composer Symfony constraints to include ^8.0, migrates serializer Groups imports in tests to the Attribute namespace, upgrades PHPUnit/dev tooling and phpunit config, updates CI matrices to include Symfony 8.0 with exclusions, and raises README PHP/Symfony requirements. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Caution Docstrings generation - FAILED No docstrings were generated. |
|
I updated composer.json to drop support for symfony 5.4 and php =< 8.1 as proposed here #402 (comment) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
composer.json(2 hunks)
🔇 Additional comments (1)
composer.json (1)
25-31: Symfony 8 support is available and stable.All listed Symfony packages now have 8.0 releases on Packagist and can be reliably included in dependencies. The addition of
|| ^8.0to these core components aligns with current package availability. Note that internal package dependencies may use^7.4|^8.0constraints rather than the broader^6.0 || ^7.0 || ^8.0shown here, which is acceptable for supporting the 8.0 release cycle while maintaining compatibility.
unicolored
force-pushed
the
symfony-8-upgrade
branch
from
574843b to
25a7446
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
composer.json (2)
21-21: Redundant PHP version constraint.The constraint
^8.2 || ^8.4is redundant—^8.2already includes 8.2.x, 8.3.x, 8.4.x, and beyond. The|| ^8.4portion is a subset and adds nothing. Consider simplifying to^8.2.Note: The previous review suggesting
^8.4alone would be incorrect here since the bundle still supports Symfony 6/7, which work on PHP 8.2/8.3. Symfony 8's own packages enforcephp: ^8.4, so Composer will naturally prevent invalid combinations.- "php": "^8.2 || ^8.4", + "php": "^8.2",
29-29: Consider removing unnecessary polyfill.
symfony/polyfill-php80provides backports of PHP 8.0 features for older PHP versions. Since the minimum PHP requirement is now^8.2, this polyfill is no longer necessary and can be removed.- "symfony/polyfill-php80": "^1.27",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
composer.json(2 hunks)
🔇 Additional comments (2)
composer.json (2)
48-54: Symfony 8 support in require-dev looks good.The
symfony/http-clientnow correctly includes|| ^8.0(addressing the previous review comment), and other Symfony dev dependencies are consistently updated.
25-31: Symfony 8 constraints properly added to require section.All core Symfony dependencies now include
|| ^8.0, enabling Symfony 8 compatibility while maintaining support for Symfony 6 and 7.
|
Thanks for your PR. I'd like to merge #410 before doing this, so we'll open additional extension points for those who cannot upgrade yet. |
|
Also please check coderabbit nitpicks, there are good suggestions:) |
|
Can you rebase? |
- Switch Dockerfile to php 8.4 - Add symfony || ^8.0 to composer.json - Update PHP_VERSION_ID limit to 80100 into one test to avoid issue - Fix use of Groups Annotation to Attribute in some tests entities => test:unit success except 1 skipped (proxy)
unicolored
force-pushed
the
symfony-8-upgrade
branch
from
25a7446 to
2f5ab86
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
composer.json (1)
51-51: Add|| ^8.0constraint to symfony/phpunit-bridge for consistency.All other Symfony dev dependencies include
|| ^8.0support, butsymfony/phpunit-bridgeremains at^7.3. For consistency and to ensure compatibility with Symfony 8 projects, update to"symfony/phpunit-bridge": "^7.3 || ^8.0".Apply this diff:
- "symfony/phpunit-bridge": "^7.3", + "symfony/phpunit-bridge": "^7.3 || ^8.0",
🧹 Nitpick comments (1)
composer.json (1)
28-28: Remove unnecessary polyfill dependency.The
symfony/polyfill-php80package provides PHP 8.0 features to PHP 7.x projects. Since this project requiresPHP ^8.1(line 21), this polyfill serves no purpose and adds unnecessary overhead.Apply this diff to remove the unnecessary dependency:
- "symfony/polyfill-php80": "^1.27",
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Dockerfile(1 hunks)composer.json(2 hunks)tests/Entity/Comment.php(1 hunks)tests/Entity/DummyCustomGroups.php(1 hunks)tests/Entity/Page.php(1 hunks)tests/Entity/Post.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/Entity/Page.php
- tests/Entity/Comment.php
🧰 Additional context used
🧠 Learnings (3)
📓 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.
📚 Learning: 2025-12-15T19:07:07.476Z
Learnt from: norkunas
Repo: meilisearch/meilisearch-symfony PR: 410
File: src/Identifier/DefaultIdNormalizer.php:18-22
Timestamp: 2025-12-15T19:07:07.476Z
Learning: In PHP projects (Symfony style), avoid importing global PHP classes (e.g., \InvalidArgumentException, \LogicException) via use statements inside namespaces. Instead, reference them with a leading backslash (fully qualified name) when used, e.g. \InvalidArgumentException $e = new \InvalidArgumentException('msg'); This preserves clarity about the global namespace and follows Symfony coding conventions for global classes.
Applied to files:
tests/Entity/Post.phptests/Entity/DummyCustomGroups.php
📚 Learning: 2025-12-15T19:07:18.247Z
Learnt from: norkunas
Repo: meilisearch/meilisearch-symfony PR: 410
File: src/Identifier/DefaultIdNormalizer.php:18-22
Timestamp: 2025-12-15T19:07:18.247Z
Learning: In the meilisearch-symfony project, global PHP classes (like \InvalidArgumentException, \LogicException, etc.) should not be imported via use statements. Instead, use the fully qualified class name with a leading backslash, following Symfony coding standards.
Applied to files:
composer.json
🔇 Additional comments (5)
tests/Entity/DummyCustomGroups.php (1)
9-9: LGTM: Correct migration to attribute-based Groups.The change from
Annotation\GroupstoAttribute\Groupsaligns with PHP 8+ attribute support and Symfony Serializer 6+ conventions.Dockerfile (1)
1-1: LGTM: PHP 8.4 base image supports latest features.Upgrading to
php:8.4-fpmenables testing with the latest PHP version and supports the attribute-based syntax used throughout the test entities.tests/Entity/Post.php (1)
11-11: LGTM: Consistent attribute-based migration.The import change to
Attribute\Groupsis consistent with the other test entities and follows PHP 8+ conventions.composer.json (2)
24-30: LGTM: Symfony 8 support correctly added.The addition of
|| ^8.0constraints to Symfony runtime dependencies enables proper Symfony 8 compatibility.
47-50: LGTM: Dev dependencies updated for Symfony 8 compatibility.The addition of
|| ^8.0constraints tosymfony/doctrine-bridge,symfony/filesystem,symfony/framework-bundle,symfony/http-client,symfony/yaml, andsymfony/var-exporterenables proper Symfony 8 development environment support.Also applies to: 52-53
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #419 +/- ##
==========================================
+ Coverage 72.38% 72.56% +0.17%
==========================================
Files 28 28
Lines 1224 1221 -3
==========================================
Hits 886 886
+ Misses 338 335 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Done! Thanks for your comments. test:unit results: |
unicolored
force-pushed
the
symfony-8-upgrade
branch
from
2f5ab86 to
1fe5283
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
composer.json (1)
28-28: Consider removing the redundant polyfill.Since the package requires PHP 8.1+ (line 21), all PHP 8.0 features are already available natively. The
symfony/polyfill-php80dependency appears unnecessary. Was this added to address a specific requirement or transitive dependency issue?If not needed, apply this diff to remove the redundant dependency:
- "symfony/polyfill-php80": "^1.27",
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
composer.json(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 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.
📚 Learning: 2025-12-15T19:07:18.247Z
Learnt from: norkunas
Repo: meilisearch/meilisearch-symfony PR: 410
File: src/Identifier/DefaultIdNormalizer.php:18-22
Timestamp: 2025-12-15T19:07:18.247Z
Learning: In the meilisearch-symfony project, global PHP classes (like \InvalidArgumentException, \LogicException, etc.) should not be imported via use statements. Instead, use the fully qualified class name with a leading backslash, following Symfony coding standards.
Applied to files:
composer.json
🔇 Additional comments (1)
composer.json (1)
24-30: LGTM! Symfony 8 support added consistently.All Symfony package constraints have been properly updated to include
|| ^8.0support. The version patterns are appropriate, with some packages using more specific minor versions where needed (e.g.,symfony/http-clientat^6.4.15 || ^7.1.8 || ^8.0).Also applies to: 47-53
norkunas
left a comment
norkunas
left a comment
There was a problem hiding this comment.
@unicolored you also need to update github workflows to include new symfony versions
Update php requirement Fix 6.0 to 6.4 for symfony packages
unicolored
force-pushed
the
symfony-8-upgrade
branch
2 times, most recently
from
8ccb0bd to
ff66d3b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/pre-release-tests.yml (1)
43-43: Verify Symfony 8.0 PHP version requirements (consistent with tests.yml).Similar to the main tests.yml workflow, the exclusions here restrict Symfony 8.0 to PHP 8.4 only. Please confirm the minimum PHP version requirement for Symfony 8.0 to ensure the test matrix is correctly configured.
This concern is identical to the one raised in tests.yml. The web search requested there will verify whether these exclusions are correct.
Also applies to: 54-59
🧹 Nitpick comments (2)
.github/workflows/tests.yml (1)
46-55: Consider adding explicit Symfony 8.0 test combinations.The include section tests edge cases with 'lowest' and 'highest' dependencies for Symfony 6.4 and 7.3. Consider adding similar combinations for Symfony 8.0 to ensure comprehensive coverage of dependency resolution scenarios.
For example, add these entries:
- php-version: '8.4' sf-version: '8.0' dependencies: 'lowest' - php-version: '8.4' sf-version: '8.0' dependencies: 'highest'.github/workflows/pre-release-tests.yml (1)
60-69: Consider adding explicit Symfony 8.0 test combinations (consistent with tests.yml).Similar to the main tests.yml workflow, consider adding explicit test combinations for Symfony 8.0 with 'lowest' and 'highest' dependencies to ensure comprehensive coverage.
Add these entries for consistency:
- php-version: '8.4' sf-version: '8.0' dependencies: 'lowest' - php-version: '8.4' sf-version: '8.0' dependencies: 'highest'
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/pre-release-tests.yml(2 hunks).github/workflows/tests.yml(2 hunks)Dockerfile(1 hunks)composer.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- composer.json
- Dockerfile
🧰 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.
🔇 Additional comments (1)
.github/workflows/tests.yml (1)
105-105: Verify Symfony version compatibility forSYMFONY_REQUIRE: 7.4.*The code-style job references
SYMFONY_REQUIRE: 7.4.*. Confirm that this version is available and compatible with current project requirements. If 7.4 does not exist or is not intended, consider using7.3.*,8.0.*, or a more flexible constraint.
Remove polyfill-php80 Add missing 7.4
unicolored
force-pushed
the
symfony-8-upgrade
branch
from
ff66d3b to
8bfdd25
Compare
|
Based on the failing CI, you need to also bump |
|
@norkunas is phpmd still used? it rely on pdepend/pdepend which is not yet ready for php 8.1, which make Tests / integration-tests (PHP 8.4) (Symfony 8.0.*) (pull_request) Failing after 17s. |
|
I am not the one who added it. As it blocks us from supporting 8.0 i guess we could remove it. But @Strift needs to approve this move |
Ok. I try without phpmd to check it there is more to be done, until an answer. |
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)
composer.json (1)
71-71: Remove the orphaned phpmd script.The
phpmd/phpmddependency was removed fromrequire-dev(as discussed in PR comments to unblock Symfony 8 support), but this script still references./vendor/bin/phpmd, which will no longer exist. Remove this script entry to maintain consistency.Apply this diff to remove the script:
"scripts": { - "phpmd": "./vendor/bin/phpmd src text phpmd.xml", "phpstan": "./vendor/bin/phpstan",
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
composer.json(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 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.
📚 Learning: 2025-08-05T04:46:01.119Z
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.
Applied to files:
composer.json
🔇 Additional comments (4)
composer.json (4)
24-29: LGTM! Symfony runtime dependencies correctly updated.All Symfony runtime packages now include
^8.0support with consistent^6.4 || ^7.0 || ^8.0constraints, enabling the bundle to work across Symfony 6.4, 7, and 8.
45-51: LGTM! Symfony dev dependencies correctly updated.All Symfony dev packages now include
^8.0support. The version constraints are appropriate and consistent, withsymfony/phpunit-bridgecorrectly starting at^7.3(following its own versioning scheme).
34-35: No issues found. Bothmatthiasnoback/symfony-config-test(^6.1) andmatthiasnoback/symfony-dependency-injection-test(^6.2) exist on Packagist and support Symfony 8.x.
44-44: php-code-coverage ^10.0 is compatible with the project's PHP ^8.1 requirement.The upgrade is compatible with Symfony 7/8 and PHPUnit 10, which the project uses via symfony/phpunit-bridge. No compatibility issues.
unicolored
force-pushed
the
symfony-8-upgrade
branch
2 times, most recently
from
c3a385c to
7debade
Compare
Remove phpmd^2.15 dependency which is not yet compatible with php8.1 Update Requirements section
unicolored
force-pushed
the
symfony-8-upgrade
branch
from
4a55a3c to
7d8a783
Compare
|
@unicolored i guess you'll have to update phpunit.xml.dist to switch to at least phpunit ^v10, but IIRC you'll need to replace |
TestWith and DataProvider
I update to phpunit 10.5 and updates the attributes. My local test:unit is successful, let's see if it works in the matrix. |
unicolored
force-pushed
the
symfony-8-upgrade
branch
2 times, most recently
from
ef804b2 to
9c88521
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
phpunit.xml.dist (1)
20-23: Good improvement to test discovery specificity.The change from
suffix=".php"tosuffix="Test.php"is a best practice that ensures only actual test files are discovered and executed. This prevents accidentally running helper files or other PHP files in the test directories.Based on the PR context showing 68 tests running successfully, test discovery is working correctly with the new suffix.
.github/workflows/tests.yml (1)
29-29: Symfony 7.4 LTS is missing from the test matrix.Per the earlier discussion, Symfony 7.4 is an LTS release and should be included in the matrix. Add
'7.4'to thesf-versionarray and an exclusion for the PHP 8.1 + Symfony 7.4 combination (since 7.4 requires PHP 8.2+).🔎 Suggested changes:
- sf-version: ['6.4', '7.0', '7.1', '7.2', '7.3', '8.0' ] + sf-version: ['6.4', '7.0', '7.1', '7.2', '7.3', '7.4', '8.0' ]And add exclusion:
- php-version: '8.1' sf-version: '7.4'composer.json (1)
50-50: Consider broadeningsymfony/phpunit-bridgeconstraint for consistency.This is the only Symfony dev dependency pinned exclusively to
^8.0, while others allow^6.4 || ^7.0 || ^8.0. If intentional (e.g., to leverage PHPUnit 10.x features in the bridge), this is fine. Otherwise, consider using^7.3 || ^8.0to maintain consistency with the other Symfony dev packages.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/tests.yml(3 hunks)composer.json(2 hunks)phpunit.xml.dist(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 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.
📚 Learning: 2025-08-05T04:46:01.119Z
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.
Applied to files:
.github/workflows/tests.ymlcomposer.json
🔇 Additional comments (8)
phpunit.xml.dist (2)
4-8: LGTM! Required PHPUnit 10 migration.The
<coverage>element was replaced with<source>in PHPUnit 10. This change is necessary and correctly implemented.
16-16: PHPUnit 10.5 upgrade is properly configured.The version bump from 9.6 to 10.5 is appropriate for the Symfony 8 upgrade. Verification confirms no deprecated
@-style PHPUnit annotations remain in the test suite, and phpunit.xml.dist is correctly configured for PHPUnit 10.5 with valid schema references..github/workflows/tests.yml (2)
40-45: Exclusions for Symfony 8.0 are correct.Symfony 8.0 requires PHP 8.4 or higher, so the exclusions for PHP 8.1, 8.2, and 8.3 with Symfony 8.0 are correct.
119-119: LGTM!The change from
simple-phpunittophpunitaligns with the PHPUnit 10.x upgrade incomposer.json.composer.json (4)
24-29: LGTM!Broadening Symfony component constraints to include
^8.0enables installation in Symfony 8 projects while maintaining backward compatibility with 6.4 and 7.x.
44-45: LGTM!PHPUnit 10.x upgrade aligns with the migration from annotation-based data providers to PHP attributes mentioned in the PR objectives.
73-74: LGTM!Updating test scripts to use
phpunitdirectly is consistent with the PHPUnit 10.x upgrade and removal ofsimple-phpunit.
34-35: matthiasnoback packages ^6.x fully support Symfony 6.4.matthiasnoback/symfony-config-test v6.1.0 requires symfony/config: ^5.4 || ^6.4 || ^7.0 || ^8.0, and matthiasnoback/symfony-dependency-injection-test v6.2.0 requires symfony/config: ^5.4 || ^6.4 || ^7.0 || ^8.0 and symfony/dependency-injection: ^5.4 || ^6.4 || ^7.0 || ^8.0. Both packages explicitly support Symfony 6.4, so the version upgrades are compatible with the bundle's minimum supported version.
simple-phpunit does not work with phpunit 10 phpunit 10 is required by phpunit/php-code-coverage 10.1 php-code-coverage > 10.1 require php >= 8.2 Update suffix to Test.php to avoid warning Force require phpunit/phpunit 10.5 Remove boostrapFiles used by simple-phpunit
unicolored
force-pushed
the
symfony-8-upgrade
branch
from
9c88521 to
b76fb29
Compare
| matrix: | ||
| php-version: [ '8.1', '8.2', '8.3', '8.4' ] | ||
| sf-version: ['6.4', '7.0', '7.1', '7.2', '7.3' ] | ||
| sf-version: ['6.4', '7.0', '7.1', '7.2', '7.3', '8.0' ] |
| - name: PHPStan | ||
| run: | | ||
| vendor/bin/simple-phpunit --version | ||
| vendor/bin/phpunit --version |
There was a problem hiding this comment.
there is no reason run phpunit here anymore.
vendor/bin/simple-phpunit was ran only because if the phpunit was not downloaded, it couldn't find classes. now that you are added direct dependency on phpunit, it's already always available
2 participants
test:unit.test:unit). After the fix:test:unitis successful except 1 skipped (proxy).Pull Request
Related issue
Fixes #402
What does this PR do?
meilisearch/search-bundlein a Symfony 8.0 project by updating dependencies, tests, and annotations to attributes for compatibility.PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!
Summary by CodeRabbit
Chores
Tests
CI
✏️ Tip: You can customize this high-level summary in your review settings.