Problem/Motivation
In #3252386: Use PHP attributes instead of doctrine annotations we added support for attribute based plugin discovery.
As part of that issue we converted block and action plugins.
This issue is to convert \Drupal\help\Annotation\HelpSectionplugins to use Attributes.
Proposed resolution
- Add a class to represent the new Attribute - Example
- Update the plugin manager constructor to include both the attribute and annotation class names - example
- Convert all plugins that use the annotation to use the new attribute - example
Remaining tasks
Add a new Attribute classUpdate the plugin manager constructorConvert front-facing pluginsConvert plugins used for testsAdd optional deriver argument to HelpSection Attribute class (per #3421793)
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | screenshot-3420993-HelpSection-UI-verification.png | 689.66 KB | amber himes matz |
Issue fork drupal-3420993
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:
- 3420993-convert-helpsection-plugin
changes, plain diff MR !6590
Comments
Comment #3
amber himes matzComment #5
amber himes matzI took a stab at this.
Comment #6
amber himes matzLooks like I missed some coding standards. I'll take a closer look tomorrow if I can. Happy to have help though.
Comment #7
amber himes matzComment #8
amber himes matzThanks for the review, @larowlan! I think I addressed everything, though there was 1 point of confusion about line 29 of core/modules/help/src/Plugin/HelpSection/HelpTopicSection.php, which I asked about in GitLab.
Comment #9
smustgrave commentedAppears to have some failures.
Comment #11
amber himes matzI'm working on this again. When I try to install Drupal 11.x-dev with this code running, the installation is failing with a typehint error in this new code.
TypeError: Drupal\Core\Plugin\Discovery\AnnotatedClassDiscovery::__construct(): Argument #4 ($annotation_namespaces) must be of type array, string given, called in /var/www/html/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php on line 305 in Drupal\Core\Plugin\Discovery\AnnotatedClassDiscovery->__construct() (line 56 of core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscovery.php).After fixing this, I will test the HelpSection in the UI to make sure it's working and upload a screenshot of that.
Comment #12
amber himes matzThere was a missing use statement for the HelpSection attribute class in HelpSectionManager.php. Adding this fixed the error on installation (because it fixed the value of the
HelpSection::classparameter passed in the parent constructor, which was meaningless without theuse Drupal\help\Attribute\HelpSection;statement.I tested this on a successfully-installed 11.x-dev site and verified that HelpSection plugins displayed as expected on admin/help. Screenshot attached with HelpSection plugins provided by Help module highlighted.
Comment #13
quietone commentedI glanced at MR and see that the test HelpSection plugins still need to be converted.
Comment #14
amber himes matzThanks @quietone for the reminder! I am working on this now.
Also, I updated the IS with remaining tasks (and those already done).
Comment #15
amber himes matzThis is ready for review.
I've converted the 2 test plugins, EmptyHelpSection and TestHelpSection to use the HelpSection attribute.
And I removed a trailing comma from the last item in the attribute HelpTopicSection. I guess we're not doing trailing commas for last items in these arrays in attributes? Yet, anyway. I think that code standards for these are still pending (that's my current understanding). Regardless, this makes everything consistent and is consistent with the Action example as well.
Comment #16
amber himes matzUpdated remaining tasks in issue summary (all tasks done).
Comment #17
smustgrave commentedSee that all @HelpSection have been replaced and tests appear to be passing.
Applying locally I can still use help module just fine
+1 from me but will leave in review for additional eyes.
Comment #18
smustgrave commentedRe-reviewed and appears all feedback has been addressed.
Comment #20
andypost+1 RTBC, just added
declare(strict_types=1);to new class and rebasedComment #21
catchThis is missing deriver support per #3421793: Add an assert that ensures all attribute plugins support derivers
Comment #22
andypostFixed #21
Comment #23
smustgrave commented@andypost mind taking a look at the phpcs error? Can review after.
Comment #24
andypostFixed, sorry
Comment #25
smustgrave commentedStill blows my mind you can make that change and few minutes later tests are done.
Feedback has been addressed.
Thanks!
Comment #26
amber himes matzUpdated issue summary’s remaining tasks about adding the optional deriver argument, and crossed it off to show that we’ve completed it.
Comment #27
amber himes matzI went through all unresolved threads and double-checked that they were resolved, and resolved them.
Comment #28
alexpottCommitted and pushed b9dfd470e3 to 11.x and c57ce025c3 to 10.3.x. Thanks!