Problem/Motivation

The minimum version of Drupal core supported by the Sitemap module at time-of-writing is 10.2.0.

Beginning in Drupal 10.2.0, the Drupal community is moving towards using PHP Attributes instead of Docblock Annotations for plugins, and deprecating plugins using annotations and plugin types not supporting attributes. We should follow suit

Using PHP Attributes would allow Sitemap contributors to take advantage of...

  1. IDE autocompletion
  2. Refactoring safety
  3. Compile-time class existence checks

... among other things. While some (paid) IDEs support Docblock Annotations in similar ways, the support for them is not as robust.

Proposed resolution

Use attributes instead of annotations for Sitemap plugin.

Remaining tasks

  1. Write a patch
  2. Review and feedback
  3. RTBC and feedback
  4. Commit
  5. Release - released in version 8.x-2.1

User interface changes

None.

API changes

Adds a \Drupal\sitemap\Attribute\Sitemap attribute.

Data model changes

None.

Issue fork sitemap-3500618

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

berramou created an issue. See original summary.

berramou opened merge request !40

berramou’s picture

berramou commented

TODO:

Delete the annotation class src/Annotation/Sitemap.php once annotations are no longer supported.
Add a note for developers to update their plugins after the change to use the attribute instead of the annotation.

mparker17 made their first commit to this issue’s fork.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Status: Active » Needs review

@berramou, thank you for the excellent contributions! The code is clear, it makes good use of APIs, and doesn't miss anything as far as I can tell. Manual testing shows everything working as far as I can tell. I don't think this needs any new tests, because the existing tests cover the changes (and they all pass!).

I've rebased this onto 8.x-2.x.

Quick question before I merge this: I noticed that in some annotations in core (for example, in the handlers sub-array of the ConfigEntityType annotation on \Drupal\block\Entity\Block), references to other classes in annotations are done with the special ::class constant... should we also do that for Sitemap's deriver annotation-keys in the following places?

  1. modules/sitemap_book/src/Plugin/Sitemap/Book.php,
  2. src/Plugin/Sitemap/Menu.php,
  3. src/Plugin/Sitemap/Vocabulary.php, and
  4. tests/modules/sitemap_custom_plugin_test/src/Plugin/Sitemap/DerivativeSitemapPlugin.php

... for example, in src/Plugin/Sitemap/Menu.php, should we modify the code as follows...?

 use Drupal\sitemap\Attribute\Sitemap;
+use Drupal\sitemap\Plugin\Derivative\MenuSitemapDeriver;
 use Drupal\sitemap\SitemapBase;
 /*...*/
 #[Sitemap(
   /*...*/
-  deriver: "Drupal\sitemap\Plugin\Derivative\MenuSitemapDeriver",
+  deriver: MenuSitemapDeriver::class,
   menu: "",
 )]
berramou’s picture

berramou commented

Hello @mparker17, thanks for the review and the rebase.

You're absolutely right — since we're using attributes and targeting PHP 8+, it's preferable to use ::class for:

- IDE autocompletion
- Refactoring safety
- Compile-time class existence checks

Speaking of PHP versions and attributes, I think it's best to explicitly add a PHP requirement (>=8.1) in composer.json.
As the module is compatible with ^10.2 || ^11 of Drupal, and Drupal 10.2+ requires PHP 8.1+, this makes sense and keeps things clear.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented

Speaking of PHP versions and attributes, I think it's best to explicitly add a PHP requirement (>=8.1) in composer.json.

Makes sense: I've done that in the latest commit.

I expected the Drupal.org Composer Service (façade) a.k.a. project_composer to update composer.json with a PHP version to match what's in sitemap.info.yml, but to my surprise, I couldn't find any code to do that, so I updated both composer.json and sitemap.info.yml.

If tests pass, then I'm going to merge this. Thanks!

  • mparker17 committed aea78c0a on 8.x-2.x authored by berramou 
    Issue #3500618 by berramou: Use attributes instead of annotations
mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes
Status: Needs review » Fixed

Marked as Fixed.

Updated the issue summary with more information, for users of the module who are interested in the change when they find it in the release notes.

berramou’s picture

berramou commented

Thank @mparker17 for your contributions.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented

I discovered that using the new keyword in the PHP annotations (i.e.: title: new TranslatableMarkup(...)) actually introduced an unstated dependency on PHP 8.1!

I've created a follow-up issue, #3541553: Unstated dependency on PHP 8.1 — see that one for more details.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes

Quick update: the changes in this issue have been released in version 8.x-2.1.

berramou’s picture

berramou commented

thank you @mparker17 for the release!