The comment date argument handlers for created all query the comment changed field. There are no argument handlers for the comment created field at all.

This is a problem in Drupal 8.2.x, 8.1.x, 8.0.x and Views 7.x.

CommentFileSizeAuthor
#13 2689655-comment-date-arguments-13.patch4.63 KBcafuego
#2 2689655-comment-date-arguments-2.patch4.18 KBcafuego

Comments

cafuego created an issue. See original summary.

cafuego’s picture

cafuego commented
StatusFileSize
new 2689655-comment-date-arguments-2.patch4.18 KB

Attached patch re-labels the existing handlers to match their database field and adds additional handlers for the created field.

I suspect that's the best way to fix it, as changing the field on the mislabeled handlers would change the data that existing views return, which we probably want to avoid.

cafuego’s picture

cafuego commented
Status: Active » Needs review
kattekrab’s picture

kattekrab commented
Title: Comment date views arguments uses incorrect database column » D8 Comment date views arguments uses incorrect database column
rteijeiro’s picture

rteijeiro commented 
+++ b/core/modules/comment/src/CommentViewsData.php
@@ -40,11 +40,65 @@ public function getViewsData() {
+    $data['comment_field_data']['created_fulldata'] = array(
+      'title' => $this->t('Created date'),
+      'help' => $this->t('Date in the form of CCYYMMDD.'),
+      'argument' => array(
+        'field' => 'created',
+        'id' => 'date_fulldate',
+      ),
+    );

Should we use the shortened syntax for arrays?

cafuego’s picture

cafuego commented

@rteijeiro Not in this patch, no. If you want to do that, do it in a separate issue and do the whole file.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Fix is fine but it needs tests.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs tests +Needs backport to Views 7.x-3.x, +VDC, +Needs subsystem maintainer review

Well, these plugin are supplied by views and NodeViewsData is already using these so I think this fix is ok as is. And, we already have test coverage for \Drupal\views\Plugin\views\argument\Date* plugin in ArgumentDateTest.
But, I think VDC team should sign off this.

dawehner’s picture

dawehner
Primary language German
commented
Issue tags: -Needs subsystem maintainer review

Works for me!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Is there any test coverage for the new fields? I guess not.

cafuego’s picture

cafuego commented

Btw, the patch doesn't need any work to backport it to D7, there's already a working patch on issue #2689657: D7 Comment date views arguments uses incorrect database column.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Issue tags: -Needs backport to Views 7.x-3.x, -Needs tests

We don't need tests here plugins are already tested with entity_test data. There are no tests for nodes. I don't see a point of adding those for comments. Leaving it NW because I think we need an update hook in comment module to clear the views comment data.

cafuego’s picture

cafuego commented
Status: Needs work » Needs review
StatusFileSize
new 2689655-comment-date-arguments-13.patch4.63 KB

Added an empty update hook as per request from @jibran.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs review » Reviewed & tested by the community

Thanks looks good now.

  • catch committed 187a3c5 on 8.2.x 
    Issue #2689655 by cafuego: D8 Comment date views arguments uses...

  • catch committed 8c1d635 on 8.1.x 
    Issue #2689655 by cafuego: D8 Comment date views arguments uses...
catch’s picture

catch
he/him
Primary language English
commented
Version: 8.2.x-dev » 8.1.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!

Agreed with jibran that I don't see explicit test coverage for this being useful - we'd only be testing the comment_views_data() hook implementation.

Let's open a new issue for Views 7.x-3.x for the backport so the core issue queue history gets retained.

kattekrab’s picture

kattekrab commented

Views 7.x-3.x issue already exists here:
#2689657: D7 Comment date views arguments uses incorrect database column

And what about for 8.0.x ?

dqd’s picture

dqd
Location London | N.Y.C | Paris | Hamburg | Berlin
commented

JFYI #1374090: Editing a comment still changes creation date too make sure that there are no overlapping side effects...

Status: Fixed » Closed (fixed)

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