- 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.
Automatically closed - issue fixed for 2 weeks with no activity.