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

Comments

xjm created an issue. See original summary.

xjm’s picture

xjm
she/her
Primary language English
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 commented
Status: Active » Needs review
StatusFileSize
new views_TaxonomyIndexTidDepth_has_a_weird_commented_IN_condition-2909373-3.patch633 bytes
new 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 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 commented
Issue tags: +Vienna2017

I'm reviewing it now on DC Sprints

f4o’s picture

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 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
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 commented
Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

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