Fix 'Drupal.Commenting.TodoComment' coding standard

This is a great start. To make it better, you can do two things

1. get the new sniff running within these tests, because without it you will not be able to demonstrate any benefit, because we wont see the errors before the fix
2. core patch testing takes a long time and uses resources. When fixing coding standards you can get quick results by only running phpcs, and skipping the phpunit section. Then when all standards are fixed, put back the phpunit calls for final running.

Await new release of coder (or pull in dev branch)

Just for background info, in #3048495: Fix Drupal.Semantics.FunctionTriggerError coding standard I tried to pull in the -dev release. In /core/composer.json I tried

-        "drupal/coder": "^8.3.2",
+        "drupal/coder": "dev-3.x",

which did not work. I tried also to get the specific commit

-        "drupal/coder": "^8.3.2",
+        "drupal/coder": "dev-3.x#efe6ac6",

Then I tried changing /core/drupalci.yml

       phpcs:
         sniff-all-files: false
         halt-on-fail: false
+        coder-version: ^8.3@dev

None of the above worked. But finally I did get the new sniff, by patching Coder from within /core/drupalci.yml

--- a/core/drupalci.yml
+++ b/core/drupalci.yml
@@ -4,6 +4,9 @@
 build:
   assessment:
     validate_codebase:
+      host_command:
+        commands:
+          - 'cd vendor/drupal/coder; sudo -u www-data curl "https://www.drupal.org/files/issues/2019-05-27/2908391-63.trigger_error_for_8.3.2.patch" | sudo -u www-data patch -p1'
       phplint:

This patch from comment #19 in the issue linked above is an example of how to change to drupalci.yml to halt after running the coding standards. I hope these two ideas help, and will save you time.

Not sure what has happened, but in the dispatcher output, phpcs sniffs.txt we get

ERROR: Referenced sniff "Drupal.Commenting.TodoComment" does not exist
Run "phpcs --help" for usage information

Maybe the Coder version change does not work. The log just has

Installing drupal/coder (8.3.10): Cloning e1d71c6bb7 from cache

with no mention of 8.3.11. I could not make that work, and ended up having to use a patch on Coder to get the new sniff, see my comments in #3 above.

It also has problems with @see and @covers annotations. Are those handled in this updated coder? Should I open some other issues for them?

git grep '\* @[a-zA-Z]*:'

Not sure I understand what previous release you're referring to.I was trying to understand if the @todo sniff a generalized annotation sniffer or does it only fix todo problems?

core/modules/jsonapi/tests/modules/jsonapi_test_non_cacheable_methods/jsonapi_test_non_cacheable_methods.module: * @see: https://www.drupal.org/project/drupal/issues/3072076.
core/modules/migrate/tests/src/Unit/RowTest.php:   * @covers::getMultiple
core/modules/user/src/Event/UserEvents.php:   * @see: \Drupal\user\UserFloodControl::isAllowed
core/modules/user/src/Event/UserEvents.php:   * @see: \Drupal\user\EventSubscriber\UserFloodSubscriber
core/modules/user/src/Event/UserEvents.php:   * @see: \Drupal\user\UserFloodControl::isAllowed
core/modules/user/src/Event/UserEvents.php:   * @see: \Drupal\user\EventSubscriber\UserFloodSubscriber
core/modules/user/src/UserFloodControl.php: * @see: \Drupal\Core\Flood\DatabaseBackend
core/tests/Drupal/Tests/Composer/ComposerTest.php:   * @covers::ensureComposerVersion
core/tests/Drupal/Tests/Core/Field/FieldFilteredMarkupTest.php:   * @covers: ::displayAllowedTags
core/tests/Drupal/Tests/Core/Utility/UnroutedUrlAssemblerTest.php:   * @covers::buildLocalUrl
core/themes/olivero/css/components/comments.css:    /* @TODO: create image-style for profile's avatar to have image squared by default. */
core/themes/olivero/css/components/comments.pcss.css:    /* @TODO: create image-style for profile's avatar to have image squared by default. */

I am putting this on pause until coder 8.3.11 is included in Drupal core.

Core 9.2 is currently using 8.3.10.

(@adamzimmermann in #9)

Core 9.2 is now using

            "name": "drupal/coder",
            "version": "8.3.12",

Un-postponing.

Added parent issue.

One little nit and one question about style, otherwise this looks pretty good.

Commented on style and fixed nit.

My review may include items out of scope - I am not sure. There are lots of links that use 'project/drupal' instead of just node, not sure if that can be fixed here or not.

To speed up the test runs, and to save resources, can you implement the changes to core/drupalci.yml as in patch #5. Not the change to use coder 8.3.11 but the change to remove everything below that. Then the tests just run phpcs and do not waste time and effort running phpunit.

quietone said

My review may include items out of scope - I am not sure ... if that can be fixed here or not.

I think we need to:

1. Make sure the tests are using the new sniff
2. Find out why the automatic fixer did not change lowercase to uppercase initial letter (I am sure @adamzimmermann added that as a major help when fixing these)
3. Change the test file temporarily to not run phpunit (as per my suggestion in #23)

Only when all three of these are done should we consider anything manual, such as the changes that quietone has highlighted.

@quitone in #22: Fixed them anyway. Maybe a follow-up to check throughout the whole source (or basically module `jsonapi`:innocent:)

@jonathan1055 in #23

Make sure the tests are using the new sniff
Find out why the automatic fixer did not change lowercase to uppercase initial letter (I am sure @adamzimmermann added that as a major help when fixing these)

I have no clue how to do both of them. I'll just put it back to Needs Work.

Sorry about the manual changes, I only read your comment after I've done those already.

Tested locally and the sniff does not care that the sentence following the '@todo' is not in sentence case. If I understand correctly that is what we want? At least that is what I think jonathan1055 is referring to. I am not sure, is a new issue made in coder for that?

Setting to NR for discussion.

@quietone

As far as I understand @jonathan1055 is sure there's a variant of the sniff out there that automagically makes the sentence after the @todo start with Uppercase.

The current sniff, like you already discovered doesn't care about CaPiTaLiSaTiOn, which is not preferable.

Back to "Needs Work", see #24/#27.

So reading your links in https://git.drupalcode.org/project/drupal/-/merge_requests/504#note_116418 it seems we were enforcing a capital letter after the '@todo' but then it was reverted to any non-space character. Lets leave it as-is, and not enforce capitalisation, as there are bound to be examples where the first word is something code-like, such a a lowerCamelCase class and it would just be a pain. Therefore yes I agree with you lets move onwards.

Now that core development has moved on to 10.1.x you might like to start a new MR ad then leave the exitsing one at 9.x in case this gets backported. The first commit on the new MR, in addition to adding the sniff in phpcs.xml.dist, should have two changes in drupalci.yml

(a) to make all files get checked, set sniff-all-files: true

   assessment:
    validate_codebase:
      phpcs:
        sniff-all-files: true

and (b) to save resources and get fast test results by not running any phpunit tests

  assessment:
    testing:

remove everything below the 'testing' keyword

Create new MR 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.

Can the changes to the drupalci file be reverted.

Revert changes to the drupalci.yml file.

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

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

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.

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.

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.

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 real todo in the middle of a string. Maybe raise that for discussion in the coding standards meeting?

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.

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

I used grep to search for "todo:" and exclude some filetypes:

grep -rni "todo:" --exclude="*.map" --exclude="*.js" --exclude="*.yml" --exclude="*.twig"

I was not able to find anything.

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

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

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.

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?

Let's 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.

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.

Opened #3444003: The text of an @todo should start with a capitalized word and sniff non-php files in coding standards
#3444004: Decide what to do with the 'todo' statements in view fields in views

One of the updated @todos was resolved in #3442766: Remove deprecated code from bootstrap and lib/Controller, lib/Config, 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).

Having circular rebase issues.

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.

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

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.

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?

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 locked in and committed. Doing that now will not make it any harder to fix the finer points when those later issues land.

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. (Edit: Other places as well.)
• 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.

Thanks everyone!

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.

Novice issue reserved for the Mentored Contribution during DrupalCon Portland 2024. After May 8th, 2024, this issue returns to being open to all. Thanks!

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

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.

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.

Appears out of scope changes have been reverted.

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.

Resolved merge conflicts. Needs review.

Updating credits

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

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 being fixed (only in comments).

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 which is the critical part I believe.

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.

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

Amending attribution.