Skip to content

Conversation

@skmedix
Copy link
Contributor

@skmedix skmedix commented Dec 2, 2025

This PR is big one because it encompasses all the necessary changes that have been building up over time.

In Symfony 7.4, XML configurations are deprecated and removed in Symfony 8.0, so I moved the configurations to PHP, as this will not result in a BC. In Symfony 8.0, there are no more annotations, but since this 4.x bundle supports Symfony versions above 6.4, I changed the controllers to use attributes. I also changed routing.yml to use php. However, to avoid introducing a breaking change, I left both routing.yml and the new routing.yml, as both should work. Please review it thoroughly in case I have overlooked something.

Copy link
Member

@yann-eugone yann-eugone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi there
First of all, thank you for reaching out
It's always a pleasure to have contributors 👍

I've set a review, and will try my best to help you finish it

symfony-version: 7.4.*
- php-version: 8.4
symfony-version: 7.3.*
symfony-version: 8.0.*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's not written anywhere but our testing matrix policy is

every Symfony LTS version, + latest compatible version
each with lowest and hight compatible PHP version

I'd like to keep it that way

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand this correctly, this should be better:

Symfony PHP Lowest PHP Highest
6.4 LTS 8.1 8.5
7.4 LTS 8.2 8.5
8.0 8.4 8.5

Copy link
Contributor Author

@skmedix skmedix Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: I had to lower the highest of 6.4 to PHP 8.3, as it was failing the CI. However, I don't have time to figure it out today :)

@skmedix skmedix force-pushed the dev/symfony-8 branch 7 times, most recently from 10fa0f0 to b3d89ae Compare December 3, 2025 16:39
Copy link
Member

@yann-eugone yann-eugone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for you help
I will try to see why you had to lower the PHP version on my own

@benjaminroche4
Copy link

Please merge it !

@yann-eugone
Copy link
Member

Nope sorry, still need to understand why @skmedix had to remove some versions from the testing matrix

@skmedix
Copy link
Contributor Author

skmedix commented Dec 15, 2025

@yann-eugone I figured out that it wasn't installing the correct version of Symfony. This has been fixed, and I have also updated the tests to use PHP 8.5. Everything should be green now :)

@yann-eugone
Copy link
Member

Because tests are OK, I will just merge it like this
But I think there things I need to try on own before going to a stable release

So for the moment, if you want to have this bundle on Symfony 8.0 you will need to require the 4.x dev version

@yann-eugone yann-eugone merged commit 46e33fc into prestaconcept:4.x Dec 16, 2025
9 checks passed
@yann-eugone
Copy link
Member

yann-eugone commented Dec 16, 2025

@skmedix the issue you encountered with the Symfony version was fixed using more composer require, see 91d2ea1

the issue with routing/annotation was fixed using double alias in each controller, see 6364bf5

@tacman
Copy link
Contributor

tacman commented Dec 16, 2025

I've updated my branch of the demo to Symfony 8

https://github.com/tacman/presta-sitemap-test-project

The dynamic sitemap is no longer working, so I added instructions to dump it to the README. I switched the database to sqlite so that this would work:

git clone git@github.com:tacman/presta-sitemap-test-project.git presta && cd presta
composer install
bin/console doctrine:schema:update --force --complete
bin/console doctrine:fixtures:load --no-interaction
symfony server:start -d
symfony open:local

no dynamic sitemaps, but

bin/console presta:sitemaps:dump
symfony open:local

works.

@yann-eugone
Copy link
Member

I don't understand the issue you are facing.
What is failing exactly?

I just tried the 4.x-dev@dev along with standard Symfony 8.0 on a very simple project with entities in the sitemap, and it just works.

@tacman
Copy link
Contributor

tacman commented Dec 17, 2025

image

No sitemaps in the index until

bin/console presta:sitemaps:dump

image

@yann-eugone
Copy link
Member

I don't get your problem
That's exactly what presta:sitemaps:dump was created for : it collect all the URLs of your sitemaps, and create the according files in the public directory

So before you call that command, there is no reason for these files to exists

@tacman
Copy link
Contributor

tacman commented Dec 17, 2025

I was under the impression that these were available dynamically, but dumping them make them static and was recommended for production. This follows the Symfony AssetMapper model.

But I must have misunderstood dynamic v static.

@tacman
Copy link
Contributor

tacman commented Dec 17, 2025

Access sitemap on the fly with a symfony controller or Dump sitemap to files for faster sitemap

I think the issue might be the dynamic list of sitemap Urlsets, v. the sitemaps themselves. The behavior shows above is that the sets aren't listed until they're dumped.

@yann-eugone
Copy link
Member

How is that list built?
If it relies on the filesystem, then you have no other option than dumping the sitemap to have it listed.

I believe the "on the fly" sitemaps are the one accessed using the routing and the controller provided by the bundle.
The content will be built whenever you will access the /sitemap.xml, but don't expect any kind of listing.

If you have other questions, please open an issue.

@skmedix skmedix deleted the dev/symfony-8 branch December 17, 2025 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants