Fix 'Drupal.Commenting.DocComment.ShortNotCapital' coding standard for callbacks

going to reroll it

re-rolled

Added some part of work an additional
@Jo, it may be useful don't put it to status "Needs review" until work done?

I set the status to "Needs review" to run the testbot on the patch you had re-rolled. Why did you not want it run? Similarly, why have you not set the status to "Needs review for your latest patch in #8?

I set the status to "Needs work" because the task isn`t finished (I did it similarly to the comment #3)
Anyway, I`m willing to get all the remarks/advises about work process. I`m new in contribution ) Thanks

It makes sense to run tests anyway - to get stats about how many inconsistencies left

Re-rolled.

@Saviktor Are you working against the latest version of the 8.5.x branch? That might explain why your patch failed to apply.

Patch in #14 no longer applies. Re-rolled.

Re-rolled #14.

Thanks for the review @mfernea.

Removed the out of scope changes and corrected a few more comments that didn't start with capital letters.

Most (if not all) of the remaining 'errors' are comments starting "(optional) ..." which I would argue should be allowed.

Any thoughts?

Removed all instances of (optional) at the beginning of comments and fixed the other coding standards error that had been introduced by the patch.

I could not think of a shorter comment that could remain on 1 line. I'm open to suggestions.

1. +++ b/core/modules/content_translation/content_translation.admin.inc
   @@ -165,7 +165,7 @@ function _content_translation_is_field_translatability_configurable(EntityTypeIn
   - * (proxied) Implements hook_preprocess_HOOK();
   + * Implements hook_preprocess_HOOK() (proxied);
   

   Do we need to fix this ; in this issue as well? It should be a period instead of the semicolon.

2. +++ b/core/modules/views/src/Annotation/ViewsAccess.php
   @@ -30,7 +30,7 @@ class ViewsAccess extends ViewsPluginAnnotationBase {
   -   * (optional) The short title used in the views UI.
   +   The short title used in the views UI.
   

   This removes the *, making it invalid.

3. +++ b/core/modules/views/src/Annotation/ViewsArgumentDefault.php
   @@ -30,7 +30,7 @@ class ViewsArgumentDefault extends ViewsPluginAnnotationBase {
   -   * (optional) The short title used in the views UI.
   +   The short title used in the views UI.
   

   Same here.

4. +++ b/core/modules/views/src/Annotation/ViewsArgumentValidator.php
   @@ -30,7 +30,7 @@ class ViewsArgumentValidator extends ViewsPluginAnnotationBase {
   -   * (optional) The short title used in the views UI.
   +   The short title used in the views UI.
   
   +++ b/core/modules/views/src/Annotation/ViewsCache.php
   @@ -30,7 +30,7 @@ class ViewsCache extends ViewsPluginAnnotationBase {
   -   * (optional) The short title used in the views UI.
   +   The short title used in the views UI.
   
   +++ b/core/modules/views/src/Annotation/ViewsDisplay.php
   @@ -30,7 +30,7 @@ class ViewsDisplay extends ViewsPluginAnnotationBase {
   -   * (optional) The short title used in the views UI.
   +   The short title used in the views UI.
   
   +++ b/core/modules/views/src/Annotation/ViewsDisplayExtender.php
   @@ -30,7 +30,7 @@ class ViewsDisplayExtender extends ViewsPluginAnnotationBase {
   -   * (optional) The short title used in the views UI.
   +   The short title used in the views UI.
   
   +++ b/core/modules/views/src/Annotation/ViewsExposedForm.php
   @@ -31,7 +31,7 @@ class ViewsExposedForm extends ViewsPluginAnnotationBase {
   -   * (optional) The short title used in the views UI.
   +   The short title used in the views UI.
   
   +++ b/core/modules/views/src/Annotation/ViewsPager.php
   @@ -30,7 +30,7 @@ class ViewsPager extends ViewsPluginAnnotationBase {
   -   * (optional) The short title used in the views UI.
   +   The short title used in the views UI.
   
   +++ b/core/modules/views/src/Annotation/ViewsQuery.php
   @@ -30,7 +30,7 @@ class ViewsQuery extends ViewsPluginAnnotationBase {
   -   * (optional) The short title used in the views UI.
   +   The short title used in the views UI.
   
   +++ b/core/modules/views/src/Annotation/ViewsRow.php
   @@ -30,7 +30,7 @@ class ViewsRow extends ViewsPluginAnnotationBase {
   -   * (optional) The short title used in the views UI.
   +   The short title used in the views UI.
   
   +++ b/core/modules/views/src/Annotation/ViewsStyle.php
   @@ -30,7 +30,7 @@ class ViewsStyle extends ViewsPluginAnnotationBase {
   -   * (optional) The short title used in the views UI.
   +   The short title used in the views UI.
   
   +++ b/core/modules/views/src/Annotation/ViewsWizard.php
   @@ -31,7 +31,7 @@ class ViewsWizard extends ViewsPluginAnnotationBase {
   -   * (optional) The short title used in the views UI.
   +   The short title used in the views UI.
   

   More of those.

5. +++ b/core/tests/Drupal/Tests/Core/Render/RendererRecursionTest.php
   @@ -26,7 +26,8 @@ protected function setUpRenderRecursionComplexElements() {
   -   * ::renderRoot() may not be called inside of another ::renderRoot() call.
   +   * The method ::renderRoot() may not be called inside of another
   +   * ::renderRoot() call.
   

   Verifies that ::renderRoot() is not called in another ::renderRoot() call.

   Does that make sense?

I've made the above mention changes in a new patch.

uploading correct interdiff.

Addressed @borisson_'s point 5 from #23.

@mfernea But the comment above (I assume you mean lines 32-34) is not currently in alphabetical order.

Added ShortNotCapital to the list of errors to sniff for.

1. +++ b/core/modules/field/tests/modules/field_test/src/Plugin/Field/FieldWidget/TestFieldWidgetMultiple.php
   @@ -94,6 +94,7 @@ public static function multipleValidate($element, FormStateInterface $form_state
      /**
       * {@inheritdoc}
   +   *
       * Used in \Drupal\field\Tests\EntityReference\EntityReferenceAdminTest::testAvailableFormatters().
       */
   

   This docblock is already invalid; either {@inheritdoc} should be the only thing in the docblock, or it needs to override the whole thing. We could spin this off into a separate issue.

2. +++ b/core/modules/media/src/Annotation/MediaSource.php
   @@ -80,7 +80,7 @@ class MediaSource extends Plugin {
   -   * (optional) The metadata attribute name to provide the thumbnail alt.
   +   * The metadata attribute name to provide the thumbnail alt.
   
   @@ -89,7 +89,7 @@ class MediaSource extends Plugin {
   -   * (optional) The metadata attribute name to provide the thumbnail title.
   +   * The metadata attribute name to provide the thumbnail title.
   
   +++ b/core/modules/views/src/Annotation/ViewsArgumentDefault.php
   @@ -30,7 +30,7 @@ class ViewsArgumentDefault extends ViewsPluginAnnotationBase {
   -   * (optional) The short title used in the views UI.
   +   * The short title used in the views UI.
   
   +++ b/core/modules/views/src/Annotation/ViewsArgumentValidator.php
   @@ -30,7 +30,7 @@ class ViewsArgumentValidator extends ViewsPluginAnnotationBase {
   -   * (optional) The short title used in the views UI.
   +   * The short title used in the views UI.
   
   +++ b/core/modules/views/src/Annotation/ViewsCache.php
   @@ -30,7 +30,7 @@ class ViewsCache extends ViewsPluginAnnotationBase {
   -   * (optional) The short title used in the views UI.
   +   * The short title used in the views UI.
   
   +++ b/core/modules/views/src/Annotation/ViewsDisplay.php
   @@ -30,7 +30,7 @@ class ViewsDisplay extends ViewsPluginAnnotationBase {
   -   * (optional) The short title used in the views UI.
   +   * The short title used in the views UI.
   
   +++ b/core/modules/views/src/Annotation/ViewsDisplayExtender.php
   @@ -30,7 +30,7 @@ class ViewsDisplayExtender extends ViewsPluginAnnotationBase {
   -   * (optional) The short title used in the views UI.
   +   * The short title used in the views UI.
   
   +++ b/core/modules/views/src/Annotation/ViewsExposedForm.php
   @@ -31,7 +31,7 @@ class ViewsExposedForm extends ViewsPluginAnnotationBase {
   -   * (optional) The short title used in the views UI.
   +   * The short title used in the views UI.
   

   So, this issue is doing the opposite of #2909370: Fix 'Drupal.Commenting.VariableComment.IncorrectVarType' coding standard.

   Looking at all the changes in this patch, many of which are to work around what is probably valid documentation, I'm thinking we need to fix this rule to be less strict, instead of fixing it. At least this particular pattern of a parenthetical before a sentence that starts with a capital and ends with a period should be allowed. So let's file an issue to tweak the rule, and postpone this issue on that.

Thanks!

It looks like we have a standard for the callbacks here that also doesn't match how we're fixing it in the patch: https://www.drupal.org/node/1354#callback-def

Friendly post #2902381: Revert phpcs.xml.dist changes from "Ensure that we're using the right ruleset for coding standards checking" reroll. No feedback addressed. No credit needed.

Just ran a test-run locally with this sniff, a lot seems to have changed for the better.
Unpostponing.

Moving in to MR-land.

I am not 100% convinced this sniff adds anything useful, it doesn't help readability for me. It will also slow down patch authoring if test runs start to fail just because of this sniff, when they wouldn't otherwise.

I am not 100% convinced this sniff adds anything useful, it doesn't help readability for me.

Not sure about this one either, for me it also doesn't add much, however readability is about the same for me before and after this sniff.

One could argue that the "real bad" non-capitalized comments were fixed in other sniffs, and that they might sneak in again in new code without this sniff.

One could argue that the sniff should not check for comments starting with numbers or "non-letters" (uses Get-Out-Of-Jail-Free-Card-Because-I-Am-A-Non-Native-Speaker card there) like !@#$%^&*().

Personally I would just turn this sniff on to prevent future "nasty" comments.

It will also slow down patch authoring if test runs start to fail just because of this sniff, when they wouldn't otherwise.

Although true, I think this can be said about any sniff, so I'm not sure if it's a valid argument

I disagree with #51 so went ahead and reviewed the changes, no worries if they are not used. Personally, I do like the comment starting with 'The' and not '#'. And since this sniff picked up only 60 files, it is very unlikely to be disruptive. That is people are already writing new summary lines which will pass this sniff.

And since this sniff picked up only 60 files, it is very unlikely to be disruptive. That is people are already writing new summary lines which will pass this sniff.

Thus spoke the mighy @quietone in #54

That alone is (at least for me) a good reason to think it's not disruptive.

If it makes more readable code is up for debate, but is, of course, a matter of personal taste.
@quietone likes it, @longwave doesn't.

Personally I say: Turn this sniff on, since it's basically non-disruptive. It will only catch the (small amount of) people that incidentally write something that's not cAPITALIZED correctly.

For that reason I addressed all the issues found by @quietone and put this on NR again.

I read through the changes and it all looks good. For me, this is RTBC. Let's wait for longwave to chime in again.

Some of the changes here have swayed me to think that this is a good idea. Added a few comments/notes as I went along.

2 unresolved threats remaining

Added suggestions for the two outstanding.

Thanks @longwave for the suggestions.

Changes made, all threads resolved -> NR

Great, it's time to see what core committers think.

Random test fail on last run, triggered new test run.

Failed asserting that 'unknown error: session deleted because of page crash\n
from unknown error: cannot determine loading status\n
from tab crashed\n

Another instance of #3208791: [random test failure] Random fail in LayoutBuilderDisableInteractionsTest

Question/comment from me in the MR..

An attempt to comment on the question/comment/suggestion/whatever from @catch in the MR, in the...
....
Wait for it....
.
..
...
MR :)

Rebased MR against 9.3.x.

I did a review and the patch fixes all doc comments that start with upper case character.
I'll move it RTBC and see if @catch will point any thing about #64.
I'm not attaching pictures from my local test because Drupal Automated Test would get it, but I ran ../vendor/bin/phpcs --extensions=php -p ./ and no error is displayed.

Thanks @paulocs for the Review.

I gave up on keeping MRs mergeable for non-RTBC issues, because somehow GitLab is notoriously bad at rebasing/merging with HEAD on Drupal (not noticed this on other projects), but since you made it an RTBC, here's a merge with HEAD from 9.3.x to get the Merge request !540 showing up in Green again.

Random test-failure #3214565: [random test failure] Random fail in BuildTestTest::testPortMany

I think we should patch coder to be more permissive - coder should follow core coding standards rather than the other way around.

@catch: Although I agree with what you're saying I also see a lot of effort put in this issue now slowly being eroded by new code and code changes.

Oh well, let's wait until coder gets patched.

This is disappointing. This sniff is implementing what is in the coding standards. Does this mean the coding standards page is wrong and needs to be edited?

Added to the IS that this is postponed on #3221049: Let ShortNotCapital allow starting with some characters that do not have case

Revisiting this. I am still not sure about the postponing of this issue. The standard states

All summaries (first lines of docblocks) must be under 80 characters, start with a capital letter, and end with a period (.).

For me, that means that the rule is, in fact, following the standard.

And further, I don't think that the changes made here are following the standard for callbacks either.

I am going to split this into two issues, one for the non-callbacks and one for the callbacks.

Looks like the last run on the merge request has some remaining failures, but I also have a question (posted on the MR).

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.

The remaining failures are expected because this issue is only for callbacks. The sibling issue needs to be committed first #3508410: Fix 'Drupal.Commenting.DocComment.ShortNotCapital' coding standard except for callbacks.

But I wanted to see if these are correct in content and meet the special standard for callbacks.

Looks like the standard is indeed clear, but I didn't know that part of it, so for me this looks good.

Actually, I think I can rtbc this, as long as the committers know this can only be committed after the sibling issue.

committed sibling issue, merge conflict to resolve now :)

Rebased and fixed all violations for callbacks. Any other violations will be handled in a separate issue.

Since it is now possible to enable the sniff in most of core, this does change the phpcs.xml.dist

Ok, back to rtbc it goes.

Committed 52e18cc and pushed to 11.x. Thanks!

There is a mistake in this commit, in ohpcs.xml.idst. Instead of adding this,

  <rule ref="Drupal.Commenting.DocComment">
    <exclude-pattern>core/tests/*</exclude-pattern>
  </rule>

it should have been

<rule ref="Drupal.Commenting.DocComment.DocComment.ShortNotCapital">
    <exclude-pattern>core/tests/*</exclude-pattern>
</rule>

Maybe this can be resolved in #3511759: Enable 'Drupal.Commenting.DocComment.ShortNotCapital' coding standard ?