Use attributes instead of annotations

Created on 17 January 2025, 5 months ago

Problem/Motivation

Use attributes instead of annotations for Sitemap plugin.

πŸ“Œ Task
Status

Active

Version

2.0

Component

Code

Created by

πŸ‡«πŸ‡·France berramou

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @berramou
  • Pipeline finished with Success
    5 months ago
    Total: 591s
    #399191
  • πŸ‡«πŸ‡·France berramou

    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.

  • Pipeline finished with Success
    5 months ago
    Total: 447s
    #399197
  • Pipeline finished with Success
    4 months ago
    Total: 338s
    #422260
  • Pipeline finished with Success
    4 months ago
    Total: 414s
    #422283
  • Pipeline finished with Success
    4 months ago
    Total: 302s
    #423150
  • First commit to issue fork.
  • πŸ‡¨πŸ‡¦Canada mparker17 UTC-4

    @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: "",
     )]
    
  • πŸ‡«πŸ‡·France berramou

    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.

  • Pipeline finished with Success
    13 days ago
    Total: 472s
    #505954
  • πŸ‡¨πŸ‡¦Canada mparker17 UTC-4

    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!

  • πŸ‡¨πŸ‡¦Canada mparker17 UTC-4

    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.

  • πŸ‡«πŸ‡·France berramou

    Thank @mparker17 for your contributions.

Production build 0.71.5 2024