allow Symfony 8, drop Symfony 5 #500

garak · 2025-10-11T14:26:18Z

Copilot

Pull Request Overview

This PR updates the KnpMenuBundle to support Symfony 8 by migrating from XML-based service configuration to PHP-based configuration and updating version constraints.

Migrated all XML service configuration files to PHP format for better compatibility
Updated Symfony dependency constraints to include version 8.0
Updated CI configuration to test against Symfony 8

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/DependencyInjection/KnpMenuExtension.php	Updated to use PhpFileLoader instead of XmlFileLoader and load .php config files
config/menu.php	New PHP service configuration replacing menu.xml
config/twig.php	New PHP service configuration replacing twig.xml
config/templating.php	New PHP service configuration replacing templating.xml
composer.json	Updated Symfony version constraints to support 8.0 and updated test dependencies
.php-cs-fixer.php	Added exclusions for 'param' and 'service' functions in native function invocation
.github/workflows/build.yaml	Updated CI matrix to test Symfony 8 and removed 5.4 testing

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

tacman · 2025-10-27T22:28:49Z

Sigh. Somehow I missed this, and have done some of the same work in my branch. Glad it's done, and hope this PR get merged now that Symfony 8 beta 1 is out.

garak · 2025-10-28T08:42:01Z

Sigh. Somehow I missed this, and have done some of the same work in my branch. Glad it's done, and hope this PR get merged now that Symfony 8 beta 1 is out.

I'm interested in a possible comparison. Did you end up with the same changes? Does it work for you?

tacman · 2025-10-28T10:15:40Z

The biggest change was the XML to PHP configuration. Fortunately, AI does a good job with that.

I got stuck with getting tests to pass cs-fixer:

https://github.com/tacman/KnpMenuBundle/actions/runs/18857731525/job/53809488034

garak · 2025-10-28T11:25:52Z

I got stuck with getting tests to pass cs-fixer:

You need a change in the "native_function_invocation" configuration

tacman · 2025-10-28T11:42:25Z

Maybe a good time to learn how to properly use cs-fixer!

<?php
// see https://github.com/FriendsOfPHP/PHP-CS-Fixer

$finder = PhpCsFixer\Finder::create()
    ->exclude('vendor')
    ->in([__DIR__])
;

return (new PhpCsFixer\Config())
    ->setRiskyAllowed(true)
    ->setRules([
        '@Symfony' => true,
        '@Symfony:risky' => true,
        '@PHP71Migration:risky' => true,
        '@PHPUnit75Migration:risky' => true,
        'ordered_imports' => true,
        'declare_strict_types' => false,
        'native_function_invocation' => ['include' => ['@all']],
    ])
    ->setFinder($finder)
;

What should it be in order for tests to pass?

garak · 2025-10-28T11:53:20Z

What should it be in order for tests to pass?

It's in the diff of this very PR

stof · 2025-10-28T12:10:43Z

FYI, a migration tool for the migration of XML config to PHP configs has been implemented in https://github.com/GromNaN/symfony-config-xml-to-php

stof · 2025-10-28T12:12:22Z

.php-cs-fixer.php

        'ordered_imports' => true,
        'declare_strict_types' => false,
-        'native_function_invocation' => ['include' => ['@all']],
+        'native_function_invocation' => ['include' => ['@all'], 'exclude' => ['param', 'service']],


A better fix might be to trigger the autoloading of the PhpFileLoader class, so that the function definitions are loaded, and so PHP-CS-Fixer would know that they are not global.

stof · 2025-10-28T12:12:58Z

.github/workflows/build.yaml

                      dev: true
                    - description: 'Dev deps'
                      php: '8.4'
+                      symfony: ^8.0


this should be reverted. The dev deps job should be running with unmodified requirements.

stof · 2025-10-28T12:14:55Z

this PR does not just allow Symfony 8. It also drop supports for Symfony 5.4 without even mentioning it.

garak · 2025-10-28T12:29:53Z

this PR does not just allow Symfony 8. It also drop supports for Symfony 5.4 without even mentioning it.

We can all agree that maintaining three versions of Symfony at the same time is enough. Four is too much.

stof · 2025-10-28T12:43:37Z

@garak I'm not saying we should not drop that. But we must communicate it instead of hiding it in a PR saying it only adds support for Symfony 8 (otherwise, it might end up being missing from the changelog as well)

stof · 2025-10-28T12:44:22Z

composer.json

+        "symfony/dependency-injection": "^6.0 | ^7.0 | ^8.0",
        "symfony/deprecation-contracts": "^2.5 | ^3.3",
-        "symfony/http-kernel": "^5.4 | ^6.0 | ^7.0"
+        "symfony/http-kernel": "^6.0 | ^7.0 | ^8.0"


If we drop support for Symfony 5.4, I would bump the min version to 6.4 (i.e. the 6.x LTS version) instead of 6.0

garak · 2025-10-28T15:18:54Z

@garak I'm not saying we should not drop that. But we must communicate it instead of hiding it in a PR saying it only adds support for Symfony 8 (otherwise, it might end up being missing from the changelog as well)

We removed the changelog more than one year ago. Changes are registered in the releases page. I see your concern anyway, I'll change the PR title

stof · 2025-10-28T16:24:10Z

@garak removing the changelog.md file inside the repo does not mean we removed the changelog at all. We just changed the location of the changelog (now, the changelog is only in the description of github releases)

Chris53897 · 2025-11-06T14:55:15Z

Is it an option to split it in 2 PRs?

One to migrate config XML => php
And another for dropping support for symfony 5.4 (under discussion ?), and adding support for symfony 8

tacman · 2025-11-06T15:40:52Z

Shouldn't it be ^6.4|^7.3|^8.0? the older versions aren't supported anymore.

Chris53897 · 2025-11-06T15:52:15Z

^6.4|^7.0|^8.0 will make migration for users easier

stof · 2025-11-06T15:53:52Z

In term of maintenance for us, supporting 6.4 as our lowest bound makes us compatible with 7.0 at no maintenance cost (as 7.0 contains all the non-deprecated features of 6.4).
and this range will allow projects to use the latest version of the bundle even if they are late on the Symfony 7.x updates, which is better for us than forcing them to use an older version of the bundle that they might ask us to maintain.

tacman · 2025-11-06T20:29:22Z

So maybe I'm not understanding how Symfony maintains older versions. Let's say PHP 8.5 has removes something in Symfony 7.*. Would ALL the version of Symfony 7 get fixed, include 7.0? My understanding was that it would not, only the maintained versions would get the fix.

Or if Symfony 7.4 changes a signature, won't all the previously maintained versions get the fix as well, but not the unmaintained ones?

https://symfony.com/releases/7.0

I understand that if the bundle does actually support Symfony 7.0 there's no reason to require a higher version.

I guess I'm thinking back to all the bundles that didn't extend AbstractBundle because they wanted to support 6.0 even after it became unmaintained.

stof · 2025-11-06T20:35:43Z

The issue is not about whether 7.0.x still receives bug fixes. As long as we don't rely on a specific fix, there is no reason for us to be very strict (this bundle won't be a sufficient reason for projects to upgrade Symfony if they are using an unmaintained Symfony version).

dmaicher · 2025-11-23T15:17:26Z

Any chance this could be merged? 🙏 Its currently blocking me from adding Symfony 8 support on sonata-project/SonataBlockBundle#1245

VincentLanglet · 2025-11-29T10:25:06Z

Is there any blocker @garak @stof ?

garak · 2025-11-29T12:16:24Z

Is there any blocker @garak @stof ?

Unfortunately, this is currently not working, and raising an exception

Cannot dump definition because of invalid class name ('knp_menu.factory').

VincentLanglet · 2025-11-29T12:43:50Z

config/menu.php

+    ;
+
+    $services
+        ->set(MenuProviderInterface::class, service('knp_menu.menu_provider'))


Set signature is

set(?string $id, ?string $class = null)

I think you should write

Suggested change

->set(MenuProviderInterface::class, service('knp_menu.menu_provider'))

->alias(MenuProviderInterface::class, 'knp_menu.menu_provider')

VincentLanglet · 2025-11-29T12:44:15Z

config/menu.php

+    $services->set(FactoryInterface::class, service('knp_menu.factory'));
+    $services->set(MatcherInterface::class, service('knp_menu.matcher'));
+    $services->set(MenuManipulator::class, service('knp_menu.manipulator'));


Suggested change

$services->set(FactoryInterface::class, service('knp_menu.factory'));

$services->set(MatcherInterface::class, service('knp_menu.matcher'));

$services->set(MenuManipulator::class, service('knp_menu.manipulator'));

$services->alias(FactoryInterface::class, 'knp_menu.factory');

$services->alias(MatcherInterface::class, 'knp_menu.matcher');

$services->alias(MenuManipulator::class, 'knp_menu.manipulator');

Thanks for the suggestions. The main service now seems to be properly defined, but as soon as I try to use any menu I get a missing reference for the twig service. The service is defined in my project, but (quoting) the container inside "Symfony\Component\DependencyInjection\Argument\ServiceLocator" is a smaller service locator that only knows about the "knp_menu.renderer.list" and "knp_menu.renderer.twig" services."

What do you call "the twig service" ?

What is your config ?

And what is the exact error message ?

I'm not calling it myself, it's supposed to be passed by the bundle to the main library. I don't know if it could be related to another problem in the migration of the config

Not sure to understand the problem.

On sonata side, we use twig in the same way you defined it
https://github.com/sonata-project/SonataAdminBundle/blob/4.x/src/Resources/config/block.php

One differences I see is the fact we have twig in our dependencies and that here twig is not required in the composer.json

KnpMenuBundle/composer.json

Lines 21 to 28 in eabe078

"require": {

"php": "^8.1",

"knplabs/knp-menu": "^3.8",

"symfony/config": "^5.4 | ^6.0 | ^7.0",

"symfony/dependency-injection": "^5.4 | ^6.0 | ^7.0",

"symfony/deprecation-contracts": "^2.5 | ^3.3",

"symfony/http-kernel": "^5.4 | ^6.0 | ^7.0"

},

so I dunno if it requires ignoreOnInvalid/nullOnInvalid/ignoreOnUninitialized or something else

I'm testing this branch on an actual project, where Twig is present.

It's the service locator that's broken. See my comment on the menu.php file.

derrabus · 2025-11-29T17:31:54Z

config/menu.php

+    $services
+        ->set('knp_menu.renderer_provider', PsrProvider::class)
+        ->args([
+            new ServiceLocatorArgument(new TaggedIteratorArgument('knp_menu.renderer', null, 'alias')),


Suggested change

new ServiceLocatorArgument(new TaggedIteratorArgument('knp_menu.renderer', null, 'alias')),

new ServiceLocatorArgument(new TaggedIteratorArgument('knp_menu.renderer', 'alias')),

I think this should fix the locator.

garak added the hacktoberfest-accepted https://hacktoberfest.digitalocean.com label Oct 11, 2025

garak requested review from Copilot and stof October 11, 2025 14:27

Copilot AI reviewed Oct 11, 2025

View reviewed changes

stof reviewed Oct 28, 2025

View reviewed changes

garak changed the title ~~allow Symfony 8~~ allow Symfony 8, drop Symfony 5 Oct 28, 2025

garak force-pushed the php-config branch 2 times, most recently from 70fa392 to f4b2731 Compare October 28, 2025 15:35

allow Symfony 8

32fd66a

garak force-pushed the php-config branch from f4b2731 to 32fd66a Compare October 28, 2025 15:37

dmaicher mentioned this pull request Nov 23, 2025

Symfony 8 sonata-project/SonataBlockBundle#1245

Merged

dmaicher approved these changes Nov 23, 2025

View reviewed changes

derrabus approved these changes Nov 23, 2025

View reviewed changes

This was referenced Nov 26, 2025

Small Symfony 7.3 deprecations sonata-project/SonataPageBundle#1838

Merged

Symfony 8 support sonata-project/SonataAdminBundle#8360

Closed

VincentLanglet mentioned this pull request Nov 29, 2025

Adding symfony 8 support. sonata-project/SonataAdminBundle#8362

Closed

VincentLanglet reviewed Nov 29, 2025

View reviewed changes

derrabus reviewed Nov 29, 2025

View reviewed changes

fix aliases

948c136

garak force-pushed the php-config branch from f02b715 to 948c136 Compare November 30, 2025 08:28

garak merged commit aa22e57 into KnpLabs:master Nov 30, 2025
10 checks passed

garak deleted the php-config branch November 30, 2025 08:30

	->set(MenuProviderInterface::class, service('knp_menu.menu_provider'))
	->alias(MenuProviderInterface::class, 'knp_menu.menu_provider')

	"require": {
	"php": "^8.1",
	"knplabs/knp-menu": "^3.8",
	"symfony/config": "^5.4 \| ^6.0 \| ^7.0",
	"symfony/dependency-injection": "^5.4 \| ^6.0 \| ^7.0",
	"symfony/deprecation-contracts": "^2.5 \| ^3.3",
	"symfony/http-kernel": "^5.4 \| ^6.0 \| ^7.0"
	},

	new ServiceLocatorArgument(new TaggedIteratorArgument('knp_menu.renderer', null, 'alias')),
	new ServiceLocatorArgument(new TaggedIteratorArgument('knp_menu.renderer', 'alias')),

allow Symfony 8, drop Symfony 5 #500

allow Symfony 8, drop Symfony 5 #500

Uh oh!

Conversation

garak commented Oct 11, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

tacman commented Oct 27, 2025

Uh oh!

garak commented Oct 28, 2025

Uh oh!

tacman commented Oct 28, 2025

Uh oh!

garak commented Oct 28, 2025

Uh oh!

tacman commented Oct 28, 2025

Uh oh!

garak commented Oct 28, 2025

Uh oh!

stof commented Oct 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stof commented Oct 28, 2025

Uh oh!

garak commented Oct 28, 2025

Uh oh!

stof commented Oct 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garak commented Oct 28, 2025

Uh oh!

stof commented Oct 28, 2025

Uh oh!

Chris53897 commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tacman commented Nov 6, 2025

Uh oh!

Chris53897 commented Nov 6, 2025

Uh oh!

stof commented Nov 6, 2025

Uh oh!

tacman commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stof commented Nov 6, 2025

Uh oh!

dmaicher commented Nov 23, 2025

Uh oh!

VincentLanglet commented Nov 29, 2025

Uh oh!

garak commented Nov 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derrabus Nov 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Chris53897 commented Nov 6, 2025 •

edited

Loading

tacman commented Nov 6, 2025 •

edited

Loading

derrabus Nov 29, 2025 •

edited

Loading