Closed (fixed)
Project:
Drupal core
Version:
10.3.x-dev
Component:
help.module
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
13 Feb 2024 at 03:15 UTC
Updated:
16 Mar 2024 at 11:29 UTC
Jump to comment: Most recent, Most recent file
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!