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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cafuego created an issue. See original summary.

cafuego’s picture

cafuego CreditAttribution: cafuego at Creative Contingencies commented
FileSize
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 CreditAttribution: cafuego at Creative Contingencies commented
Status: Active » Needs review
kattekrab’s picture

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

rteijeiro CreditAttribution: rteijeiro at Tieto 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 CreditAttribution: cafuego at Creative Contingencies 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 Sydney, Australia
CreditAttribution: jibran 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 Sydney, Australia
CreditAttribution: jibran 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
CreditAttribution: dawehner commented
Issue tags: -Needs subsystem maintainer review

Works for me!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three 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 CreditAttribution: cafuego at Creative Contingencies 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 Sydney, Australia
CreditAttribution: jibran 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 CreditAttribution: cafuego at Creative Contingencies commented
Status: Needs work » Needs review
FileSize
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 Sydney, Australia
CreditAttribution: jibran 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
CreditAttribution: catch at Third and Grove 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 CreditAttribution: kattekrab as a volunteer 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
(formerly "diqidoq")
CreditAttribution: dqd as a volunteer 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.