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

FileSize
442.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

FileSize
27.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

FileSize
691 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
FileSize
1.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
FileSize
1.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
FileSize
1.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.