Last read comment field/filter/argument uses still the node.changed instead of node_field_data.changed column

@ravi.shankar

I reviewed you patch all works fine, but you need to fix translated text in views:

            previous: ‹‹
            next: ››

Otherwise RTBC I think

@darvanen @ravi.shankar Thanks a lot!

Removed translated text per comment in #108

This will have to be committed to 11.x first.

dawehner already has a long beard ... 10 years ...

Rerolled patch #111 for 11.x branch, please review it.

Thanks!

The last submitted patch, 115: 2086125-115.patch, failed testing. View results →

Can we pay someone to finally commit the thing?

You could RTBC it ...

capysara → changed the visibility of the branch 11.x to hidden.

capysara → changed the visibility of the branch 2086125-last-read-comment to hidden.

I moved this into a MR instead of a patch. I'm attaching an interdiff, but I only changed the $field_alias property to allow it to be nullable to address the failed test.

#115 introduced a schema.orig file I'm not familiar with. Is that correct? Otherwise I'd RTBC it.

Ooops. That was unintentional. Thanks for catching!

I updated the MR to remove it.

Few nitpicky stuff.

Updated per MR comments. Thanks Stephen!

Hiding all patches for clarity.

Ran the test-only feature

1) Drupal\Tests\comment\Functional\Views\CommentStatisticsTest::testEntityMulChanged
Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for views.view.test_ces_entity_test_mul_changed with the following errors: views.view.test_ces_entity_test_mul_changed:display.default.display_options.fields.last_updated.date_format missing schema, views.view.test_ces_entity_test_mul_changed:display.default.display_options.fields.last_updated.custom_date_format missing schema, views.view.test_ces_entity_test_mul_changed:display.default.display_options.fields.last_updated.timezone missing schema, 0 [display.default.display_options.fields.last_updated.date_format] 'date_format' is not a supported key., 1 [display.default.display_options.fields.last_updated.custom_date_format] 'custom_date_format' is not a supported key., 2 [display.default.display_options.fields.last_updated.timezone] 'timezone' is not a supported key.
/builds/issue/drupal-2086125/core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php:98
/builds/issue/drupal-2086125/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php:111
/builds/issue/drupal-2086125/core/lib/Drupal/Core/Config/Config.php:230
/builds/issue/drupal-2086125/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php:278
/builds/issue/drupal-2086125/core/lib/Drupal/Core/Entity/EntityStorageBase.php:486
/builds/issue/drupal-2086125/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php:257
/builds/issue/drupal-2086125/core/lib/Drupal/Core/Entity/EntityBase.php:352
/builds/issue/drupal-2086125/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php:609
/builds/issue/drupal-2086125/core/modules/views/src/Tests/ViewTestData.php:49
/builds/issue/drupal-2086125/core/modules/comment/tests/src/Functional/Views/CommentStatisticsTest.php:42
/builds/issue/drupal-2086125/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
ERRORS!
Tests: 1, Assertions: 2, Errors: 1.

Applying locally on a standard install and doesn't appear to have issues.
Created a test view with the Comment Statistics: Updated/commented date field added, no issues
Created some test Articles adding comments as I went.
Date seems correct to me.

Issue credits

Left some comments on the MR

Some regarding new coding standards, but also some about making sure the field-type is what we expect and some missing test coverage.

I'm keeping a close eye on this one as I'm keen to see it resolved too.

For anyone following, this is still NW for the test coverage.

I'm still working on the test, but that will take me a bit longer. If someone else can create the test quicker, feel free to grab it. Or if anyone has thoughts or pointers, you can also ping me in Slack.

dwb17 → changed the visibility of the branch 2086125-last-read-comment-11x to hidden.

loze → changed the visibility of the branch 2086125-last-read-comment-11x to active.

beunerd → changed the visibility of the branch 2086125-last-read-comment to active.

beunerd → closed merge request !8561

I'm having trouble parsing the status of this issue and getting a working patch out of it. Unknown column 'node.changed' on filter by 'Updated/commented date' 🐛 Unknown column 'node.changed' on filter by "Updated/commented date" Closed: duplicate has been closed as a duplicate in favor of this issue, but it did have a patch that ( with a little tweak as I commented 🐛 Unknown column 'node.changed' on filter by "Updated/commented date" Closed: duplicate ) seems to get my 10.3.7 site back to working without errors. Sharing here in case it helps others.

Hello, not sure if i should comment here or create a new issue. None of the patches work for me, (i'm using the sort StatisticsLastUpdated handler).
blame is on the field_alias parameter, which is non nullable, and $this->query->addOrderBy(.....) is being called.
That function does return null, so the non nullable parameter raises an exception.

For me the solution was as easy as just call the orderBy without assigning it to a field_alias parameter, which i think is unnecesary in a sort filter.

@pandepoulus yep assigning the return $this->query->orderBy() to a property is pointless. I've updated the MR to fix this for 11.x.

We still need to add test coverage for the comments on entity types that do not implement EntityChangedInterface

So I think we only need the StatisticsLastUpdated field, filter and sort to entity types which implement EntityChangedInterface and have a changed field. That way we avoid quite a few issues. If you want to filter on the last_updated time in comment_entity_statistics then you can use the last comment timestamp field, filter and sort... you do not need this special implementation.