Remove all TODO comments in the code

[GuySartorelli]

There are a ton of TODO comments in the code. It's a bad look, and it makes it a lot harder to find temporary TODO comments that are added during development, since a simple search for TODO is polluted by the existing ones.
What's more, a TODO comment in the codebase is never going to be actioned, so they should be replaced with GitHub issues we can track.

Acceptance criteria

• Any TODO comment that should be actioned has an issue in the appropriate repository
• All TODO comments are removed from the codebase for all supported and core modules

PRs

• MNT Remove TODO comments silverstripe-versionfeed#92
• MNT Remove TODO comments silverstripe-versioned#418
• FIX Method CopyMethod::copy as deprecated vendor-plugin#70
• MNT Remove TODO comments silverstripe-userforms#1247
• MNT Remove TODO comments silverstripe-segment-field#92
• MNT Remove TODO comments silverstripe-mfa#515
• MNT Remove TODO comments silverstripe-ldap#63
• MNT Remove TODO Comments silverstripe-errorpage#92
• MNT Remove TODO comments silverstripe-contentreview#215
• MNT Remove TODO comments silverstripe-cms#2898
• MNT Remove TODO comments silverstripe-blog#730
• MNT Remove TODO comments silverstripe-behat-extension#252
• MNT Remove TODO comments silverstripe-assets#577
• MNT Remove TODO comments silverstripe-tagfield#264
• MNT Remove TODO comments silverstripe-subsites#546
• MNT Remove TODO comments silverstripe-securityreport#70
• MNT Remove TODO comments silverstripe-graphql#556
• MNT Remove TODO comments silverstripe-staticpublishqueue#176
• MNT Remove TODO comments silverstripe-elemental#1112
• MNT Remove TODO comments cwp-core#129
• MNT Remove TODO comments silverstripe-restfulserver#114
• MNT Remove TODO comments silverstripe-admin#1608
• MNT Remove TODO comments silverstripe-asset-admin#1413
• MNT Remove TODO comments silverstripe-framework#11032
• MNT Remove TODO comments silverstripe-campaign-admin#283
• MNT Remove TODO comments silverstripe-versioned-admin#312

[sabina-talipova]

I plan to create a separate epic task for these purposes. This will make it easier to group and track the process of how TODO is performed.

[sabina-talipova]

TODO tickets list:

• Test coverage: Check for the content of the original version in the request result silverstripe-versionfeed#90
• CMS 6: VersionFeed::getSiteRSSLink() should return link from home page. silverstripe-versionfeed#91
• TODO: DataDifferencerTest::testArrayValues. Investigate issue with using getter. silverstripe-versioned#413
• DataDifferencer::getObjectDisplay should use CMSThumbnail to limit max size of images in diffs silverstripe-versioned#414
• ChangeSet::removeObject() should handle case of implicit added item being removed. silverstripe-versioned#415
• TODO: Build this action into explicit UX action silverstripe-versioned#416
• We're calling a method (having) that doesn't exist on DataList silverstripe-versioned#417
• TODO: Investigate ability to turn on / off Versioned::prepopulate_versionnumber_cache in RecoverUploadLocationsHelper::chunk method silverstripe-userforms#1244
• Improve unit test coverage silverstripe-userforms#1245
• Make form actions editable, the same way that fields are editable silverstripe-userforms#1246
• TODO: Implement methods in test classes silverstripe-mfa#512
• TODO: Define the default value for Default Registered Method as fallback. silverstripe-mfa#514
• Investigate how we can support for writing back relations in LDAPService::updateLDAPFromMember silverstripe-ldap#62
• CMSExternalLinksController::start() hangs as it waits for the connection to be released silverstripe-externallinks#110
• Add ability to return a warning instead of error when checks fail silverstripe-environmentcheck#94
• Behat test cases for adding files to a banner block via inline editing silverstripe-elemental-bannerblock#127
• Remove duplicated methods (getSchemaRequested() and getSchemaResponse()) silverstripe-contentreview#214
• Add tests for canAddChildren() and canCreate() methods in SiteTreePermissionsTest silverstripe-cms#2893
• Use NestedTitle instead of Title in various reports silverstripe-cms#2894
• Change the BackLinks relation in SiteTree to a polymorphic many_many relation silverstripe-cms#2895
• Refactor CMSMain and its subclasses silverstripe-cms#2896
• TODO: Should ContentController::LoginForm() return all available log-in forms? silverstripe-cms#2897
• TODO: BlogPost::getCMSFields(). Reimplement the sidebar silverstripe-blog#729
• File::BackLinkTracking() - Implement PolymorphicManyManyList to replace ArrayList silverstripe-assets#575
• Consider utilising the Upload class for file uploads that don't involve the database silverstripe-assets#576
• Don't allow concurrent processes to try creating files with the same name silverstripe-assets#578
• TODO: Assess whether we require lazy loading limits for the StringTagField silverstripe-tagfield#263
• Check if SiteTree_ImageTracking is even still valid, and whether it should be ignored in FileSubsites::augmentSQL() silverstripe-subsites#542
• TODO: LeftAndMain_Subsites.getChangeTrackerOptions() - Investigate should we still address some edge cases with the Entwine API extension silverstripe-subsites#541
• TODO: SubsiteDomain::getSubstitutedDomain() - Refactor domains into separate wildcards / primary domains silverstripe-subsites#543
• TODO: Future enchantment for GridFieldSubsiteDetailFormItemRequest::ItemEditForm silverstripe-subsites#544
• Subsite model permission enhancements  silverstripe-subsites#545
• TODO: Update Unit tests. Pre-fill the report with fixture-defined users. silverstripe-securityreport#68
• TODO: Investigate should GridFieldComponents change behaviour based on whether others are available in the config silverstripe-securityreport#69
• RFC: Refactor plugin API silverstripe-graphql#364
• Bust query cache for last-deleted (or a combination of last deleted and last edited, i.e. last modified) for versioned records silverstripe-graphql#554
• TODO: Move $authenticators to a per-schema configuration and refer to the schema from the current endpoint silverstripe-graphql#555
• TODO: Add subsites integration silverstripe-staticpublishqueue#175
• Update the data for the current element after saving it instead of rerunning the whole query. silverstripe-elemental#1111
• Follow up on the LoginAttemptNotifications extension cwp-core#41
• TODO: CWP - Miscellaneous improvements cwp-core#127
• TODO: HTMLEditorField_Toolbar is no longer exist in CMS.  cwp-core#128
• RestfulServerTest / JSONDataFormatterTest - Miscellaneous improvements silverstripe-restfulserver#111
• Pass $api_base from the API to the data formatter silverstripe-restfulserver#112
• DataFormatter - Miscellaneous improvements silverstripe-restfulserver#113
• Miscellaneous Front end improvements silverstripe-admin#1595
• TODO: GridField Component Improvements silverstripe-admin#1596
• TODO: GridFieldTable Component refactoring silverstripe-admin#1597
• LeftAndMain.Panel.js - fix redraw order in togglePanel silverstripe-admin#1598
• Remove CMSMenu controller routing rule when the controller is removed. silverstripe-admin#1599
• Behat test cases enhancement silverstripe-admin#1600
• Remove or correctly implement commented-out unit tests silverstripe-admin#1601
• TODO: FormBuilderLoader.getMessages - define standard message type silverstripe-admin#1602
• TODO: LeftAndMain entwine enhancement  silverstripe-admin#1603
• buildApolloGraphqlContainer - investigate do we still need to implement logic of scaffold() function silverstripe-admin#1604
• Fix registration or JS singletons across modules silverstripe-admin#1605
• Enhancements to sitetree js (LeftAndMain.Tree.js) silverstripe-admin#1606
• TODO: injectGraphql.js - investigate converting context to a callback. silverstripe-admin#1607
• Update GraphQL models after form submission silverstripe-graphql#14
• Update GraphQL store with new model data silverstripe-asset-admin#1406
• TODO: Refactor "queued files" into separate visual area and remove coupling here silverstripe-asset-admin#1407
• Add date localisation in Javascript silverstripe-asset-admin#1408
• Refactor moveFilesMutation to invalidate the relevant queries in the cache silverstripe-asset-admin#1409
• AssetAdmin.js handleSubmitEditor() can't find the file silverstripe-asset-admin#1410
• Accessibility: Wrap the contents of HistoryItem Component wrapped in an <a> tag (or make them otherwise accessible to screenreaders) silverstripe-asset-admin#1411
• Use caching instead of polling in HistoryList silverstripe-asset-admin#1412
• ContentNegotiator class improvements silverstripe-framework#10980
• TODO: Controller::beforeHandleRequest improvements silverstripe-framework#10981
• CookieJar class improvements silverstripe-framework#10982
• Non-public dir support was removed in CMS 5.0.0 - remove dead code silverstripe-framework#10983
• TODO: Investigate passing $response object to RSSFeed::outputToBrowser() silverstripe-framework#10984
• Move ClassInfo::hasTable to SS_Database or DB class silverstripe-framework#10985
• Move ClassInfo::dataClassesFor into DataObjectSchema silverstripe-framework#10986
• TODO: Extensible::remove_extension - add support for removing extensions with parameters silverstripe-framework#10987
• TODO: Injector::inject improvements silverstripe-framework#10988
• TODO: A "lazy loading" DataObjectSet implementation silverstripe-framework#10989
• ViewableDataContains::toString - add representation for more than one match silverstripe-framework#10990
• Convert::xml2raw - &#xxx should be converted silverstripe-framework#10991
• CurrencyField add localization support silverstripe-framework#10992
• BulkLoader improvements silverstripe-framework#10993
• TODO: CsvBulkLoader - Add support for deleting existing records not matched in the import silverstripe-framework#10994
• TODO: CsvBulkLoader::findExistingObject - support $columnMap silverstripe-framework#10995
• Provide better feedback when using CsvBulkLoader silverstripe-framework#10996
• Debug: provide an option to _not_ call die() silverstripe-framework#10997
• Refactor Debug::require_developer_login to use the ORM instead of raw SQL silverstripe-framework#10998
• DevelopmentAdmin class tidy-up silverstripe-framework#10999
• TODO: Update CheckboxSetField documentation silverstripe-framework#11000
• Reduce static coupling to Controller::has_curr() and Controller::curr() silverstripe-framework#4484
• FieldList::findOrMakeTab - support recursive creation of TabSets silverstripe-framework#11001
• Make FieldList::fieldByName work with nested sets silverstripe-framework#11002
• TODO: TimeField - add Timezone support silverstripe-framework#11003
• Update many_many through relations to support polymorphic "to" silverstripe-framework#11004
• TODO: Replace GroupedDropdownField::schemaDataType with 'SCHEMA_DATA_TYPE_*' constant  silverstripe-framework#11005
• TODO: MoneyField::schemaDataType - replace with FormField::SCHEMA_DATA_TYPE_TEXT silverstripe-framework#11006
• Update DB::manipulate to support paramaterised queries silverstripe-framework#11007
• TODO: DBField - remove MySQL specific code from subclasses silverstripe-framework#11008
• TODO: Implement method DBPolymorphicForeignKey::scaffoldFormField silverstripe-framework#11009
• Improve test coverage in DataObjectTest silverstripe-framework#11010
• Actually test validation of RequiredFields validator silverstripe-framework#11011
• Improve test coverage in FieldListTest silverstripe-framework#11012
• TODO: Update the expectation in CurrencyFieldReadonlyTest and CurrencyFieldDisabledTest silverstripe-framework#11013
• TODO: Make PhpSyntaxTest::testShortTagsOffWillWork work on all platforms silverstripe-framework#11014
• Improve test coverage in ObjectTest silverstripe-framework#11015
• TODO: Investigate possibility to move Member::beforeMemberLoggedIn into AuthenticationMiddleware silverstripe-framework#11016
• Finish implementing WithinRangeFilter class  silverstripe-framework#11017
• Make FulltextFilter more generic, so that modules can add support for databases besides MySQL silverstripe-framework#11018
• TODO: DBPrimaryKey - Allow for custom limiting/filtering silverstripe-framework#11019
• DBDatetime - Add localization support silverstripe-framework#11020
• RFC: Improve behaviour of unsaved has_one / belongs_to relations silverstripe-framework#7818
• TODO: FormField::getSchemaDataType - Throw exception if value is missing silverstripe-framework#11021
• Merge arrays recursively for FormField::setSchemaState and FormField::setSchemaData silverstripe-framework#11022
• DBComposite - Update Unit tests for loading and saving composite values silverstripe-framework#11023
• TODO: DataObject - Add instance specific removeExtension() silverstripe-framework#11024
• TODO: DataObject::merge - remove redundant merge of has_one fields silverstripe-framework#11025
• Refactor DataObject::delete() - move the logic into DataQuery silverstripe-framework#11026
• TODO: DataObject::setFieldValue improvememnts silverstripe-framework#11027
• TODO: DBSchemaManager::requireTable - Change detection for CREATE TABLE $options other than "ENGINE" silverstripe-framework#11028
• Remove DBSchemaManager::alterIndex silverstripe-framework#11029
• TODO: Investigate ability to refactor GridField::getCastedValue into GridFieldComponent silverstripe-framework#11030
• TODO: Improvements for sake tool silverstripe-framework#11031
• TODO: FixtureContext::stepCreateRecordWithTable - Support more than one record silverstripe-behat-extension#253
• TODO: FixtureContext::stepThereAreTheFollowingRecords - Run prepareAsset() for each File and Folder record silverstripe-behat-extension#254
• TODO: CampaignAdmin::getClientConfig - use schemaUrl instead silverstripe-campaign-admin#281
• CampaignAdmin class refactoring opportunities silverstripe-campaign-admin#282
• Replace previewState with import VIEW_MODE_STATES silverstripe-versioned-admin#311

[michalkleiner]

I've checked a few of the tickets created from the todo comments but for me in most cases it lacks any context where it comes from, so it could be good to add e.g. a link to the line in file it comes from? Something so that whoever looks at the GH issue doesn't need to spend too much time trying to find those details.

[GuySartorelli]

e.g. a link to the line in file it comes from?

^ Note that a link to that line of code should use the "permanent" link, so that when your PRs to remove the todo comments are merged, we can still see the context that is required for the issue.

[GuySartorelli]

I have added comments requesting clarification on the following new issues:

• TODO: DataDifferencerTest::testArrayValues. Investigate issue with using getter. silverstripe-versioned#413
• TODO: Define the default value for Default Registered Method as fallback. silverstripe-mfa#514
• Remove or correctly implement commented-out unit tests silverstripe-admin#1601
• LeftAndMain.Panel.js - fix redraw order in togglePanel silverstripe-admin#1598
• TODO: GridField Component Improvements silverstripe-admin#1596
• TODO: Refactor "queued files" into separate visual area and remove coupling here silverstripe-asset-admin#1407
• TODO: CampaignAdmin::getClientConfig - use schemaUrl instead silverstripe-campaign-admin#281
• Provide better feedback when using CsvBulkLoader silverstripe-framework#10996
• Debug: provide an option to _not_ call die() silverstripe-framework#10997
• Refactor Debug::require_developer_login to use the ORM instead of raw SQL silverstripe-framework#10998
• TODO: DBSchemaManager::requireTable - Change detection for CREATE TABLE $options other than "ENGINE" silverstripe-framework#11028
• TODO: DataObject::setFieldValue improvememnts silverstripe-framework#11027
• TODO: SubsiteDomain::getSubstitutedDomain() - Refactor domains into separate wildcards / primary domains silverstripe-subsites#543

[sabina-talipova]

@GuySartorelli , I updated the following issues and PRs:

• MNT Remove TODO comments silverstripe-blog#730
• MNT Remove TODO comments silverstripe-subsites#546
• MNT Remove TODO comments silverstripe-asset-admin#1413
• MNT Remove TODO comments silverstripe-framework#11032
• MNT Remove TODO comments silverstripe-admin#1608
• MNT Remove TODO comments cwp-core#129
• MNT Remove TODO comments silverstripe-subsites#546
• MNT Remove TODO comments silverstripe-graphql#556
• MNT Remove TODO comments silverstripe-campaign-admin#283
• TODO: SubsiteDomain::getSubstitutedDomain() - Refactor domains into separate wildcards / primary domains silverstripe-subsites#543
• Remove or correctly implement commented-out unit tests silverstripe-admin#1601
• TODO: DBSchemaManager::requireTable - Change detection for CREATE TABLE $options other than "ENGINE" silverstripe-framework#11028
• Refactor Debug::require_developer_login to use the ORM instead of raw SQL silverstripe-framework#10998
• Debug: provide an option to _not_ call die() silverstripe-framework#10997
• Provide better feedback when using CsvBulkLoader silverstripe-framework#10996
• TODO: CsvBulkLoader::findExistingObject - support $columnMap silverstripe-framework#10995
• TODO: Refactor "queued files" into separate visual area and remove coupling here silverstripe-asset-admin#1407
• TODO: GridField Component Improvements silverstripe-admin#1596
• LeftAndMain.Panel.js - fix redraw order in togglePanel silverstripe-admin#1598

[sabina-talipova]

And one new ticket for CMS6, probably.

• The $columnMap argument is no longer required in CsvBulkLoader::findExistingObject silverstripe-framework#11036

[sabina-talipova]

New ticket:

• Using Laminas\Ldap\Attribute::setPassword instead of LDAPService::setPassword silverstripe-ldap#64

[GuySartorelli]

PRs merged and new issues triaged