Problem/Motivation

Should be a new line between description and @param.

Hardly seems worth opening an issue for, but should be sorted one way or another.

Proposed resolution

Add the line

Remaining tasks

Review and commit.

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

timmillwood created an issue. See original summary.

cilefen’s picture

Title: Nit pick: missing new line in docblock » Fix Drupal.Commenting.DocComment.SpacingBeforeTags coding standard
Component: content_moderation.module » other
Status: Needs review » Needs work
Issue tags: -Workflow Initiative +Novice, +Coding standards
Parent issue: » #2571965: [meta] Fix PHP coding standards in core

I think we've been scoping these like this. I count 27 cases in core/modules alone.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Ketan Harit’s picture

Assigned: Unassigned » Ketan Harit
arunkumark’s picture

Status: Needs work » Needs review
Issue tags: +ChennaiSprint2017
FileSize
534 bytes

Hi,

I have re-rolled patch for latest Drupal 8.4.x version.

oo0shiny’s picture

Status: Needs review » Reviewed & tested by the community

Tested this locally on an 8.4.x install. Patch applied successfully and the changes showed up as they should. Moving this to RTBC!

dawehner’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +SprintWeekend2017, +SprintWeekendBerlin

@arunkumark
On top of that we need to add the new rule into our phpcs.xml.dist file, so we can get automatic checking of it.

cilefen’s picture

Plus, see comment #2, where I found more than one.

rigoucr’s picture

I removed the phpcs rule from the blacklist and fixed the errors

Status: Needs review » Needs work

The last submitted patch, 9: drupal-comment-coding-standard-2842949-9-d8.patch, failed testing.

rigoucr’s picture

Status: Needs work » Needs review

All the tests passed

rigoucr’s picture

kporras07’s picture

Hi,
I just tested this and all coding standard passes after removing the rule from the blacklist with the patch in #12.
Marking as RTBC

kporras07’s picture

Status: Needs review » Reviewed & tested by the community

oops! Now I marked it as RTBC

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

It is important to review generated patches especially. Looking through at least found these problems and a couple questions:

  1. +++ b/core/lib/Drupal/Core/Entity/EntityStorageInterface.php
    @@ -9,6 +9,7 @@
      * implementations are used by default when the @ContentEntityType or
    + *
      * @ConfigEntityType annotations are used.
    

    This change does not seem to be right.

  2. +++ b/core/lib/Drupal/Core/Logger/LogMessageParserInterface.php
    @@ -13,6 +13,7 @@
        *   - PSR3 format:
    +   *
        *     @see https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-3-logger-interface.md#12-message
        *   - Drupal specific string placeholder format:
        *     @see \Drupal\Component\Utility\SafeMarkup::format()
    
    --- a/core/lib/Drupal/Core/Menu/menu.api.php
    +++ b/core/lib/Drupal/Core/Menu/menu.api.php
    

    Also does not seem to be the right change.

  3. +++ b/core/lib/Drupal/Core/Render/Element/Select.php
    @@ -26,6 +26,7 @@
      *     since all user agents automatically preselect the first available
      *     option. But people are used to this being the behavior of select
      *     controls.
    + *
      *     @todo Address the above issue in Drupal 8.
      *   - If #required is not TRUE and this value is set (most commonly to an
      *     empty string), then an extra option (see #empty_option above)
    

    Should this newline change apply to @todo items in a list like this?

  4. +++ b/core/lib/Drupal/Core/Template/Loader/FilesystemLoader.php
    @@ -10,6 +10,7 @@
      * This loader adds module and theme template paths as namespaces to the Twig
      * filesystem loader so that templates can be referenced by namespace, like
    + *
      * @block/block.html.twig or @mytheme/page.html.twig.
    

    Also incorrect.

  5. +++ b/core/phpcs.xml.dist
    @@ -29,7 +29,6 @@
         <exclude name="Drupal.Commenting.DocComment.ParamNotFirst"/>
    -    <exclude name="Drupal.Commenting.DocComment.SpacingBeforeTags"/>
         <exclude name="Drupal.Commenting.DocComment.LongFullStop"/>
    

    Hm, why are we removing this?

dawehner’s picture

@Gábor Hojtsy
I'm confused by your review ... So

  1. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextProviderInterface.php
    @@ -9,6 +9,7 @@
      *
      * The fully qualified context ID then includes the service ID:
    + *
      * @{service_id}:{unqualified_context_id}.
      *
    

    This means we need a @code @endcode statement around this line, I guess?

  2. +++ b/core/phpcs.xml.dist
    @@ -29,7 +29,6 @@
           @see https://www.drupal.org/node/2253915 -->
         <exclude name="Drupal.Commenting.DocComment.ParamNotFirst"/>
    -    <exclude name="Drupal.Commenting.DocComment.SpacingBeforeTags"/>
         <exclude name="Drupal.Commenting.DocComment.LongFullStop"/>
         <exclude name="Drupal.Commenting.DocComment.ShortNotCapital"/>
    

    What this patch is doing is to no longer exclude the "SpcaingBeforeTags" rule, so removing this line is exactly expected.

Gábor Hojtsy’s picture

@dawehner: you are right on point 2, I did not realize that is *phpdoc* tags, my bad, sorry.

For the others, I don't necessarily know what the fix would be, but when a phpdoc tag is on the first character of a line, it could also be part of a sentence as shown in the examples I found. I am not sure putting them in code tags is necessarily better, you would need to put the other one as well in the example you cited. But the proposed change for those lines does seem like incorrect.

gaurav.kapoor’s picture

Fixed according to suggestions in 15,16 and 17. Still not sure about this change.

+++ b/core/lib/Drupal/Core/Render/Element/Select.php
@@ -26,6 +26,7 @@
  *     since all user agents automatically preselect the first available
  *     option. But people are used to this being the behavior of select
  *     controls.
+ *
  *     @todo Address the above issue in Drupal 8.
  *   - If #required is not TRUE and this value is set (most commonly to an
  *     empty string), then an extra option (see #empty_option above)

Status: Needs review » Needs work

The last submitted patch, 18: drupal-comment-coding-standard-2842949-18-d8.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Gábor Hojtsy’s picture

I don't think removing the newlines would work as the fail showed, since those lines do not match the coding standards then :) Yet adding the newlines would break the code comments.

xjm’s picture

The fail is not about coding standards, because the automated failures for the coding standards are not happening yet. You can see the actual fail from the "view results" link: https://www.drupal.org/pift-ci-job/695012 That's an actual test failure in LazyContextRepositoryTest.

The coding style fails are further down on the results:

core/lib/Drupal/Core/Entity/EntityStorageInterface.php
line 12	There must be exactly one blank line before the tags in a doc comment
core/lib/Drupal/Core/Logger/LogMessageParserInterface.php
16	There must be exactly one blank line before the tags in a doc comment
core/lib/Drupal/Core/Template/Loader/FilesystemLoader.php
13	There must be exactly one blank line before the tags in a doc comment

In SqlContentEntityStorage and EntityStorageInterface; they are false fails that we need to skip somehow, reorderm or address in coder.

In LogMessageParserInterface the comment is just malformatted entirely. @see should be the last section in the docblock, not inline in another section. If we really absolurely need to link it inline in the exact spot, link tags can be used inline instead like:
See @link whatever @endlink.

The comment should probably be rewritten for LogMessageParserInterface; I'd open a separate blocking issue for that.

harsha012’s picture

Status: Needs work » Needs review
FileSize
36.66 KB

fixing the test and also the coding standard.

pk188’s picture

FileSize
2.34 KB

Interdiff #18 and #22. Not sure about some changes of #22.

xjm’s picture

Assigned: Ketan Harit » Unassigned
Status: Needs review » Postponed

Thanks @pk188 for the interdiff.

Unfortunately none of the changes from #22 are complete or correct, so I'm removing the issue credit for now for @harsha012. Let's make sure to only change things in the way that they should be changed, rather than rewriting them in an incorrect way to make the rule pass.

  1. +++ b/core/lib/Drupal/Core/Logger/LogMessageParserInterface.php
    @@ -13,7 +13,9 @@
        *   - Drupal specific string placeholder format:
        *     @see \Drupal\Component\Utility\SafeMarkup::format()
    

    We'll need to fix this to be @link as well.

  2. +++ b/core/lib/Drupal/Core/Template/Loader/FilesystemLoader.php
    @@ -10,7 +10,7 @@
    - * @block/block.html.twig or @mytheme/page.html.twig.
    + * block.html.twig or page.html.twig.
    

    I am fairly certain this change is not correct as the @ is actually meaningful here. See core/themes/seven/templates/block--local-actions-block.html.twig which has:

    {% extends "@block/block.html.twig" %}
    

    For this line, we could enclose them in single quotes, but I actually think the coder rule has too many false positives. Maybe the rule should check against a list of defined docblock tags.

  3. +++ b/core/includes/file.inc
    @@ -436,6 +436,7 @@ function file_valid_uri($uri) {
      *   will rename the file until the $destination is unique.
      * - Works around a PHP bug where copy() does not properly support streams if
      *   safe_mode or open_basedir are enabled.
    + *
      *   @see https://bugs.php.net/bug.php?id=60456
      *
      * @param $source
    
    @@ -621,6 +622,7 @@ function file_destination($destination, $replace) {
      *   replace the file or rename the file based on the $replace parameter.
      * - Works around a PHP bug where rename() does not properly support streams if
      *   safe_mode or open_basedir are enabled.
    + *
      *   @see https://bugs.php.net/bug.php?id=60456
      *
      * @param $source
    

    These @see should be moved to the end of their docblocks instead, or should use @link. Simply adding a blank line and leaving the list indentation will cause it to be malformed.

  4. +++ b/core/lib/Drupal/Component/Diff/DiffFormatter.php
    @@ -10,6 +10,7 @@
      * This class formats the diff in classic diff format.
      * It is intended that this class be customized via inheritance,
      * to obtain fancier outputs.
    + *
      * @todo document
      * @private
    
    +++ b/core/lib/Drupal/Component/Diff/MappedDiff.php
    @@ -4,6 +4,7 @@
     
     /**
      * FIXME: bad name.
    + *
      * @todo document
      * @private
      * @subpackage DifferenceEngine
    

    As @sugaroverflow and I discussed a bit ago, this code is a third-party library that we've pulled into core, so it should probably be skipped. #2887052: Ignore Diff component files in phpcs coding standards is the related issue to skip it. So once that lands, we can remove these hunks from the patch.

  5. +++ b/core/lib/Drupal/Core/Asset/CssCollectionRenderer.php
    @@ -16,6 +16,7 @@
      *   files are downloaded in parallel, resulting in faster page load, but if
    + *
      *   @import statements are used and span across multiple STYLE tags, all the
    

    This is just splitting up a paragraph into two paragraphs and is not correct. I'm starting to think we need to improve the rule somehow to only check for existing docblock tags rather than anything with an @ sign.

  6. +++ b/core/lib/Drupal/Core/Entity/EntityStorageInterface.php
    @@ -7,9 +7,10 @@
      * For common default implementations, see
      * \Drupal\Core\Entity\Sql\SqlContentEntityStorage for content entities and
    - * \Drupal\Core\Config\Entity\ConfigEntityStorage for config entities. Those
    - * implementations are used by default when the @ContentEntityType or
    - * @ConfigEntityType annotations are used.
    + * \Drupal\Core\Config\Entity\ConfigEntityStorage for config entities.
    + *
    + * @ContentEntityType
    + *   it is used by default or can be used by providing this annotation.
    

    This change is incorrect and should be removed.

  7. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextProviderInterface.php
    @@ -9,7 +9,10 @@
    - * @{service_id}:{unqualified_context_id}.
    + *
    + * @code
    + * {service_id}:{unqualified_context_id}.
    + * @endcode
    

    Here as well, removing the @ is incorrect, because it is actually meaningful.

  8. +++ b/core/lib/Drupal/Core/Render/Element/Select.php
    @@ -26,7 +26,9 @@
    + *
      *     @todo Address the above issue in Drupal 8.
    + *
    

    The @todo should be moved to the bottom of the docblock (with more explanation of "this" and a dedicated issue for it... also, clearly, this did not get fixed in Drupal 8).

  9. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
    @@ -22,6 +22,7 @@
      * For attachment handling of HTML responses:
    + *
    

    Hmm, this change is not really correct either.

  10. +++ b/core/modules/views/src/Plugin/views/access/AccessPluginBase.php
    @@ -31,6 +31,7 @@
    + *
      * @Plugin(
      *   id = "denyall",
    

    This should be wrapped in a @code / @endcode and indented properly instead.

So, before we proceed with this issue, we should maybe open an issue to discuss whether to make this coder rule more focused so there are fewer false positives. Thanks!

harsha012’s picture

@xjm , is this issue queue is postponed till we get the proper solution related to the coding standard issue for doc block.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

jungle’s picture

Issue tags: +Needs followup

So, before we proceed with this issue, we should maybe open an issue to discuss whether to make this coder rule more focused so there are fewer false positives. Thanks!

-- #24

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

quietone’s picture

Category: Bug report » Task

Pretty sure all the coding standards issues are Tasks not Bugs.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.