Problem/Motivation

Working to enable the sniff Drupal.Commenting.FunctionComment.Missing.

Steps to reproduce

Proposed resolution

Fix this sniff for core/lib tests

Remaining tasks

Create an MR
Enable the sniff in phpcs.xml.dist to include core/lib
Fix reported errors
Review

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#26 3517527-nr-bot_di_b96ew.txt91 bytesneeds-review-queue-bot
#24 3517527-nr-bot_xji4z8ee.txt91 bytesneeds-review-queue-bot
#20 3517527-nr-bot_db1buq85.txt91 bytesneeds-review-queue-bot
#13 3517527-nr-bot.txt91 bytesneeds-review-queue-bot
#11 3517527-nr-bot.txt91 bytesneeds-review-queue-bot
#6 3517527-nr-bot.txt25.25 KBneeds-review-queue-bot

Issue fork drupal-3517527

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

quietone created an issue. See original summary.

quietone opened merge request !11744

quietone’s picture

quietone commented
Status: Active » Needs review
smustgrave’s picture

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

Do we want to open a follow up for the one thread?

Rest seems fine

quietone’s picture

quietone commented
Title: Fix Drupal.Commenting.FunctionComment.Missing in core/libts » Fix Drupal.Commenting.FunctionComment.Missing in core/lib
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3517527-nr-bot.txt25.25 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

quietone’s picture

quietone commented
Status: Needs work » Needs review

Another rebase, with a conflict that needs review.

smustgrave’s picture

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

Additional change seems fine.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

It's not good that phpcs is forcing us to ignore this rule when the documentation is there, but when there is another phpcs rule in place that breaks this rule. To me this feels worse than not having the rule enabled at all to be honest.

quietone’s picture

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

Follow up made #3524937: Fix unneeded Drupal.Commenting.FunctionComment.Missing

The MR is what it was when catch commented. I have commented, made an issue in Coder and a follow up for core, so I think it is OK to restore the RTBC.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3517527-nr-bot.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

quietone’s picture

quietone commented
Status: Needs work » Reviewed & tested by the community

Another rebase

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3517527-nr-bot.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

quietone’s picture

quietone commented
Status: Needs work » Reviewed & tested by the community

Rebased. There were conflicts due to the update of Coder, which fixed a problem with phpcs:ignore lines such that those lines could be removed. Linting passing so restoring RTBC.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

I read all the methods to confirm that the added one-line summaries matched what was actually happening, and looked up the parent class or interface for everything with {@inheritdoc} to confirm the docblock described what was happening in the method including signature, return value, etc.

I posted one suggestion to fix a grammatical error in a docblock that's being updated to differentiate it from an added docblock. I also suggest doing the GitLab contrib search to check for usages of the mentioned method to decide whether we want a followup to deprecate it or not.

Marking NR to make the status clear, but given that the tasks are just reviewing/applying a suggestion and doing a contrib search, the changes do not need a separate reviewer and author to be re-RTBCed.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Needs work

Actually @quietone in the linked issue, @klausi claims that the issue will be solved if we upgrade to the latest version of coder. 11.x now is on the latest version of coder. Can we therefore remove these other additional ignores?

quietone’s picture

quietone commented
Status: Needs work » Needs review

Linting checks have passes do back to NR.

smustgrave’s picture

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

Believe feedback has been addressed, left the 1 thread open jsut in case

Saved credit for xjm and catch for the MR reviews.

xjm’s picture

xjm
she/her
Primary language English
commented

Saving credit for @smustgrave for issue management, since this has gotten complicated.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3517527-nr-bot_db1buq85.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

quietone’s picture

quietone commented
Status: Needs work » Needs review

Change required in ElementInfoManager.php.

smustgrave’s picture

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

cspell was random, re-running fixed it.

Additional change seems fine to me.

quietone’s picture

quietone commented

Rebase with no conflicts.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3517527-nr-bot_xji4z8ee.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

quietone’s picture

quietone commented
Status: Needs work » Reviewed & tested by the community

Another rebase. Conflict due to removed code in \Drupal\Core\Render\ElementInfoManager, the file doesn't have changes anymore. Therefor restoring RTBC.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3517527-nr-bot_di_b96ew.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

quietone’s picture

quietone commented
Status: Needs work » Reviewed & tested by the community

Conflict due to #3489026: Document InputCollectorInterface which added better documentation than what was in this MR. Restoring RTBC.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

I think we need a follow-up to try to deprecate UrlMatches::finalMatch(), also one comment that I think can probably just be inheritdoc

quietone’s picture

quietone commented
Status: Needs work » Needs review

@catch, thanks. I have fixed that.

I rebased and there were more instances to fix, so back to NR for review, particularly the last two commits.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

The last 2 commits look great, and it was previously rtbc, so restoring to that state.

  • catch committed 877332b4 on 11.x 
    Issue #3517527 by quietone, smustgrave, catch, xjm: Fix Drupal....

catch closed merge request !11744

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x, thanks!

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

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

Maintainers, please credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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