Problem/Motivation

In #2909163: Fix 'Drupal.Commenting.InlineComment.WrongStyle' coding standard and related issues we noticed this:
core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTidDepth.php

     $operator = 'IN';# " IN (" . implode(', ', array_fill(0, sizeof($this->value), '%d')) . ")";

Proposed resolution

Follow the steps in #2909362: [meta] Commented-out code in Drupal to determine why this comment is there and whether it is safe to remove it.

Remaining tasks

  • Use git log -L.
  • This might even date to the contrib Views module so you can also check there.
  • Check to ensure that the code with this comment has test coverage.
CommentFileSizeAuthor
#3 Views-before-VDC-commit-log.png234.28 KBkarthikkumarbodu
#3 views_TaxonomyIndexTidDepth_has_a_weird_commented_IN_condition-2909373-3.patch633 byteskarthikkumarbodu
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm created an issue. See original summary.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: +Novice

A novice could do this task. Be sure to research when the commented code was added (what issue, what comment, etc.) and explain it clearly in your issue comments.

karthikkumarbodu’s picture

karthikkumarbodu CreditAttribution: karthikkumarbodu as a volunteer and at Axelerant commented
Status: Active » Needs review
FileSize
views_TaxonomyIndexTidDepth_has_a_weird_commented_IN_condition-2909373-3.patch633 bytes
Views-before-VDC-commit-log.png234.28 KB

@xjm This commented out code was added before views module became part of the core.
There was no issue number or comment number found in the log message.

commit 62e67cbe05b395d5798d4299b49ed31616ab105d
Author: Daniel Wehner <daniel.wehner@erdfisch.de>
Date:   Sun Jul 29 16:28:16 2012 -0500

    convert a lot of modules to annotations

diff --git a/lib/Views/taxonomy/Plugin/views/filter/TaxonomyIndexTidDepth.php b/lib/Views/taxonomy/Plugin/views/filter/TaxonomyIndexTidDepth.php
new file mode 100644
index 00000000..ef7f25eb
--- /dev/null
+++ b/lib/Views/taxonomy/Plugin/views/filter/TaxonomyIndexTidDepth.php

I feel that we can get rid of this commented code as it doesn't make any sense in the existing logic.
Attaching the patch to remove the commented code.

mfernea’s picture

mfernea CreditAttribution: mfernea at AmeXio commented
Status: Needs review » Reviewed & tested by the community

I agree that the comment was there before Views became part of core. This is the commit for the 8.x branch: bd65ce7887.
I also think that we can safely remove this comment.

f4o’s picture

f4o CreditAttribution: f4o commented
Issue tags: +Vienna2017

I'm reviewing it now on DC Sprints

f4o’s picture

f4o CreditAttribution: f4o commented

Checked this function and applied patch successfully. Seams perfectly fine to me.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 3: views_TaxonomyIndexTidDepth_has_a_weird_commented_IN_condition-2909373-3.patch, failed testing. View results

mfernea’s picture

mfernea CreditAttribution: mfernea at AmeXio commented
Status: Needs work » Needs review

Removing a comment fails the tests?! :) I re-submitted the patch for testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

I assume this was purely a random failure :)
The value is stored in $this->value so it will be picked up later: 

    // Now build the subqueries.
    $subquery = db_select('taxonomy_index', 'tn');
    $subquery->addField('tn', 'nid');
    $where = (new Condition('OR'))->condition('tn.tid', $this->value, $operator);
    $last = "tn";

  • catch committed 0d11cb1 on 8.5.x 
    Issue #2909373 by karthikkumarbodu, xjm, mfernea, dawehner: Views...

  • catch committed d7d3de9 on 8.4.x 
    Issue #2909373 by karthikkumarbodu, xjm, mfernea, dawehner: Views...

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 3: views_TaxonomyIndexTidDepth_has_a_weird_commented_IN_condition-2909373-3.patch, failed testing. View results

mfernea’s picture

mfernea CreditAttribution: mfernea at AmeXio commented
Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

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