Problem/Motivation

In #2665992: @file is not required for classes, interfaces and traits, we are implementing a new coding standard to remove redundant @file docblocks from classes. (See #2304909: Relax requirement for @file when using OO Class or Interface per file and #2665744: @file is not required for classes, interfaces and traits.) However, a handful of classes in core have actual documentation of some sort added to the @file docblock.

Proposed resolution

These docs should be merged into the class docblock instead. Move the docs into the class docblock and consolidate them appropriately (e.g. removing redundant information and ordering the sections according to our standards).

Remaining tasks

CommentFileSizeAuthor
#12 2676816-12.patch9.06 KBxjm
move-docs-in-file-docblock.patch9.06 KBxjm

Comments

xjm created an issue. See original summary.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +Novice

A novice contributor could review this patch to check its documentation standards and to proofread the slight changes to the documentation.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

The patch looks good to me -- all of the changes seem to be in the right places and the small grammar/punctuation edits are good.

I haven't verified that it covers all of the classes' @file blocks that it should. Is there some script/sniff we can run to check that? So I haven't marked it RTBC, although if you can vouch for that, you can set it to RTBC with my blessing on the actual patch.

Also a slightly unrelated note...

+++ b/core/modules/field/src/Tests/Views/FieldTestBase.php
@@ -24,6 +12,13 @@
+ * @todo Test on a generic entity not on a node. What has to be tested:

Don't we also need an issue filed with a @todo, and the issue number added to this block?

jhodgdon’s picture

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

Oh wait, I see the list on #2665992-8: @file is not required for classes, interfaces and traits. So... comparing to what is in this patch...

Looks like

core/lib/Drupal/Core/ProxyClass/*
core/modules/book/src/ProxyClass/BookUninstallValidator.php
core/modules/forum/src/ProxyClass/ForumUninstallValidator.php
core/modules/language/src/ProxyClass/LanguageConverter.php
core/modules/views_ui/src/ProxyClass/ParamConverter/ViewUIConverter.php

are missing, but then you said you'd create a separate issue for proxy classes, so I think that is OK.

It's ironic that DrupalStandardsListener had a non-standard doc block. ;)

Anyway... should we take care of the issue for @todo on this patch? If not, set to RTBC and make a new issue for that, but it seems like maybe it should be done here if we're reformatting that block and changing @TODO to @todo anyway...

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes

Posted the followup: #2676836: Update proxy class generator for new @file standard

xjm’s picture

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

I haven't verified that it covers all of the classes' @file blocks that it should. Is there some script/sniff we can run to check that? So I haven't marked it RTBC, although if you can vouch for that, you can set it to RTBC with my blessing on the actual patch.

So I'm not 100% sure I didn't miss any since I scanned through a couple MB of phpcbf diff. ;) But if we did miss one #2676540: Fixer should not delete elaborate @file doc comments should surface it once that is fixed in Coder.

I think it's better to leave filing an @todo out of scope, because filing the issue would mean researching what it actually means and when it was filed, whereas just fixing the grammar doesn't require that extra domain of knowledge.

It's ironic that DrupalStandardsListener had a non-standard doc block. ;)

Yes, I also found that amusing.

Thanks @jhodgdon! Setting to RTBC per #4 since I think that covers all the points. It does feel weird to be setting my own patch RTBC though. ;)

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Well I did give you permission. ;)

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Can we make a sniffer for @todo blocks without issue links? :)

catch’s picture

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

No longer applies.

xjm’s picture

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

@catch, it applies fine?

xjm’s picture

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

Edit: No idea what I did but it applies to 8.0.x only currently, not 8.1.x. My attempt to rebase went weird. Attempting to reroll for 8.1.x now.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new 2676816-12.patch9.06 KB

Here is the rebased 8.1.x patch. Setting back to RTBC.

The earlier patch still applies to 8.0.x.

xjm’s picture

xjm
she/her
Primary language English
commented
Version: 8.0.x-dev » 8.1.x-dev

Hm, this is a case where our current branch assignment strategy gets confusing. But this change is only really important for 8.1.x+ anyway.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: -Novice

Untagging since there is no current novice task.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Regarding #13, I think we need to stop marking issues with the lowest branch number they're for, and do more like what we did for 7.x/8.x issues: mark with the *highest* branch number they are for (or the current branch), and add "Needs backport to ... " tags for issues that are or should be eligible to backport, after commit, to other branches. Just a suggestion... the current way of doing it is inconsistent with the 8.x/7.x thing and is very confusing for people.

+1 for RTBC on the latest patch.

  • catch committed 9611d98 on 8.2.x 
    Issue #2676816 by xjm: Consolidate miscellaneous docs from class @file...
catch’s picture

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

@jhodgdon that'd definitely help with re-testing of RTBC patches, I've been seeing a lot of conflicts the past month or so. The backport tags are hard to keep track of though as well.

Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Weird, the commit message for 8.1 didn't make it into this issue, but I checked and it was committed. Thanks!

Status: Fixed » Closed (fixed)

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