Fix 'Drupal.Commenting.TodoComment' coding standard

Created on 4 November 2020, over 3 years ago
Updated 19 June 2024, 24 days ago

Problem/Motivation

Drupal core doesn't fully follow the @todo formatting guidelines.
https://www.drupal.org/docs/develop/standards/api-documentation-and-comm...

Steps to reproduce

Add this sniff to phpcs.xmldist and run code style checks.

Proposed resolution

Run code style check tool with fixing enabled and commit the patch.

Remaining tasks

Decide if the sniff needs to be changed. The sentence following the '@todo' is not capitalized.

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

Fix @todo comment formatting.

📌 Task
Status

Fixed

Version

10.3

Component
Other 

Last updated about 15 hours ago

Created by

🇺🇸United States adamzimmermann

Live updates comments and jobs are added and updated live.
  • Coding standards

    It involves compliance with, or the content of coding standards. Requires broad community agreement.

  • Novice

    It would make a good project for someone who is new to the Drupal contribution process. It's preferred over Newbie.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Merge request !5436Resolve #3180696 "For 11.x" → (Closed) created by zniki.ru
  • Pipeline finished with Failed
    8 months ago
    Total: 176s
    #51083
  • Pipeline finished with Failed
    8 months ago
    Total: 180s
    #51096
  • Pipeline finished with Success
    8 months ago
    Total: 3570s
    #51101
  • Issue was unassigned.
  • Status changed to Needs review 8 months ago
  • 🇷🇺Russia zniki.ru

    Create new for 11.x branch. Not sure what should be done with drupalci.yml, I think if we use gitlab, then .gitlab-ci.yml should be used.
    Waiting for review.

  • Status changed to Needs work 8 months ago
  • 🇺🇸United States smustgrave

    Can the changes to the drupalci file be reverted.

  • Status changed to Needs review 8 months ago
  • 🇷🇺Russia zniki.ru

    Revert changes to the drupalci.yml file.

  • Pipeline finished with Success
    8 months ago
    Total: 666s
    #51281
  • Status changed to Needs work 8 months ago
  • 🇺🇸United States smustgrave

    So searching for "todo:" for example I still get some hits.

    So I wonder why the rule didn't find them?

  • 🇬🇧United Kingdom jonathan1055

    Open up the file > and see what they are - they might not be actual todo comments, or rather that the sniff does not think they are.

  • 🇷🇺Russia zniki.ru

    Great question.
    I found 16 results for "todo:" in .js files the core. I believe this is not the part of this issue, because phpcs doesn't support js file.

    And I made search for "todo:" in all files except .js

    Targets
        Occurrences of 'todo:' in Directory core with mask '!*.js'
    Found Occurrences in Directory core with mask '!*.js'  (15 usages found)
        Usage in comments  (13 usages found)
            drupal-2885351  (13 usages found)
                core/modules/config/tests/config_test/config/install  (1 usage found)
                    config_test.types.yml  (1 usage found)
                        10 # compliant. @todo: revisit parsing of octal once PECL YAML supports YAML 1.2.
                core/modules/media  (1 usage found)
                    media.permissions.yml  (1 usage found)
                        12 # @todo: Deprecate some permissions in https://www.drupal.org/project/drupal/issues/2925459
                core/modules/migrate_drupal/src/Plugin/migrate/source  (1 usage found)
                    ContentEntity.php  (1 usage found)
                        251 // @TODO: Determine a better way to retrieve a valid count for translations.
                core/modules/system/templates  (1 usage found)
                    checkboxes.html.twig  (1 usage found)
                        14  @todo: remove this file once https://www.drupal.org/node/1819284 is resolved.
                core/modules/system/tests/modules/entity_test/src  (1 usage found)
                    EntityTestForm.php  (1 usage found)
                        35 // @todo: Is there a better way to check if an entity type is revisionable?
                core/modules/views/src/Plugin/views/query  (1 usage found)
                    Sql.php  (1 usage found)
                        1518 // are used on the query. TODO: Find a better way to do this.
                core/modules/views_ui/src  (1 usage found)
                    ViewUI.php  (1 usage found)
                        343 // Compatibility, to be removed later: // TODO: When is "later"?
                core/themes/claro/templates/form  (1 usage found)
                    checkboxes.html.twig  (1 usage found)
                        12  @todo: remove this file once https://www.drupal.org/node/1819284 is resolved.
                core/themes/olivero/css/components  (2 usages found)
                    comments.css  (1 usage found)
                        142 /* @TODO: create image-style for profile's avatar to have image squared by default. */
                    comments.pcss.css  (1 usage found)
                        129 /* @TODO: create image-style for profile's avatar to have image squared by default. */
                core/themes/olivero/templates/includes  (1 usage found)
                    get-started.html.twig  (1 usage found)
                        8  * TODO:
                core/themes/stable9/templates/form  (1 usage found)
                    checkboxes.html.twig  (1 usage found)
                        12  @todo: remove this file once https://www.drupal.org/node/1819284 is resolved.
                core/themes/starterkit_theme/templates/form  (1 usage found)
                    checkboxes.html.twig  (1 usage found)
                        12  @todo: remove this file once https://www.drupal.org/node/1819284 is resolved.
        Usage in string constants  (2 usages found)
            drupal-2885351  (2 usages found)
                core/assets/vendor/shepherd  (1 usage found)
                    shepherd.min.js.map  (1 usage found)
                        1 nceOnHandler(selector, step);\n\n    // TODO: this should also bind/unbind on show/hide\n    let el;\n    try {\n      el = document.querySelecto
                core/assets/vendor/transliteration  (1 usage found)
                    bundle.umd.min.js.map  (1 usage found)
                        1 tringSlice = uncurryThis(''.slice);\n// TODO: Use only propper RegExpIdentifierName\nvar IS_NCG = /^\\?<[^\\s\\d!#%&*+<=>@^][^\\s!#%&*+<=>@^]*>/;
    
    

    My suggestion, we just ignore some of the file types or locations on phpcs.

  • 🇺🇸United States smustgrave

    Some of the findings in the screenshot aren't in .js file though.

    Opened the folders to help, should of done that in first screenshot.

  • 🇬🇧United Kingdom jonathan1055

    Thanks for the details. Of the six ocurrences you are showing, 2 are .yml 1 is .twig and 1 is .js and the phpcs sniff does not cover those file types.

    The last 2 are .php but the text 'todo' is in the middle of a comment. The sniff currently only detects and works on 'todo' when it is at the start of a comment (eiher // or *, with or without the @ tag). But a 'todo' word in the middle of a line, ie following some other words, is not treated as being wrong and needing to be re-formatted for standards.

    Here is the todo sniff in Coder. I recall that we commented it quite well :-)

    I suppose there could be a case for changing the Coding Standards to try to include and detect a todo in the middle of a string. Maybe raise that for discussion in the coding standards meeting?

  • 🇬🇧United Kingdom jonathan1055

    Now looking through the examples found by @zniki.ru in #46, there are two which are .php files with the 'todo' at the start of the comment.

    core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
    251 // @TODO: Determine a better way to retrieve a valid count for translations.
    
    core/modules/system/tests/modules/entity_test/src/EntityTestForm.php
    35 // @todo: Is there a better way to check if an entity type is revisionable?
    

    Those should be found and fixed by the sniff.

  • 🇷🇺Russia zniki.ru

    @jonathan1055
    These files is already fixed in the MR.
    Maybe I forgot to switch to correct git branch.

  • Status changed to Needs review 8 months ago
  • 🇬🇧United Kingdom jonathan1055

    Thanks @zniki.ru that is good to hear. So I think this is back to 'needs review' now.

  • 🇷🇺Russia zniki.ru

    Do we want to fix these violations we manually found in yml, twig, js files?

  • 🇬🇧United Kingdom jonathan1055

    Do we want to fix these violations we manually found in yml, twig, js files?

    You can do if you want to. But without any sniff/rule to enforce it, there would be nothing to prevent a new occurence being introduced later. It's up to you.

    Also, with your grep, I suggest to add -i to the options to make it case-insensitive.

  • Status changed to Needs work 8 months ago
  • 🇺🇸United States smustgrave

    Rechecked the MR and the .php files from my screenshot are still there.

    Should there be a follow up for getting the other file types?

  • Pipeline finished with Success
    3 months ago
    Total: 958s
    #155308
  • Status changed to Needs review 3 months ago
  • 🇳🇿New Zealand quietone New Zealand

    Let' get this back on track.

    1) An issue can be made in the Coding standards project to discuss 1) changing the sniff so that the sentence following the '@todo' is capitalized or not and 2) enforcing the rule for non-php files.
    2) An issue can be made in Views to decide what to do with the 'todo' statements in those files. They will take more research they are from '09 and '12. They are a special case and should not hold up this issue.

    That will allow this enhancement to be added to core.

    This also needs a 10.3.x MR.

  • Status changed to Needs work 3 months ago
  • 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.

  • Pipeline finished with Success
    3 months ago
    Total: 499s
    #156414
  • Status changed to Needs review 3 months ago
  • 🇳🇿New Zealand quietone New Zealand
  • Status changed to RTBC 3 months ago
  • 🇮🇳India Pravesh_Poonia

    Resolve 'Drupal.Commenting.TodoComment' coding standard issue by following the correct @todo formatting guidelines, Ensure that sentences following '@todo' are capitalized. Run code style checks with fixing enabled and commit the patch for consistency and compliance with Drupal coding standar

  • First commit to issue fork.
  • 🇺🇸United States xjm

    One of the updated @todos was resolved in 📌 Remove deprecated code from bootstrap and lib/Controller, lib/Config Fixed , so I rebased to update for that.

    I had a couple other merge conflicts on the update and made the following corrections:

     +   * @todo Is the hidden op operator still here somewhere, or is that part of
     +   *   the docblock outdated?
    

    (was missing indentation of the second line)

    /**                                                                             
     * Returns contextual links for each handler of a certain section.              
     *                                                                              
     * @todo Bring in relationships.                                                
     * @todo Refactor this function to use much stuff of                            
     *    views_ui_edit_form_get_bucket.                                            
     */

    (had a second, orphaned todo).

  • 🇺🇸United States xjm

    Having circular rebase issues.

  • Pipeline finished with Success
    2 months ago
    #160090
  • Status changed to Needs work 2 months ago
  • 🇺🇸United States xjm

    I had to force-push a rebase correctly built against 11.x HEAD.

    Reviewed locally with git diff --color-words. There appear to be two things not covered by the sniff:

    • Correctly casing the next word (it should be capitalized per our general docs standards)
    • Ensuring the second line of the @todo is correctly indented by two spaces

    The MR includes a mix of fixing these things in some places and not fixing them in others. There are two options:

    • Fix them all here, then have a followup to fix those things elsewhere in HEAD
    • Revert fixes to casing and spaces here, then have a followup to adjust the rule(s) and fix them everywhere.

    I am leaning toward option 2. Edit: It looks like @quietone already recommended this approach too, so we just need to revert the spacing and capitalization fixes in the existing MR.

    The URL changes are also out of scope. They are reverting previous improvements, probably from a bad rebase, because the MR has the wrong source branch and there is no 11.x in the issue repo. They should be corrected back to the https and full-path versions.

  • Pipeline finished with Success
    2 months ago
    Total: 642s
    #160114
  • 🇬🇧United Kingdom jonathan1055

    From #62, in relation to the two things not covered by the sniff, there are already existing Coder issues for both of these:
    📌 Create sniff for checking the indentation of lines that follow an @todo comment Active
    📌 @todo should start with capitalized letter or non-alpha symbol Active

  • Pipeline finished with Success
    2 months ago
    Total: 677s
    #163397
  • Status changed to Needs review 2 months ago
  • 🇳🇿New Zealand quietone New Zealand

    I restored the URLs and made a fix for a recent commit. I didn't spot any casing errors but it has been a long day for me.

  • Status changed to RTBC 2 months ago
  • 🇺🇸United States smustgrave

    Not sure if this should be postponed till the 2 tickets in #63 are done. But marking because this does make a little better now. Can open a follow up though that's actually postponed on those 2 thoughts?

  • 🇬🇧United Kingdom jonathan1055

    The current MR changes 123 files, I've not re-read it again in detail, but it seems good to get those fixed now and not wait on the two subsequernt Coder sniff issues. If we wait, there will be a lot of conflicts/manual fixing again, and probably more new incorrectly formatted ones. Let's not waste the work done here so far, but get it looked in and committed. Doing that now will not make it any harder to fix the finer points when those later issues land.

  • 🇺🇸United States xjm

    This definitely should not be postponed on the other issues; rather, this should go in first.

    There are still out-of-scope changes per #62. For example, see the change in core/modules/content_moderation/content_moderation.module and core/modules/system/src/Controller/AssetControllerBase.php.

    There is also still a bad URL change in core/modules/file/tests/src/Functional/MultipleFileUploadTest.php and in other places.

    core/modules/locale/src/PluralFormula.php is adding a grammatically incomplete sentence to fill out the @todo rule.

  • Status changed to Needs work 2 months ago
  • 🇺🇸United States xjm

    That said, removing the out-of-scope changes will be a good novice task. I recommend using git diff --color-words to more easily spot the out-of-scope changes.

  • First commit to issue fork.
  • Pipeline finished with Failed
    2 months ago
    Total: 531s
    #167901
  • Status changed to Needs review 2 months ago
  • 🇺🇸United States SuperfluousApostrophe

    I worked on this at #portland2024 & updated the MR to fix another few coding standard items.

  • Status changed to Needs work 2 months ago
  • 🇺🇸United States mradcliffe USA

    It looks there are additional changes that are yet to be made that @xjm requested in #68 so I am changing this back to Needs work.

  • First commit to issue fork.
  • Pipeline finished with Success
    about 2 months ago
    Total: 511s
    #181197
  • Pipeline finished with Success
    about 2 months ago
    Total: 510s
    #181355
  • Status changed to Needs review about 2 months ago
  • 🇮🇳India manish-31

    I have rebased the MR, resolved conflicts from /core/modules/system/tests/src/Functional/Menu/LocalTasksTest.php and /core/modules/system/src/Controller/AssetControllerBase.php

    Also, added some more fixes as per the comment #62.

  • Status changed to RTBC about 2 months ago
  • 🇺🇸United States smustgrave

    Appears out of scope changes have been reverted.

  • Status changed to Needs work about 1 month ago
  • 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.

  • Status changed to Needs review about 1 month ago
  • 🇮🇳India manish-31

    Resolved merge conflicts. Needs review.

  • Pipeline finished with Success
    about 1 month ago
    Total: 673s
    #190973
  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    Updating credits

    • larowlan committed 742abe0a on 11.0.x
      Issue #3180696 by Spokje, Nikolay Shapovalov, adamzimmermann, quietone,...
    • larowlan committed d01e3e2d on 11.x
      Issue #3180696 by Spokje, Nikolay Shapovalov, adamzimmermann, quietone,...
  • Status changed to Downport about 1 month ago
  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    Committed to 11.x and 11.0.x. Needs a 10.4.x and 10.3.x specific version

    Would be good to get that in today before the freeze

  • Merge request !828910.4.x todocomment → (Closed) created by quietone
  • Status changed to Needs review about 1 month ago
  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
  • Status changed to RTBC about 1 month ago
  • 🇦🇺Australia sime Canberra

    I have review all changes and confirm that only comments are being changed. Mostly these are @todos but there is a small amount of whitespace.

    I tested the new rule

    These pass

      // @todo Fix this hey there.
    
      // @todo Fix this hey there
      //   Fix this hey there
    

    These fail

      //  @todo Fix this hey there.
    
      //  todo hey there
    
      // @todo: hey there
    
      // @todo hey there  
    

    I didn't dive further into /** */ - everything in core is passing as bugs would be created on the rule if an invalid format is not throwing an error.

    • larowlan committed 54d8b195 on 10.3.x
      Issue #3180696 by Spokje, Nikolay Shapovalov, adamzimmermann, quietone,...
    • larowlan committed df2dfa91 on 10.4.x
      Issue #3180696 by Spokje, Nikolay Shapovalov, adamzimmermann, quietone,...
  • Status changed to Fixed about 1 month ago
  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    Reviewed locally with --color-words
    Committed to 10.4.x and backported to 10.3.x
    Ran phpcs for that rule for 10.3.x against all files locally and it passed

    phpcs --standard=core/phpcs.xml.dist -p --sniffs=Drupal.Commenting.TodoComment core

    Asking @quietone if we normally do a change record for this kind of change.

  • 🇳🇿New Zealand quietone New Zealand

    According to the recently added Change records , a CR is not needed here.

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

Production build 0.69.0 2024