Fix 'Drupal.Commenting.TodoComment' coding standard

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.

Can the changes to the drupalci file be reverted.

Revert changes to the drupalci.yml file.

Spokje → closed merge request !504

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 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.

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' 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: Change sniff rule for @todo to be capitalized and sniff non-php files → in coding standards
📌 Decide what to do with the 'todo' statements in view fields Active in views

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

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).

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:
📌 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

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 looked 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.

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.

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.

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

larowlan → closed merge request !5436

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.

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 → closed merge request !8289

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.

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