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

Simple patch which replaces the node table with node_field_data table to fix the last updated/commented field. The handler for the filter and sort seems to be broken so I couldn't test, will check if there is a separate issue for that.

Let me know if this covers this issue, maybe there are other things I've missed.

re-roll, seems needs tests

New patch:
- "ces" plugns now detects data table of the entity
- added demo view against User entity, to properly check that plugins work with other entities
- filed issue #2380071: No way to add comment field to any entity - you need to create a new comment type to add comment field to other entity

When commented entity has "changed" field then filter should use addWhereExpression() no idea how proceed with that.

I'm using $this->options['relationship'] to get entity type, is that right?

Added test and changed view to use node
PS: test is broken somehow

patch with debug, and fix from #2322457: Error in Views Filter Nodes based on Current user that have term reference same in nodes.

Plain reroll

A couple of small fixes, not sure what's the change proposed in #9. Shall we do something like this in CommentViewsData:

foreach ($entities_types as $type => $entity_type) {
  if ($entity_type->isSubclassOf('\Drupal\Core\Entity\EntityChangedInterface')) {
    $data['comment_entity_statistics'][$entity_type]['last_updated'] =...
  }
}

And get rid of StatisticsLastUpdated?

sure, makes sense and simplifies a code.

But I stuck with expression that inherited from date plugin #4 (see debug #7)

When commented entity has "changed" field then filter should use addWhereExpression() no idea how proceed with that.

hello,
any update on that issue?
i have Drupal 8.2.2 and I created a view that simply list the Article content type, ordered by "Updated/commented date".
But unfortunately, i'm unable i got the following error :

Column not found: 1054 Unknown column 'node.changed' in 'field list':

the code that creates this error is probably in the file core/modules/comment/src/Plugin/views/sort/StatisticsLastUpdated.php

the SQL query created includes something like "GREATEST(node.changed, comment_entity_statistics.last_comment_timestamp) AS comment_entity_statistics_last_updated"
instead of "GREATEST(node_field_data.changed, comment_entity_statistics.last_comment_timestamp) AS comment_entity_statistics_last_updated"
which would be correct.

any help/explanation appreciated,

best

The field handler was fixed in #2429447: Use data table as views base table, if available., though with a different approach. I attached an updated patch that applies against the 8.4.x-dev branch.

Run tests

The handler's entity type is not in $this->options['relationship'] anymore. This patch - instead - uses $this->getEntityType() to find out which entity type it relates to. Additionally, the patch replaces the deprecated EntityManager with EntityTypeManager.

I am using Drupal 8.2.4
Is there a way to solve this issue temporary?
thanks

@just_like_good_vibes #21 has its own issue #2841379: Unknown column 'node.changed' in 'field list' when sorting by updated/commented date.

Maybe it's related.

Closed it because duplicates this issue

1. +++ b/core/modules/comment/src/Tests/Views/CommentStatisticsNodeTest.php
   @@ -0,0 +1,32 @@
   +    debug($view->result);
   

   remove

2. +++ b/core/modules/views/src/Plugin/views/query/QueryPluginBase.php
   @@ -283,6 +283,10 @@ public function getEntityTableInfo() {
        foreach ((array) $this->view->relationship as $relationship_id => $relationship) {
   +      if (!isset($relationship->definition['base'])) {
   +        debug($relationship->definition);
   +        continue;
   +      }
   

   should be removed

#2879923: Unknown column 'node.changed' on filter by "Updated/commented date" has a one line fix for the node filter part of this, should probably be closed as a duplicate of this.

Novice work to address #30

Here is an updated patch and interdiff implemented as per #30

This filter has been broken for 2 years, but seems to only need a one line fix to get it working for nodes.

How about we reopen #2879923: Unknown column 'node.changed' on filter by "Updated/commented date" , move across a test from this patch, and retitle this issue as "Refactor Comment entity statistics".

Currently this issue is stuck on firstly
#4 / #16:

When commented entity has "changed" field then filter should use addWhereExpression() no idea how proceed with that.

Can you explain more @andypost what the problem is here?

and secondly
#9 add a handler manually to the corresponding field in hook_views_data(), and then use 'real field' in the filter.

@jonathanshaw - I wholly agree! I ran into this issue and found #2879923 which seems to fix the issue, and a simple one line fix for an issue that's been around for years should make its way out, allowing more work on the re-write to occur but allowing the community to not run into the issue.

Happy to help move out the simple fix in the meantime as I'm sure there are a number of people who could use the filter.

@jonathanshaw, @Cellar Door I think that this issue should be fixed for all entity types, because any entity type can use comment field.

#34 works great for me should we reroll it for 8.6?

let's see

We should go green with testbot to review.

It still needs work to place test in phpunit, cos this nase class deprecated

I wrote a new test based on phpunit, works fine so far. But the filter handler fo the views has some issue with config schema. I was not able to save the view if this filter is present.

@webflo given the earlier comments on this issue, what work do you think is still needed?

I just tested #47 and it worked well for nodes with comments in D8.8

I have tested #47 in drupal8.9
It works well.

@shimpy: Please don't add a screenshot that the patch applies, this does not add any information to the system, if the patch wouldn't apply anymore then the testbot will let us know when it's being retested. in #48 there was a question to @webflo about what extra steps are still needed here.

I don't see any new replies from him - but I don't see what else should be needed.

About the actual patch.

1. +++ b/core/modules/comment/src/Plugin/views/field/StatisticsLastUpdated.php
   @@ -13,10 +16,67 @@
   +      // @todo Find proper column name.
   +      $entity_data_table = $this->query->ensureTable($entity_type->getDataTable(), $this->relationship);
   +      $this->field_alias = $this->query->addField(NULL, "GREATEST(" . $entity_data_table . ".changed, " . $this->tableAlias . ".last_comment_timestamp)", $this->tableAlias . '_' . $this->field);
   

   The proper column name here is something I'm not sure about. Are we sure that this is always changed?

2. +++ b/core/modules/comment/src/Plugin/views/filter/StatisticsLastUpdated.php
   @@ -13,12 +15,60 @@
   +      // @todo Find proper column name.
   +      $entity_data_table = $this->query->ensureTable($entity_type->getDataTable(), $this->relationship);
   +      $field = 'GREATEST(' . $entity_data_table . '.changed, ' . $this->tableAlias . '.last_comment_timestamp)';
   

   Same here.

3. +++ b/core/modules/comment/src/Plugin/views/filter/StatisticsLastUpdated.php
   @@ -13,12 +15,60 @@
   +    // @todo This is broken, to proceed with GREATEST() should use
   +    //    \Drupal\views\Plugin\views\query\Sql::addWhereExpression().
   

   We should fix this as well.

4. +++ b/core/modules/comment/src/Plugin/views/sort/StatisticsLastUpdated.php
   @@ -13,10 +15,58 @@
   +      // @todo Find proper column name.
   +      $entity_data_table = $this->query->ensureTable($entity_type->getDataTable(), $this->relationship);
   +      $this->field_alias = $this->query->addOrderBy(NULL, "GREATEST(" . $entity_data_table . ".changed, " . $this->tableAlias . ".last_comment_timestamp)", $this->options['order'], $this->tableAlias . '_' . $this->field);
   

   ^

5. +++ b/core/modules/comment/tests/src/Functional/Views/CommentStatisticsTest.php
   @@ -0,0 +1,71 @@
   +  /**
   +   * Modules to install.
   +   *
   +   * @var array
   +   */
   

   Can be {@inheritdoc}

I have reviewed the comments given in #55 and According to me column name 'changed' should be fine also sql function 'GREATEST()' is working perfect in this patch.

Patch created, Please review.

@vsujeetkumar please provide interdiff from latest patch #47

It will be easy to see what's changed

Updated Patch after fixed the PHPLintFailed #56

As I mentioned on the #56, I have just worked on comment given by #55, interdiff added Please reivew.

Please review.

More Test Fixes, Please reivew.

#62 works for me. Please test further so that this can finally be committed.

Nice work @vsujeetkumar!

+++ b/core/modules/comment/src/Plugin/views/sort/StatisticsLastUpdated.php
@@ -13,10 +15,56 @@
+    if ($entity_type->entityClassImplements('\Drupal\Core\Entity\EntityChangedInterface')) {
+      // @todo Find proper column name.

#55.1 is right to be concerned about this, as the @todo suggests.

The fact that an entity implements EntityChangedInterface does not guarantee it has a field called 'changed'. In fact, we can't even be sure that a field called 'changed' is a timestamp indicating when the entity was last updated, even if the entity also implements EntityChangedInterface.

I think the best solution is to require the entity to implement EntityChangedInterface, and also to check that the entity type actually has a 'changed' field. I've no idea how to do that.

#2209971: Automatically provide a changed field definition is the issue where one day EntityChangedTrait will standardise the field to be called 'changed', but even then EntityChangedInterface won't guarantee it.

Added link for todo according #67

Cleaned-up constructors but as previous patch did not fail it means we missing test coverage,
that's because \Drupal\comment\Plugin\views\field\StatisticsLastUpdated extends Date plugin which require "date_format" stotage in constructor

+++ b/core/modules/comment/src/Plugin/views/field/StatisticsLastUpdated.php
@@ -13,10 +16,65 @@
+  public function __construct(array $configuration, $plugin_id, $plugin_definition, DateFormatter $date_formatter, EntityTypeManagerInterface $entity_type_manager) {
+    parent::__construct(
+      $configuration,
+      $plugin_id,
+      $plugin_definition,
+      $date_formatter,
+      $entity_type_manager->getStorage('date_format')
+    );

it missing "date_format" storage handler

Fix CS

+++ b/core/modules/comment/src/Plugin/views/field/StatisticsLastUpdated.php
@@ -17,57 +16,33 @@
+    if ($entity_type->entityClassImplements(EntityChangedInterface::class)) {
+      // @todo Find proper column name. https://www.drupal.org/node/2209971
       $entity_data_table = $this->query->ensureTable($entity_type->getDataTable(), $this->relationship);
       $this->field_alias = $this->query->addField(NULL, "GREATEST(" . $entity_data_table . ".changed, " . $this->tableAlias . ".last_comment_timestamp)", $this->tableAlias . '_' . $this->field);

@andypost do you know a way we can check for the existence of the changed field before adding it?

@jonathanshaw \Drupal\Core\Entity\EntityLastInstalledSchemaRepositoryInterface::getLastInstalljonathanshawedDefinition() could help here, trying

Kind of it works for me

Nice, so glad you're working on this.

1. +++ b/core/modules/comment/src/Plugin/views/field/StatisticsLastUpdated.php
   @@ -2,7 +2,9 @@
     * Field handler to display the newer of last comment / node updated.
   

   Should be 'entity' not 'node'.

2. +++ b/core/modules/comment/src/Plugin/views/field/StatisticsLastUpdated.php
   @@ -13,10 +15,50 @@
   +    $entity_type = $this->entityTypeManager->getDefinition($this->getEntityType());
   +    $base_fields = $this->entityFieldManager->getBaseFieldDefinitions($this->getEntityType());
   +    if ($entity_type->entityClassImplements(EntityChangedInterface::class) && isset($base_fields['changed'])) {
   +      // @todo Lookup changed field in keys https://www.drupal.org/node/2209971
   +      $entity_data_table = $this->query->ensureTable($entity_type->getDataTable(), $this->relationship);
   +      $this->field_alias = $this->query->addField(NULL, "GREATEST(" . $entity_data_table . ".changed, " . $this->tableAlias . ".last_comment_timestamp)", $this->tableAlias . '_' . $this->field);
   +    }
   +    else {
   +      // No changed field on entity so using own table.
   +      $this->field_alias = $this->query->addField(NULL, $this->tableAlias . ".last_comment_timestamp", $this->tableAlias . '_' . $this->field);
   +    }
   

   This will appear to work like it is supposed to, but actually silently be unaware of the changed time, if the entity does not implement EntityChangedInterface. Is this OK?

3. In CommentViewsData we have

   $data['comment_entity_statistics']['last_updated'] = [
         'title' => $this->t('Updated/commented date'),
         'help' => $this->t('The most recent of last comment posted or entity updated time.'),
         'field' => [
           'id' => 'comment_ces_last_updated',
           'no group by' => TRUE,
         ],
         'sort' => [
           'id' => 'comment_ces_last_updated',
           'no group by' => TRUE,
         ],
         'filter' => [
           'id' => 'comment_ces_last_updated',
         ],
       ];

   Should we wrap this in a conditional so that this handler is only added if there is a changed field?

Thank you for working on this. I am no longer seeing any errors.

Be good to see it wrapped up.

This is a PHP error via normal site operation, so bumping priority.

#34 work as expected, but with #71 I got errors for custom entity with comment field:

InvalidArgumentException: A valid cache entry key is required. Use getAll() to get all table data. in Drupal\views\ViewsData->get() (line 140 of core/modules/views/src/ViewsData.php).
Drupal\views\Plugin\views\HandlerBase::getTableJoin() (Line: 776)
Drupal\views\Plugin\views\query\Sql->getJoinData() (Line: 610)
Drupal\views\Plugin\views\query\Sql->ensureTable() (Line: 55)
Drupal\comment\Plugin\views\field\StatisticsLastUpdated->query() (Line: 1373)
Drupal\views\ViewExecutable->_build() (Line: 1282)
Drupal\views\ViewExecutable->build() (Line: 395)
Drupal\views\Plugin\views\display\PathPluginBase->execute() (Line: 196)
Drupal\views\Plugin\views\display\Page->execute() (Line: 1630)
Drupal\views\ViewExecutable->executeDisplay() (Line: 81)
Drupal\views\Element\View::preRenderViewElement()
call_user_func_array() (Line: 101)
Drupal\Core\Render\Renderer->doTrustedCallback() (Line: 772)
Drupal\Core\Render\Renderer->doCallback() (Line: 363)
Drupal\Core\Render\Renderer->doRender() (Line: 201)
Drupal\Core\Render\Renderer->render() (Line: 241)
Drupal\Core\Render\MainContent\HtmlRenderer->Drupal\Core\Render\MainContent\{closure}() (Line: 564)
Drupal\Core\Render\Renderer->executeInRenderContext() (Line: 242)
Drupal\Core\Render\MainContent\HtmlRenderer->prepare() (Line: 132)
Drupal\Core\Render\MainContent\HtmlRenderer->renderResponse() (Line: 90)
Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray()
call_user_func() (Line: 142)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch() (Line: 163)
Symfony\Component\HttpKernel\HttpKernel->handleRaw() (Line: 80)
Symfony\Component\HttpKernel\HttpKernel->handle() (Line: 58)
Drupal\Core\StackMiddleware\Session->handle() (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle() (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass() (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle() (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() (Line: 23)
Stack\StackedHttpKernel->handle() (Line: 708)
Drupal\Core\DrupalKernel->handle() (Line: 19)

Marked related issue as duplicate, transferring credit

I applied patch #71 on latest version of Drupal and all worked fine.

Tests are failing and this hasn't been tested against 10.1.x, there are also outstanding review issues from #72, so not RTBC.

Addressed point 1 of comment #72.

Rerolled patched against #84. please review it

The test fail is simple: $modules needs to be protected in CommentStatisticsTest.

#72.3 is not fixable in any way I know.

#72.2 is I think OK as is; #72.3 would solve it nicely if that were possible.

Therefore this is RTBC once tests pass on 9.5 and 10.1.

Small change to fix the tests

Nitpicks, if others think it is fine like this, don't care too strongly. Just wanted to queue up the other databases since this is doing a little more than plain queries.

1. +++ b/core/modules/comment/src/Plugin/views/filter/StatisticsLastUpdated.php
   @@ -13,11 +15,47 @@
   +  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
   +    $instance = new static(
   +      $configuration,
   +      $plugin_id,
   +      $plugin_definition
   +    );
   +    $instance->entityTypeManager = $container->get('entity_type.manager');
   +    $instance->entityFieldManager = $container->get('entity_field.manager');
   +    return $instance;
   

   Not a fan of this pattern inside Drupal core. It's great for using outside core to avoid breaks, but don't think we should do that inside core, just use Dependency Injection properly. Also, if you do this, it should add a setter so you could actually unit test this properly (which isn't possible now)

2. +++ b/core/modules/comment/tests/modules/comment_test_views/test_views/views.view.test_ces_entity_test_mul_changed.yml
   @@ -0,0 +1,157 @@
   +uuid: 92c9bbbd-3a1f-4f5c-813c-21869412041d
   

   I think we try to avoid adding uuid's even in test config

I've tried to address point number 1 and 2 of comment #88.

+++ b/core/modules/comment/src/Plugin/views/field/StatisticsLastUpdated.php
@@ -13,10 +15,50 @@
+  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
+    $instance = new static(
+      $configuration,
+      $plugin_id,
+      $plugin_definition,
+      $container->get('date.formatter'),
+      $container->get('entity_type.manager')->getStorage('date_format')
+    );
+    $instance->entityTypeManager = $container->get('entity_type.manager');
+    $instance->entityFieldManager = $container->get('entity_field.manager');
+    return $instance;

Thanks @ravi.shankar, same goes for the other create method being modified here, I think that should use normal DI too.

The fails on PostGres and SQLite seem unrelated.

Used proper dependency injection in remaining classes as well.

Forgot to fix the failed test of patch 89 in patch #92 so added a new patch with test fix.

The test fail is caused by a missing use statement.

Fixing failed tests of patch #93.

Will there be a solution in the 9.5.x version? We urgently need this functionality. Or is there a solid workaround?

Don't think there is a solid workaround there

@klemendev - thanks!

The __construct method on the parent object (\Drupal\views\Plugin\views\field\Date) is expecting two more injected services:

      $container->get('date.formatter'),
      $container->get('entity_type.manager')->getStorage('date_format')

As a bugfix I'm pretty sure this should still be targetting 9.5.x.

Added description for $date_formatter parameter.

Ah rats, thanks Ravi :)

@ravi.shankar I reviewed you patch, 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!

Can we pay someone to finally commit the thing?

You could RTBC it ...

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.

Update note: I also the deleted the orig file that I unintentionally introduced in my previous patch.

#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.

Still needs tests, but I'm un-assigning myself for now.

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' 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) 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.

This is looking good. Due to the changes not applying in D10, it’s trickier for me to test manually right now.

I’ve postponed #3540867: Views 'last updated/commented' field fails to sort entities without comment statistics on this, it’s related but not the same issue, just commenting for awareness. It's related to the stats not being populated in D8+

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Got the tests for entity_test which doesn't implement EntityChangedInterface. I hope that gets this further. Removing the tag I added earlier.

Forgot to change to needs review after adding the tests and attempt to shore this up by moving the logic onto the plugins.

Caught up the commits, tests were passing a few weeks ago... 🤞

Any brave souls up for reviewing this so we can get this major out the door?

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.