Problem/Motivation

Automated hook conversion relies on accurate implements comments.
Several themes have several functions incorrectly identified.

Steps to reproduce

Open the files.

Proposed resolution

Fix them

Remaining tasks

Review

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3554678

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:

About issue forks

Comments

nicxvan created an issue. See original summary.

nicxvan opened merge request !13599

nicxvan’s picture

nicxvan commented
Issue summary: View changes
Status: Active » Needs review
nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Title: Fix implements comments in themes for conversion » Fix implements comments in themes to prepare for Rector
quietone’s picture

quietone commented

I haven't looked closely at the MR so just a question, should this add the 'for ...' phrase per the standard. There is an issue to do that for all hooks but perhaps it is easy to add it here? That issue is #2886719: Follow code style for complex hooks

nicxvan’s picture

nicxvan commented

I took a pass since I thought it would be easy even if it seems slightly out of scope, I think just like many of the implements are wrong the pattern is all over the place, and some of the test preprocess are so long they go over the 80 character limit so I didn't change those ones.

I'd like to get this in so we can get the rector conversion working for themes if possible so if we need to take another pass, I'd like to just keep scope at getting the Implements right.

Also #2886719: Follow code style for complex hooks is postponed and I suspect the issue to remove Implements entirely will get in first so this likely doesn't need more work, we just need the implements right for rector.

shubham_pareek_19’s picture

shubham_pareek_19 commented
Status: Needs review » Reviewed & tested by the community

I’ve gone through the merge request and checked all the updated comments. Everything looks correct now the patterns follow the format.I didn’t see any functional changes.
Overall, looks good to me and ready to move forward.

  • quietone committed cea3e856 on 11.3.x 
    Issue #3554678 by nicxvan, shubham_pareek_19: Fix implements comments in...

  • quietone committed e48c7c34 on 11.x 
    Issue #3554678 by nicxvan, shubham_pareek_19: Fix implements comments in...
quietone’s picture

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

@shubham_pareek_19, thanks for explaining what you did to review this MR.

Committed to 11.x and cherry-picked to 11.3.x

Thanks.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

quietone closed merge request !13599

Status: Fixed » Closed (fixed)

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