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.