Problem/Motivation
There's lots of commented-out code in the core codebase, especially. Code might be commented for a lot of reasons -- missed cleanup on a patch, accidentally leaving something commented out after debugging, skipped tests for later functionality -- but it is almost always wrong and a code smell that can indicate bugs, incomplete APIs, etc.
Any commented-out code purposefully left for later development should be accompanied by an @todo linking to the issue that will address the point in the future.
Proposed resolution
- Find commented-out code in the Drupal core codebase.
- In each case, use git log -L for the commented lines to figure out when and why the code was commented in the first place, and how the lines around it have changed.
- Do research into the issues that you find. Read them carefully and check comments and interdiffs for how and why the code got commented out.
- Based on this, make a recommendation as to what to do. Possibilities:
- The commented code should be accompanied with an
@todo
and followup issue (find or file it). - The commented code is leftover cruft that can be removed. (If this is the case, read the code very carefully to see what the purpose of the code was and that it's no longer needed! Check for test coverage of the code.
- For test code, commented test methods should possibly be marked skipped instead.
- The commented code should be accompanied with an
- Check with the maintainers for the subsystem to make sure your changes are correct.
Remaining tasks
Open issues
- #2909373: Views TaxonomyIndexTidDepth has a weird commented IN condition
- #2911164: Undo accidental commenting of message in MetadataGeneratorTest
- #2911165: LoggingTest.php - Weird comments
- #2911166: Undo accidental commenting of message in EntityDefinitionUpdateTest
Where to find other issues
Some examples from #2715485: Fix 'Drupal.Commenting.InlineComment.NoSpaceBefore' coding standard and the related issues:
+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
@@ -111,7 +111,7 @@ public static function open(array &$connection_options = []) {
//elseif (phpversion('pdo_pgsql') < 'version_this_was_fixed_in') {
+++ b/core/lib/Drupal/Core/Database/StatementInterface.php
@@ -37,7 +37,7 @@
//protected function __construct(Connection $dbh);
+++ b/core/modules/field/tests/src/Functional/EntityReference/EntityReferenceAutoCreateTest.php
@@ -213,17 +213,17 @@ public function testMultipleTargetBundles() {
//unset($handler_settings['auto_create_bundle']);
//$field_config->setSetting('handler_settings', $handler_settings);
//$field_config->save();
//
//$this->drupalGet('node/add/' . $this->referencingType);
//$error_message = sprintf(
// "Create referenced entities if they don't already exist option is enabled but a specific destination bundle is not set. You should re-visit and fix the settings of the '%s' (%s) field.",
// $field_config->getLabel(),
// $field_config->getName()
//);
//$this->assertErrorLogged($error_message);
+++ b/core/modules/rdf/tests/src/Functional/EntityReferenceFieldAttributesTest.php
@@ -125,23 +125,23 @@ public function testNodeTeaser() {
+ // $this->assertTrue($graph->hasProperty($taxonomy_term_1_uri, 'http://www.w3.org/1999/02/22-rdf-syntax-ns#type', $expected_value), 'Taxonomy term type found in RDF output (skos:Concept).');
...
//$this->assertTrue($graph->hasProperty($taxonomy_term_1_uri, 'http://www.w3.org/2000/01/rdf-schema#label', $expected_value), 'Taxonomy term name found in RDF output (rdfs:label).');
...
//$this->assertTrue($graph->hasProperty($taxonomy_term_2_uri, 'http://www.w3.org/1999/02/22-rdf-syntax-ns#type', $expected_value), 'Taxonomy term type found in RDF output (skos:Concept).');
...
//$this->assertTrue($graph->hasProperty($taxonomy_term_2_uri, 'http://www.w3.org/2000/01/rdf-schema#label', $expected_value), 'Taxonomy term name found in RDF output (rdfs:label).');
+++ b/core/modules/rdf/tests/src/Functional/StandardProfileTest.php
@@ -407,7 +407,7 @@ protected function assertRdfaArticleProperties($graph, $message_prefix) {
//$this->assertEqual($graph->type($this->termUri), 'schema:Thing', 'Tag type was found (schema:Thing).');
+++ b/core/modules/rest/tests/src/Functional/EntityResource/Comment/CommentResourceTestBase.php
@@ -279,7 +279,7 @@ public function testPostDxWithoutCriticalBaseFields() {
//$this->assertResourceErrorResponse(422, "Unprocessable Entity: validation failed.\nentity_type: This value should not be null.\n", $response);
@@ -288,14 +288,14 @@ public function testPostDxWithoutCriticalBaseFields() {
//$this->assertSame(500, $response->getStatusCode());
...
//$this->assertSame("Error: Call to a member function get() on null\nDrupal\\comment\\Plugin\\Validation\\Constraint\\CommentNameConstraintValidator->getAnonymousContactDetailsSetting()() (Line: 96)\n", $e->getMessage());
}
//$response = $this->request('POST', $url, $request_options);
//$this->assertResourceErrorResponse(422, "Unprocessable Entity: validation failed.\nentity_type: This value should not be null.\n", $response);
@@ -303,7 +303,7 @@ public function testPostDxWithoutCriticalBaseFields() {
//$this->assertResourceErrorResponse(422, "Unprocessable Entity: validation failed.\nfield_name: This value should not be null.\n", $response);
+++ b/core/modules/update/update.module
@@ -349,7 +349,7 @@ function update_get_available($refresh = FALSE) {
//update_create_fetch_task($project);
+++ b/core/modules/user/src/Tests/Views/AccessRoleTest.php
@@ -128,8 +128,8 @@ public function testRenderCaching() {
//$this->assertTrue(in_array('user.roles', $build['#cache']['contexts']));
//$this->assertEqual([], $build['#cache']['tags']);
+++ b/core/modules/views/tests/src/Functional/Plugin/ArgumentDefaultTest.php
@@ -131,11 +131,7 @@ public function testArgumentDefaultFixed() {
//function testArgumentDefaultPhp() {}
-
/**
* Test node default argument.
*/
+++ b/core/tests/Drupal/KernelTests/Core/Database/LoggingTest.php
@@ -64,7 +64,7 @@ public function testEnableTargetLogging() {
db_query('SELECT age FROM {test} WHERE name = :name', [':name' => 'Ringo'], ['target' => 'replica']);//->fetchCol();
+++ b/core/tests/Drupal/KernelTests/Core/Entity/Element/EntityAutocompleteElementFormTest.php
@@ -353,7 +353,7 @@ public function testEntityAutocompleteAccess() {
//$form = $form_builder->getForm($this);
+++ b/core/tests/Drupal/Tests/Core/Entity/EntityTypeBundleInfoTest.php
@@ -100,7 +100,7 @@ protected function setUp() {
//$container->get('typed_data_manager')->willReturn($this->typedDataManager->reveal());
Completed issues
None yet.
Comments
Comment #2
xjmMaking the summary a little more legible by removing diff noise from the other issue.
Comment #3
xjmRemoving the Diff examples because we should leave those alone, and added the taxonomy issue I filed.
Comment #4
mfernea CreditAttribution: mfernea at AmeXio commentedI added 3 new child issues and also mentioned them in the "Open issues" section.
Comment #11
alexpottOpen #3177919: Uncomment code in EntityReferenceAutoCreateTest to address one of the remaining things.