Allow Symfony 7, dropped support for 4.4 and bumped minimal version to 5.4

[evertharmeling]

No description provided.

[yann-eugone]

Hello, first of all, thank you for your contribution
Can you explain why you wish we drop support for older php/symfony versions ?
Is there something that is not working in your cases ?

[evertharmeling]

Hi Yann,

Thanks for your response! Main thing is that Symfony 4.4 isn't maintained anymore, security fixes end this month, also see the overview of all supported versions.

Next to that the adjustment in src/Messenger/DumpSitemapMessageHandler.php would be much more complicated to fix as the AsMessageHandler attribute is introduced in Symfony 5.4.
Supporting both the interface MessageHandlerInterface (not being available in Symfony >=7.0) and the attribute AsMessageHandler (not being available in Symfony <5.3) would require some (unwanted) hacks. Dropping support for 4.4 solves this problem.

So all in all I think this justifies dropping the support for 4.4. Users who still use that version could easily keep using version 3.3.1 of this bundle.

[yann-eugone]

I agree
Still, this will require a new major version, so marked this PR as bc break

[evertharmeling]

It would not cause a BC break, and wouldn't necessarily require a new major version, as users who are on Symfony 4.4 won't be able to install a newer version of the bundle as they do not meet the requirements (symfony 5.4 and up).

But you could still release a new version if you want. Cheers!

[yann-eugone]

I see your point
But I still consider dropping support for Symfony versions to be done in a major release
Removing interfaces from classes might also be considered as BC, because you don't know what tools developers might have created on this

Still very appreciated, and will definitely merge it whenever the CI will be green

[evertharmeling]

Sure!

I dropped the usage of symfony/framework-extra-bundle as it is deprecated since Symfony 6.2 and incompatible with Symfony >=7.0. AFAIK it was only used for the Route annotations, as PHP attributes are the new best practices but only supported from Symfony >=6.4, I refactored lower versions to make use of Yaml routes.

This should fix the CI build hopefully.

[evertharmeling]

Could you approve the workflow Yann, so we at least see if the CI is all fixed now?

[yann-eugone]

I was sure I approved it... Sorry

[evertharmeling]

No problem! I've fixed some tests and now need your approval again :). Hopefully the suite passes for all versions now.

[evertharmeling]

Hmm they changed the return type of KernelTestCase::getContainer() from ContainerInterface to Container (see). Used in SitemapTestCase.

Not really sure how we may bypass this easily...

EDIT: maybe the same solution as done with Kernel :), let me see

Alright looks like this could work, could you approve the workflow once again :)

[yann-eugone]

what are these tests/Integration/config/*/routes/controllers.yaml files for ?

[evertharmeling]

As I dropped the usage of sensio/framework-extra-bundle which is also 'end-of-life' since Syfmony 6.2 (see).

I had to refactor the use of routing annotations (which was part of that bundle), and as routing attributes are not supported on lower versions I refactored the routing annotations to yaml routing config. So these controllers.yaml files contain the yaml routing definitions.

[yann-eugone]

So it means that we are not able to test that on symfony 5.4 the bundle is still able to work with annotation
This might be an issue IMO

[evertharmeling]

We aren't explicitly testing Route-annotations anymore indeed, but the only change made is the way the routing is defined. Both of the syntaxes result in the same underlying Route-object.

For example

/**
 * @Route("/blog", name="blog_read", options={"sitemap"={"section"="blog"}})
*/

Is simply rewritten to the Yaml syntax

blog_read:
    path: /blog
    controller: Presta\SitemapBundle\Tests\Integration\Controller\BlogController::read
    options:
        sitemap:
            section: blog

Since the bundle (sensio/framework-extra-bundle) is abandoned, it's very unlikely that the currently used syntax wouldn't work anymore (for projects using Symfony <6.3). And even if it would turn out to be erroneous it would be related to the same code in symfony/routing as the Yaml or Attribute syntax uses and thus be still supported in our testsuite.

If we would have had an own Annotation-route class depending on the bundle above that would be something different, the defined 'options' we use are just part of the same code as before.

So we are not compromising on our testsuite, we just use an alternative syntax to define the same arrangement to build the testsuite.

[J-Ben87]

Hi evertharmeling,
First of all thank you for your contribution, this is really great!

I'm confused here: why would you remove the @Route annotations? I understand that the sensio/framework-extra-bundle is abandonned, but I'm pretty sure the annotations we are talking about are Symfony's ones, right?

Looking at the diff here and here, you will notice that you didn't have to remove any import and that the imported Route comes from Symfony's namespace.

If I'm correct, maybe we could keep the annotations as it's a valid way to declare a route in the latest Symfony versions.
What do you think?

[evertharmeling]

Hi Benoit,

Thanks for reaching out, first of all I never used annotations to configure my routes myself, so I had to dive into it all too, and this is what I got out of it.

You both are right. sorry for my stubbornness , I went off-track with the notification on the SensioFrameworkExtraBundle docs, and mistakenly assumed Annotations were completely dropped in higher versions, which would make still supporting it on all versions problematic.

It's no longer recommended to use this bundle in current Symfony applications. All the annotations provided by this bundle are now built-in in Symfony as PHP attributes. Check out the full list of Symfony attributes.

They don't mention Annotations are still supported, but here they do :)

I've reverted the changes and it looks like the tests still pass.

Things to note:

• if I'm correct we are now only testing the Annotations routes on Symfony 5.4 and PHP < 8.0, hence the check in Kernel.php

Let's see if the whole workflow still passes 🤞

[J-Ben87]

No worries, it's quite confusing anyway that Symfony started to use the same @Route annotation's name in replacement of Sensio's one.

Here is the trick:

-use Sensio\Bundle\FrameworkExtraBundle\Configuration\Route;
+use Symfony\Component\Routing\Annotation\Route;

The first import comes from Sensio's bundle and is deprecated, the second one comes from Symfony and is perfectly valid to be used as annotation @Route or attribute #[Route].

Thank you for having taken this into account 🙂

[evertharmeling]

Ah sorry I seem to have messed up the contents of the /special/annotations.yaml file, fixed it. One last time approving the workflow 🤞

[dkarlovi]

@yann-eugone

I still consider dropping support for Symfony versions to be done in a major release

This is typically not the case, dropping support for specific dependencies is not a breaking change because the users of the dropped dependency will not get the new package offered, no?

when we drop support for an older version of PHP, composer will not consider the new version if the PHP version requirement is no longer fulfilled. Thus, you won't end up with a fatal error due to a wrong method signature, you just won't get the new version.

You don't need to release a major version just to stop supporting an old dep version.

[yann-eugone]

@dkarlovi sure, I've understand this point of view
I believe everyone is free to deal with this kind of problem the way they want, I understand why Doctrine is doing it like it, and that's OK
But as you can see in the first paragraph of the link you provided

Since many people are encountering issues with these updates ...

And here is the issue : people complains for almost anything
As a maintainer of package, I want to be clear what the contract are when using the bundle
And to me, it's very clear that :

• dropping version of a package
• dropping compatibility with a package
• dropping compatibility with a feature of a package

Remains a BC

[yann-eugone]

@evertharmeling thank you for your help, your efforts, and everything else

[yann-eugone]

hmmm ok I've missed something
We already have a 4.x branch, and these devs here should have been done on that branch
do you want to rebase your branch @evertharmeling ?

[evertharmeling]

@yann-eugone done!

No problem, glad I could be of help! Thanks for your help and patience!

[Valmonzo]

Hello, what is missing to merge this PR ? Any helps needed ?

[shmshd]

@yann-eugone, just wondering if there's any particular reason for the delay in merging this PR?

[yann-eugone]

I was unavailable for some holidays 🎄

Unfortunately, it seems that the rebase of that branch picked some commits we don't want.

These are commits added to the 3.x branch for the upgrade path to 4.x.

I understand this is partially my fault, because I didn't told you this from the beginning you targeted the wrong branch, still, these commits must be dropped in order for that PR to be merged.

[evertharmeling]

Sorry missed the commits popped in, I removed them and it should be fixed now! @yann-eugone could you approve the workflow once again?

[yann-eugone]

Thank you for your help, very much appreciated

[evertharmeling]

No problem, glad to be of help! Thanks for the feedback and assistance!

[CoalaJoe]

Thanks for the work guys! Is there a release already planned containing these changes?

[yann-eugone]

Not yet, the next major version will take some time to be tested

You can help by requiring the dev version on your projects

composer require presta/sitemap-bundle:4.x-dev@dev

And provide feedback by opening issues if something fails

[Valmonzo]

Thx for your works guys !