- Issue created by @berramou
- Merge request !40Issue #3500618: Use attributes instead of annotations β (Merged) created by berramou
- π«π·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. - 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 theConfigEntityType
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'sderiver
annotation-keys in the following places?modules/sitemap_book/src/Plugin/Sitemap/Book.php
,src/Plugin/Sitemap/Menu.php
,src/Plugin/Sitemap/Vocabulary.php
, andtests/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 checksSpeaking of PHP versions and attributes, I think it's best to explicitly add a PHP requirement (
>=8.1
) incomposer.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. - π¨π¦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 insitemap.info.yml
, but to my surprise, I couldn't find any code to do that, so I updated bothcomposer.json
andsitemap.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 β
committed aea78c0a on 8.x-2.x authored by
berramou β
- π¨π¦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.