In the "map" variable found in \core\modules\comment\lib\Drupal\comment\Tests\Views\DefaultViewRecentComments.php the "comment_changed" array key is unconditionally assigned twice.

offending code:

$map = array(
      'comment_nid' => 'nid',
      'comment_subject' => 'subject',
      'comment_changed' => 'changed',
      'comment_changed' => 'created',
      'cid' => 'cid'
    );
Files: 
CommentFileSizeAuthor
#14 drupal8.comment-module.2091139-14.patch1.1 KBmcrittenden
PASSED: [[SimpleTest]]: [MySQL] 59,245 pass(es).
[ View ]
#10 drupal-duplicate-array-key-2091139-10.patch1.08 KBmcrittenden
PASSED: [[SimpleTest]]: [MySQL] 59,678 pass(es).
[ View ]
#8 drupal-duplicate-array-key-2091139-8.patch1.3 KBmcrittenden
FAILED: [[SimpleTest]]: [MySQL] 59,595 pass(es), 1 fail(s), and 7 exception(s).
[ View ]
#5 drupal-duplicate-array-key-2091139-4.patch691 bytesmcrittenden
FAILED: [[SimpleTest]]: [MySQL] 59,245 pass(es), 1 fail(s), and 5 exception(s).
[ View ]
#4 Execute PHP Code.png27.02 KBsandipmkhairnar
#3 gets-the-second.png442.97 KBYesCT

Comments

YesCT’s picture

Title:Duplicate array key in DefaultViewRecentComments» Duplicate array key comment_changed in DefaultViewRecentComments
YesCT’s picture

this one looks like maybe a different key was intended.

      'comment_changed' => 'changed',
      'comment_created' => 'created',

maybe?

YesCT’s picture

StatusFileSize
new442.97 KB

if we get a meta, we can add this there, but for now. I checked and php does use the second value. checked with devel/php page.
gets-the-second.png

sandipmkhairnar’s picture

StatusFileSize
new27.02 KB

@YesCT - Yes checked with devel/php page and php does use the second value.

But in \core\modules\comment\lib\Drupal\comment\Tests\Views\DefaultViewRecentComments.php "comment_changed" array key is assigned single value.

mcrittenden’s picture

StatusFileSize
new691 bytes
FAILED: [[SimpleTest]]: [MySQL] 59,245 pass(es), 1 fail(s), and 5 exception(s).
[ View ]

For the record, here's the original commit: http://drupalcode.org/project/drupal.git/commitdiff/e14fbaa3ec3e64e2af27...

And the issue: #1758486: Write tests for the recent comments view.

Looking at the code, it looks to me like the 2nd was meant to be comment_created like YesCT said in #2 and the only reason the tests still passed was because $comment->created and $comment->changed happened to be the same value since the comment had never been edited.

Here's a patch, let's see if the tests still pass.

mcrittenden’s picture

Assigned:Unassigned» mcrittenden
Status:Active» Needs review

Status:Needs review» Needs work

The last submitted patch, drupal-duplicate-array-key-2091139-4.patch, failed testing.

mcrittenden’s picture

Status:Needs work» Needs review
StatusFileSize
new1.3 KB
FAILED: [[SimpleTest]]: [MySQL] 59,595 pass(es), 1 fail(s), and 7 exception(s).
[ View ]

Ah, looks like the view doesn't contain a comment_created field which is why the original code was written like that. So to avoid the duplicate key warning, let's swap keys/values and then swap the order of the test args to match.

Status:Needs review» Needs work

The last submitted patch, drupal-duplicate-array-key-2091139-8.patch, failed testing.

mcrittenden’s picture

Status:Needs work» Needs review
StatusFileSize
new1.08 KB
PASSED: [[SimpleTest]]: [MySQL] 59,678 pass(es).
[ View ]

I'm an idiot--assertIdenticalResultset requires the first argument to be the view.

Anyways, since there's no comment_created field in the view, there's probably no point in comparing the $comment->created field to anything, so let's just remove those altogether.

sandipmkhairnar’s picture

Thanks @mcrittenden for patch.

I have tested patch its working fine for me.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:+VDC

It is right, this view does not load the created field

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Patch no longer applies.

mcrittenden’s picture

Status:Needs work» Needs review
StatusFileSize
new1.1 KB
PASSED: [[SimpleTest]]: [MySQL] 59,245 pass(es).
[ View ]

Re-rolled.

mcrittenden’s picture

Issue tags:-Needs reroll

Tags

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

Still good to go.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Great catch!

Committed and pushed to 8.x. Thanks!

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