Problem/Motivation

The comment module offers a combination of field, sort and filter for Views on "The most recent of last comment posted or entity updated time." Sorting and filtering on this value, however, is completely broken, because they are trying to get the changed field from the node table, which has long ago moved to the node_field_data table.

Steps to reproduce

  • Set up a content type and comment type.
  • Create at least two nodes, one with comments, one without
  • Create a view of nodes of the content type
  • Add a field "The most recent of last comment posted or entity updated time"
  • (Add a sort on "The most recent of last comment posted or entity updated time" - you won't be able to do this because the previous step will already fail)

Proposed resolution

  • Adjust the handlers to use the correct table
  • Create one or more tests to verify the solution

Remaining tasks

  • Create tests
  • Create solution for the problem
  • Review
  • Commit

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

Todo.

CommentFileSizeAuthor
#21 0001-Updated-patch-for-10.3-compatibility.patch11.63 KBzoeblair
#9 git.drupalcode.org_project_drupal_-_merge_requests_3280.patch12.64 KBzoeblair

Issue fork drupal-3335480

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

eelkeblok created an issue. See original summary.

eelkeblok’s picture

eelkeblok
he/him
Primary language Dutch
Location Netherlands 🇳🇱
commented
Title: Sorting on last updated/commented date is broken » Sorting and filtering on last updated/commented date in Views is broken
eelkeblok’s picture

eelkeblok
he/him
Primary language Dutch
Location Netherlands 🇳🇱
commented
Issue summary: View changes

eelkeblok opened merge request !3280

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

AndersTwo’s picture

AndersTwo commented

This fix did not make it into 10.1. Patching core is not a permament solution.

The bug essentially breaks any view that summarizes current site activity (similar to what forums do). That functionality is so central to the misssion of Drupal, I wonder how this could have been neglected for so long.

I suggest raising its priority.

AndersTwo’s picture

AndersTwo commented

In order for others to find this issue with Google:

Sort criteria
Comment Statistics: Updated/commented date (desc)

TypeError: Cannot assign null to property Drupal\comment\Plugin\views\sort\StatisticsLastUpdated::$field_alias of type string in Drupal\comment\Plugin\views\sort\StatisticsLastUpdated->query() (line 29 of core/modules/comment/src/Plugin/views/sort/StatisticsLastUpdated.php).
eelkeblok’s picture

eelkeblok
he/him
Primary language Dutch
Location Netherlands 🇳🇱
commented
Version: 11.x-dev » 10.1.x-dev
Status: Active » Needs review

Not sure how this got bumped to core 11.

Edit: Doh. Not sure why I missed the automated comment #5, I was looking for that sort of thing... I think I'll leave it on 10.x though, because I don't understand why a bug that basically prevents a particular functionality from doing what it is meant to should not be targeted at the current major version.

zoeblair’s picture

zoeblair commented
StatusFileSize
new git.drupalcode.org_project_drupal_-_merge_requests_3280.patch12.64 KB

Exported the diff as a patch for my D9 site, thanks @eelkeblok

eelkeblok opened merge request !4432

eelkeblok’s picture

eelkeblok
he/him
Primary language Dutch
Location Netherlands 🇳🇱
commented

Created a tests-only merge request. Not sure if the tests were already functional.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Bug Smash Initiative

Can confirm the issue.

Wonder those with the type change in the schema if any post update hook is going to be needed?

quietone’s picture

quietone commented
Issue summary: View changes
Status: Reviewed & tested by the community » Needs review
Related issues: +#2086125: Last read comment field/filter/argument uses still the node.changed instead of node_field_data.changed column

I am doing triage on the core RTBC queue.

The issue summary is clear and there are steps to reproduce. I used the steps to reproduce on Drupal 11.x, standard install, and got a failure when saving the last step, not before. I am updating the steps based on my results. The remaining steps could use an update.

I do not see a code review here. Setting back to NR.

On the other hand this is about comment and views, two systems that have a fair number of issues. Has anyone looked for duplicates? I am taking a moment now... There is #2086125: Last read comment field/filter/argument uses still the node.changed instead of node_field_data.changed column. This may be a duplicate of that. That needs investigation. I've put that in the remaining tasks as the next step here.

smustgrave’s picture

smustgrave commented

They certainly have some overlap. But appears a lot more is going on in #2086125: Last read comment field/filter/argument uses still the node.changed instead of node_field_data.changed column.

So we have a fix here should we delay this one until that one is ready? Tried testing their patch but doesn't work, causes errors in the view.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Let me know if I'm wrong. But restoring status as we got a fix on this ticket while #2086125: Last read comment field/filter/argument uses still the node.changed instead of node_field_data.changed column does not and seems to have less momentum. I believe the other issue is solving more but may not be close yet to being ready.

niklp’s picture

niklp commented

@eelkeblok I believe the reason that this (and all improvements/fixes to Drupal) are targetted at D11 is because it is deemed better to fix in a place that is future-proof, and then backport to current releases.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue summary: View changes
Status: Reviewed & tested by the community » Needs work

It looks like one of the MRs has a failing test.

It looks like MR 3280 is adding a new test view for the sake of a test, but I don't see any tests that are using that view.

Can we confirm we have test coverage here?

jaydarnell’s picture

jaydarnell
he/him
Primary language English
Location Guelph, Ontario
commented

I could not get the patch from 3280 to apply in Drupal 10.1.3. I will attempt to rewrite it today and report back. One thing to note in the meantime, this appears to be an issue with the Comment Statistics: Updated/commented date wholesale, not just for sorting and filtering. Just trying to output the value of the field in 10.1.3 also fails so I'll see if I can add coverage to that to a patch as well.

jaydarnell’s picture

jaydarnell
he/him
Primary language English
Location Guelph, Ontario
commented

False alarm. The 3280 patch does in fact apply in 10.1.3. The problem is that I already had an older patch (#47) for Drupal 8+ from this issue https://www.drupal.org/project/drupal/issues/2086125 applied but it was no longer working for Drupal 10. Replacing that patch with this one fixed the issue for me.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

zoeblair’s picture

zoeblair commented
StatusFileSize
new 0001-Updated-patch-for-10.3-compatibility.patch11.63 KB

The patch doesn't seem to be applying to 10.3.6
And I see there's a note that patch files are not recommended? I can't figure out if I can update the merge request to be to 10.3 instead of 10.1

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

Looks like the same code is changing in #2086125: Last read comment field/filter/argument uses still the node.changed instead of node_field_data.changed column — maybe this is a duplicate?

I feel the title represents something similar to an issue I am having, but I’ll create a separate issue to be safe (and not step on toes) #3540867: Views 'last updated/commented' field fails to sort entities without comment statistics.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.