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

  1. Add a class to represent the new Attribute - Example
  2. Update the plugin manager constructor to include both the attribute and annotation class names - example
  3. Convert all plugins that use the annotation to use the new attribute - example

Remaining tasks

  1. Add a new Attribute class
  2. Update the plugin manager constructor
  3. Convert front-facing plugins
  4. Convert plugins used for tests
  5. Add optional deriver argument to HelpSection Attribute class (per #3421793)

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3420993

Command icon 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:

Comments

larowlan created an issue. See original summary.

Amber Himes Matz made their first commit to this issue’s fork.

amber himes matz’s picture

Assigned: Unassigned » amber himes matz

amber himes matz’s picture

Assigned: amber himes matz » Unassigned
Status: Active » Needs review

I took a stab at this.

amber himes matz’s picture

Status: Needs review » Needs work

Looks like I missed some coding standards. I'll take a closer look tomorrow if I can. Happy to have help though.

amber himes matz’s picture

Assigned: Unassigned » amber himes matz
amber himes matz’s picture

Assigned: amber himes matz » Unassigned
Status: Needs work » Needs review

Thanks 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.

smustgrave’s picture

Status: Needs review » Needs work

Appears to have some failures.

sakthi_dev made their first commit to this issue’s fork.

amber himes matz’s picture

Assigned: Unassigned » amber himes matz

I'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.

amber himes matz’s picture

Assigned: amber himes matz » Unassigned
Status: Needs work » Needs review
StatusFileSize
new689.66 KB

There 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::class parameter passed in the parent constructor, which was meaningless without the use 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.

Screenshot verifies HelpSection plugins on Help page.

quietone’s picture

I glanced at MR and see that the test HelpSection plugins still need to be converted.

amber himes matz’s picture

Assigned: Unassigned » amber himes matz
Issue summary: View changes
Status: Needs review » Needs work

Thanks @quietone for the reminder! I am working on this now.

Also, I updated the IS with remaining tasks (and those already done).

amber himes matz’s picture

Assigned: amber himes matz » Unassigned
Status: Needs work » Needs review

This 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.

amber himes matz’s picture

Issue summary: View changes

Updated remaining tasks in issue summary (all tasks done).

smustgrave’s picture

See 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.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Re-reviewed and appears all feedback has been addressed.

andypost made their first commit to this issue’s fork.

andypost’s picture

+1 RTBC, just added declare(strict_types=1); to new class and rebased

catch’s picture

Status: Reviewed & tested by the community » Needs work
andypost’s picture

Status: Needs work » Needs review

Fixed #21

smustgrave’s picture

@andypost mind taking a look at the phpcs error? Can review after.

andypost’s picture

Fixed, sorry

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Still blows my mind you can make that change and few minutes later tests are done.

Feedback has been addressed.

Thanks!

amber himes matz’s picture

Issue summary: View changes

Updated issue summary’s remaining tasks about adding the optional deriver argument, and crossed it off to show that we’ve completed it.

amber himes matz’s picture

I went through all unresolved threads and double-checked that they were resolved, and resolved them.

alexpott’s picture

Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed b9dfd470e3 to 11.x and c57ce025c3 to 10.3.x. Thanks!

  • alexpott committed c57ce025 on 10.3.x
    Issue #3420993 by Amber Himes Matz, andypost, sakthi_dev, smustgrave,...

  • alexpott committed b9dfd470 on 11.x
    Issue #3420993 by Amber Himes Matz, andypost, sakthi_dev, smustgrave,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.