Automated Drupal 11 compatibility fixes for xmlsitemap

Created MR 46 with required change, since all tests are passing, hence requesting review.

To ensure compatibility with D11, we should incorporate D11-based testing. Please enable it for verification purposes.

Now all tests are passing, including the next major: https://git.drupalcode.org/issue/xmlsitemap-3435760/-/pipelines/227593, hence I'll remove the changes added for fetching modules from the CVS path and request review for the MR46.

Moving this to RTBC as there is no more actionable items pending and we have verified that this works with the latest upcoming changes of drupal/robotstxt in comment #10. Tests will start passing only when https://www.drupal.org/project/robotstxt/issues/3434261 is released.

Upstream issue #3434261: Automated Drupal 11 compatibility fixes for robotstxt is now fixed and RobotsTxt 8.1.6 has been released with D11 compatibility: https://www.drupal.org/project/robotstxt/releases/8.x-1.6

Now CI is green, can we get this merge and release?

OTOH, upgrade_status still reports three instances of error Call to deprecated method renderPlain() of class Drupal\Core\Render\Renderer. Deprecated in drupal:10.3.0 and is removed from drupal:12.0.0. Use Drupal\Core\Render\RendererInterface::renderInIsolation() instead.

But these are only a problem for D12 migration, so should not be holding back the D11 release. Pushing a fix to see if it breaks anything. For some reason, my IDE reformatted the code, but there are only three actual changes.

Pipeline still passes with tons of code style, spelling, etc warnings, but no errors, so leaving RTBC.

New additions by @fgm are necessary, but will result in a fatal error for Drupal < 10.3. Use the DeprecationHelper instead.

Tried the Deprecation Helper, seems to work just fine in 10.2.7 and 11.0.1

Tests are currently failing for previous major.

Tests are green now.

But I am really unsure about this MR... There are LOADS of unrelated (and seemingly unnecessary) code style changes, which makes reviewing quite cumbersome. Found one more issue, which I commented.

Besides, 18 new phpcs issues were introduced, but the maintainer should decide here in the end.

AIUI, these code style changes were originally added just to make the tests pass, not for any technical purpose. Seeing how tests are now passing, I think it would be good to release this to unblock at least D11 users.

Users of D10.3 and of obsolete versions do not have any more requirement to upgrade XMLSitemap than the core versions which they did not upgrade, so that should not be a problem for them.

Seems weird to me, that changes like https://git.drupalcode.org/project/xmlsitemap/-/merge_requests/46/diffs#... would fix any tests?

But not really relevant, in the end, the maintainer needs to decide here 🙂

Only https://git.drupalcode.org/project/xmlsitemap/-/merge_requests/46#note_3... left. Afterwards, I can ping the maintainer.

This MR could now apply for D11 with:

...
    "repositories": {
        "drupal": {
            "canonical": false,
            "type": "composer",
            "url": "https://packages.drupal.org/8"
        },
        "drupal/xmlsitemap": {
            "canonical": false,
            "type": "vcs",
            "url": "https://git.drupalcode.org/issue/xmlsitemap-3435760.git"
        },
    },
...
    "require": {
        "drupal/xmlsitemap": "dev-3435760-automated-drupal-11",
    }
...

@grevil I added a workaround for that default parameter value, but I'm not a big fan of it. Any better suggestion ?

The "previous major" fail is because Drupal 9 (RIP), didn't have the DeprecationHelper so we cannot have a version for D9 to D11 anyway.

The "previous major" fail is because Drupal 9 (RIP), didn't have the DeprecationHelper so we cannot have a version for D9 to D11 anyway.

Sorry, I didn't know this! Thought they backported it to D9, but hence it has reached its EOL, they probably just ditched the backport.

@maintainers: I'd vote to remove Drupal 9 compatibilty (EOL!) and require ^10.3 so we can remove the if-statements! Please decide how to proceed.

What about the 2.x branch? I think this should be against 2.x and not 8.x-1.x!

Alright, as the old "3435760-automated-drupal-11" branch contained a LOT of unrelated (and unnecessary) code style changes, I created a new branch "3435760-drupal-11-compatibility-cleaned" containing only the D11 compatibility changes.

I also used version_compare instead of the DeprecationHelper, to keep the D9 compatibility. Please review!

Thanks @Grevil! Maybe it then makes sense

1. to merge this into 8.x-1.x (as last change?)
2. rebase 2.x on latest 8.x-1.x (if it has no changes yet, but 8.x-1.x has)
3. Remove the ^9 compatibility and require ^10.3 in 2.x - so we can remove the if's in 2.x branch
4. Release 2.0.0 for the future?

But let's wait for maintainer feedback. Setting priority to major as D11 is out!

100% for removal of 9.x compatibility. Keeping contrib compatible with dead versions just helps prolong the suffering of those still running them and makes code worse for those maintaining modules.

Agreed! Let's see what @dave reid has to say about this.

Left some comments, it seems like almost all of the changes in the xmlsitemap.module file could be reverted.

I think as next step we urgently need a maintainer decision here if we should drop Drupal 9 support and require Drupal ^10.3 at least
(^10.3 || ^11) which would allow a lot of cleanup. In that case this would also mean we need a 2.x branch for BC, I think?

Could anyone ping maintainers? Seems there are many, but many inactive...

Just realised I reviewed the wrong MR yesterday 🤦‍♂️

I've reached out on slack to see if any maintainers can comment.

Latest commit fixes composer issues with next major and they're all green!

Just realised I reviewed the wrong MR yesterday 🤦‍♂️

Happens to the best! 😄

I've reached out on slack to see if any maintainers can comment.

Already did that a few weeks ago... no answer yet, but maybe you have better luck! 🙂👍

@acbramley any luck?
Otherwise request maintainership perhaps?

No response unfortunately, I haven't pinged anyone directly though. There are 13 maintainers on this project, has anyone tried contact forms or direct messages?

I've pinged several maintainers (all the ones I could find in slack) here https://drupal.slack.com/archives/C1BMUQ9U6/p1727322868231979?thread_ts=...

Oh my apologies, the last couple of weeks I contacted quite a few maintainers in different modules. Seems I mixed it up, I wanted to contact @dave reid regarding this issue (as he is the most recently active maintainer), but it seems I haven't!

I'll contact him now (on slack and via mail).

EDIT: Done

I reached out in slack to @dave reid as well.

I'd vote for moving forward with dropping Drupal 9, it's no longer supported, and there is an update path for people still on Drupal 9.

I can't imagine there being a strong objection.

I'd say somebody creates a maintainer ship issue?

@nicxvan would you be interested in (minimally) maintaining this module?

I can minimally maintain

Very nice @ivnish!

Please create an issue regarding the maintainership, so we can hopefully get this fixed ASAP!

@fgm @acbramley what about maintainership for you?

Maintainers should be cleaned up then! Unbelievable and risky they're all inactive!

I actually just realized that I use https://www.drupal.org/project/simple_sitemap on most of my sites. The site using this was inherited and I didn't realize it was different.

Simple sitemap is already d11 ready and I think is in drupal cms so I'm planning on switching modules.

@anybody : I'm not in a position to take up maintainership of one more module.

With above discussion threads on MR!48 for bifurcating the cleaner implementation without managing the version_compare checks for handling the deprecations, I've cut the separate branch i.e 3435760-d11-readiness against 2.x considering the next major release with supporting drupal version ^10.3 || ^11

Please review MR!49 ~~ pipelines are passing for phpunit and we can fix the validate pipeline separately.

cc: @anybody @grevil @acbramley

I would like to suggest to consider both MR as MR!48 with backward compatibility and MR!49 with minimum support for d10.3 for next major release i.e 2.x

Reviewed MR !49, looks good to me now. Hence, RTBC.

Re: maintainership. I am one of the maintainers of this module, but was maintaining only the 7.x branch. I do not have permission to add another maintainers. So I think we need at least one issue - either from me to get full permissions, or from someone else to get on board. Thanks!

//edit - That said, I would be able to take a look at the D8+ version of the module if needed, but I will be too busy until Christmas, so only then..

@poker10 Do you have permission to cut a release for the 2.x branch? Seems the community has tested this and feels comfortable with this.

https://www.drupal.org/project/xmlsitemap/maintainers.json

Looks like poker10 has all permissions except "administer maintainers"

I think he can commit this and create a new release. It won't take long

Yes, I can commit this and create a release. I looked at the MR and all changes looks good to me (tests are passing with these changes). I can't tell without a deeper check, if the MR is not missing something else, but if tests are green, probably not.

One thing I am not sure about is the status of the 2.x branch. There seems to be some additional commits in comparision with the 8.x-1.x branch. Yes, I can roll a 2.x-beta, but have anyone tested the 2.x-dev if it is really working with all changes currently committed? Sorry, I do not have time to check it commit by commit now.

Thanks!

Good call, @poker10. Here's my audit & analysis:

1. The cumulative code changes between the 2.x and 8.x-1.x branch can be seen at https://git.drupalcode.org/project/xmlsitemap/-/compare/2.x..8.x-1.x

• The change committed to 8.x-1.x not present in 2.x is https://git.drupalcode.org/project/xmlsitemap/-/commit/40727c92415802bb1... , which is innocuous enough and could simply be merged into 2.x without issue
• The changes committed to 2.x not present in 8.x-1.x consist of https://git.drupalcode.org/project/xmlsitemap/-/commit/1d59a51562d50b7d3... (see #3257944: PHP 8.1: XmlSitemapWriter::openUri($uri) should either be compatible with XMLWriter::openUri(string $uri): bool). This commit removes functions that were deprecated in 8.x-1.x and provides additional PHP 8.1 compatibility with return type hinting.

2. Of note, this MR removes the explicit PHP version constraints from the composer.json file and xmlsitemap.info.yml file that were added in https://git.drupalcode.org/project/xmlsitemap/-/commit/1d59a51562d50b7d3... . I believe this makes sense, since there is no longer a need to specify minimum PHP 8.1 compatibility in the module itself, since its constraint to Drupal 10/11 does the same thing. Conclusion: this change in the MR is fine.

3. I functionally tested this MR locally and can confirm that it works the same in our codebase as 8.x-1.5. While my testing certainly didn't comprehensively evaluate all of the options for XML sitemap, the code review above suggests that there should not be any functional or backwards-compatibility breaking change, and since the Drupal core compatibility is set at ^10.3 || ^11, which have a minimum PHP version requirement of 8.1, there is no risk of incompatibility with sites running on PHP 7.

4. Based on the above, the actions to take, I believe, are:

• (Optional, but probably a good idea) Merge 8.x-1.x into 2.x once more, which will simply add the logo.png file to 2.x and get the branch history up to date.
• Merge the Drupal 11 compatibility changes in this issue into 2.x and release 2.0.0-beta1. The release notes should call out the following:
  • The 2.x major version release of XML Sitemap removes functions deprecated in 8.x-1.x four years ago. The functions were likely unused, but developers who have extended XML Sitemap should review #3156088: user_access() does not exist in Drupal 8 anymore for confirmation.
  • 2.x also drops support of Drupal 9 up to Drupal 10.2 (i.e., compatible with Drupal 10.3 and 11). Sites still using lower versions of Drupal core should continue to use 8.x-1.5.

@mark_fullmer good job!

I think we can just add D11 support to 8.x-1.x and allow people to upgrade their sites to D11

@poker10 I created https://www.drupal.org/project/projectownership/issues/3488525

@ivnish I would probably not do that in 8.x-1.x, as it is going to drop support for anything below 10.3, which can be disruptive for existing sites?

Thanks for creating the other issue.

as it is going to drop support for anything below 10.3, which can be disruptive for existing sites

Yes, of course. Just an idea :)

Let's working on 2.x branch

@poker10, you need to create your own issue to become a maintainer of this module. My request was closed :(

The commits noted in #64 match the analysis & recommendation from #59. Code review looks good. This is RTBC and ready for a 2.0.0-beta1 release, which calls out the following:

• The 2.x major version release of XML Sitemap removes functions deprecated in 8.x-1.x four years ago. The functions were likely unused, but developers who have extended XML Sitemap should review #3156088: user_access() does not exist in Drupal 8 anymore for confirmation.
• 2.x also drops support of Drupal 9 up to Drupal 10.2 (i.e., compatible with Drupal 10.3 and 11). Sites still using lower versions of Drupal core should continue to use 8.x-1.5.

Saving credits.

Yeah! Thank you, @poker10

Merged this to 2.x, thanks everyone! Closing this as fixed - if there will be any other issues, please open a new issue. 8.x-1.x will remain without the D11 support, as that is much easier. And I will create a new 2.0.0-beta1 release today.

Here is the new release: https://www.drupal.org/project/xmlsitemap/releases/2.0.0-beta1

This is a pretty long thread. But would it be fair to summarize it as: for D11 use version 2?

And in that case can we add a summary to the OP?

Finally, a link to the issues stopping a official / non-beta release would be helpful, so people can help with those.
Because not everyone is capable or able to use beta-releases ( not skilled enough with composer, government compliance, etc.)

Thanks for the thoughts @firfin. Week or two ago, I updated the XML Sitemap project description (see https://www.drupal.org/project/xmlsitemap), so the branches should be described also here. It is true, that the 2.0.x branch is with the D11 support and I do not think we will add it to the previous 8.x-1.x branch. If you feel that this IS should be updated as well, feel free to update it.

I am not aware of any new issues with the 2.0.x branch so far. One of the reasons it was tagged as beta was the fact, that I am not as familiar with the D8+ branches of the module (was previously maintaining only the D7 branch). But if there won't be any new issues, I think that we can tag a stable release soon.