[META] Modernize the History module

+++ b/core/modules/history/lib/Drupal/history/HistoryManager.php
@@ -0,0 +1,128 @@
+    if (!isset($account)) {
+      $account = \Drupal::currentUser();
+    }
+

Any reason to not inject that service?

+++ b/core/modules/forum/lib/Drupal/forum/ForumManager.php
@@ -110,6 +111,13 @@ class ForumManager implements ForumManagerInterface {
+   * Translation manager service.
+   *
+   * @var \Drupal\history\HistoryManagerInterface
+   */
+  protected $historyManager;
+
+  /**

@@ -122,13 +130,16 @@ class ForumManager implements ForumManagerInterface {
+  public function __construct(ConfigFactory $config_factory, EntityManager $entity_manager, Connection $connection, FieldInfo $field_info, TranslationInterface $translation_manager, HistoryManagerInterface $history_manager) {

@@ -326,18 +337,16 @@ protected function numberNew($nid, $timestamp) {
+    $this->history[$nid] = $this->historyManager->read('node', array($nid));

If we can't be sure that the history manager is available we should check that the variable exists before we call it?

What about marking the HistoryManagerInterface on the constructor = NULL?

Test results for posterity:

#10: drupal8.history-module.2081585-10.patch queued for re-testing.

1. +++ b/core/modules/history/history.install
   @@ -13,13 +13,20 @@ function history_schema() {
   +        'description' => 'The {users}.uid that read the entity_id.',
   

   entity_id => entity

2. +++ b/core/modules/history/history.install
   @@ -13,13 +13,20 @@ function history_schema() {
   +        'description' => 'The entity_type of the entity was read.',
   
   +++ b/core/modules/node/node.install
   @@ -670,9 +670,37 @@ function node_update_8011() {
   +    'description' => 'The entity_type of the entity was read.',
   

   that was read

3. +++ b/core/modules/history/history.install
   @@ -13,13 +13,20 @@ function history_schema() {
   +        'description' => 'The entity_id that was read.',
   
   +++ b/core/modules/node/node.install
   @@ -670,9 +670,37 @@ function node_update_8011() {
   +    'description' => 'The entity_id that was read.',
   

   The ID of the entity that was read.

4. +++ b/core/modules/history/history.module
   @@ -56,85 +58,40 @@ function history_read($nid) {
   +  Drupal::service('history.manager')->write('node', $nid, $account);
   ...
   +  Drupal::service('history.manager')->purge();
   ...
   +  Drupal::service('history.manager')->deleteByEntity('node', $node->id());
   
   @@ -143,9 +100,7 @@ function history_node_delete(EntityInterface $node) {
   +      Drupal::service('history.manager')->deleteByUid($account->id());
   
   @@ -154,9 +109,7 @@ function history_user_cancel($edit, $account, $method) {
   +  Drupal::service('history.manager')->deleteByUid($account->id());
   

   \Drupal

5. +++ b/core/modules/history/lib/Drupal/history/HistoryManager.php
   @@ -0,0 +1,143 @@
   +   * Array of history keyed by entity type and entity id.
   

   Given uid is an optional parameter, shouldn't that be in the mix here or we'll need a resetCache method.

6. +++ b/core/modules/history/lib/Drupal/history/HistoryManagerInterface.php
   @@ -0,0 +1,68 @@
   +   *   The user account id to purge history
   

   ID

Attached patch fixes these and tests.

#17: history-manager-2081585.17.patch queued for re-testing.

+++ b/core/modules/forum/lib/Drupal/forum/ForumManager.php
@@ -326,18 +337,19 @@ protected function numberNew($nid, $timestamp) {
+      $this->history[$nid] = $this->historyManager->read('node', array($nid));

According to HistoryManager::read() this returns history information by entity ID, though on the left side this sets the history information to a specific value.

Generally looking pretty good!

1. +++ b/core/modules/forum/lib/Drupal/forum/ForumManager.php
   @@ -110,6 +111,13 @@ class ForumManager implements ForumManagerInterface {
   +   * History manager service.
   

   Nitpick alert: 'The history manager service' ?

2. +++ b/core/modules/forum/lib/Drupal/forum/ForumManager.php
   @@ -326,18 +337,19 @@ protected function numberNew($nid, $timestamp) {
   +      $this->history[$nid] = HISTORY_READ_LIMIT;
   

   Can this constant get converted to a class constant?

3. +++ b/core/modules/history/lib/Drupal/history/Controller/HistoryController.php
   @@ -54,9 +83,10 @@ public function readNode(Request $request, NodeInterface $node) {
   +    return new JsonResponse((int) $history[$node->id()]);
   

   What's going down here? why the casting? If it's legit, maybe a comment..

4. +++ b/core/modules/history/lib/Drupal/history/HistoryManager.php
   @@ -0,0 +1,143 @@
   +      ->condition('entity_id', array_keys($entities_to_read), 'IN')
   +      ->execute();
   ...
   +    foreach ($result as $row) {
   +      $entities_to_read[$row->entity_id] = (int) $row->timestamp;
   +    }
   

   We could return an assoc array of the results so they are in the form entity_id => timestamp already. But I guess you want this casting.

5. +++ b/core/modules/history/lib/Drupal/history/HistoryManager.php
   @@ -0,0 +1,143 @@
   +      $account = \Drupal::currentUser();
   

   Could just inject the current user service instead.

6. +++ b/core/modules/history/lib/Drupal/history/HistoryManager.php
   @@ -0,0 +1,143 @@
   +    $this->history = array();
   

   This should just call resetCache() instead.

7. +++ b/core/modules/history/lib/Drupal/history/HistoryManager.php
   @@ -0,0 +1,143 @@
   +    $this->history = array();
   

   And here.

8. +++ b/core/modules/history/lib/Drupal/history/HistoryManager.php
   @@ -0,0 +1,143 @@
   +    // Clean static cache.
   +    unset($this->history[$entity_type][$entity_id]);
   

   We could let resetCache() take an optional array of entity ids to clear? Just a thought.

9. +++ b/core/modules/history/lib/Drupal/history/HistoryManagerInterface.php
   @@ -0,0 +1,73 @@
   +   * Reset the static cache.
   

   Resets

Also, do you think the new HistoryManager class needs it's own unit test?

+++ b/core/modules/history/lib/Drupal/history/HistoryManager.php
@@ -0,0 +1,143 @@
+      $account = \Drupal::currentUser();

I'm not sure how just changing this for the injected current_user service will stop using $account etc..? You will still have the same logic as now.

1. +++ b/core/modules/history/lib/Drupal/history/HistoryManagerInterface.php
   @@ -0,0 +1,73 @@
   +/**
   + * Provides history manager interface.
   + */
   +interface HistoryManagerInterface {
   

   The name "HistoryManager" wasn't very intuitive and self-descriptive until I looked at what it actually does.

   I do not have a concrete improvement suggestion, but I wonder whether we're able to find a more intuitive name?

   The goal for a better name would be to at least hint a little bit at what exactly this service is doing (retrieving and updating last viewed timestamps for entities).

   I know that we struggled a lot with finding a proper name for History module in the past already (and IIRC, we still have a dedicated issue open for that even), but perhaps:

   \Drupal\history\EntityViewManager

   ?

2. +++ b/core/modules/history/lib/Drupal/history/HistoryManagerInterface.php
   @@ -0,0 +1,73 @@
   +   * Retrieves the timestamp for the current user's last view of the entities.
   ...
   +  public function read($entity_type, $entity_ids);
   ...
   +   * Updates 'last viewed' timestamp of the entity for the current user.
   ...
   +  public function write($entity_type, $entity_id, AccountInterface $account = NULL);
   

   Hm. The "read" and "write" terminology originates from the procedural implementation, in which it was rather unusual to use more descriptive function names.

   In an object-oriented implementation, developers expect more self-explanatory method names.

   Could we rename these methods along the lines of the following?

   getLastViewedTimestamps()
   updateLastViewedTimestamps()

   For brevity, I guess it would be fine to exclude the "Timestamp" part and just go with getLastViewed() and updateLastViewed().

   Such method names would be much more clear compared to read() and write(), wouldn't they?

   Lastly, while I understand the use-case-driven difference of getting multiple last viewed timestamps vs. updating a single last viewed timestamp, I do not understand why only the update method allows to pass a custom user account — if there's a use-case for updating the history for a user account that is not the current user, then there must also be a use-case for getting the history for a user that is not the current user, no?

   Finally, as this is all about entity system entities (as opposed to arbitrary "entity type" strings), would it make sense to replace the $entity_type + $entity_id arguments in the update method with a single EntityInterface $entity argument?

   updateLastViewed(EntityInterface $entity, AccountInterface $account = NULL)

   I consider these aspects to be the most important part of this patch, as we're defining an interface (API) for a service that may be replaced with a different implementation.

3. +++ b/core/modules/history/lib/Drupal/history/HistoryManagerInterface.php
   @@ -0,0 +1,73 @@
   +  public function deleteByUid($uid);
   

   I don't know whether it is technically feasible (primarily concerning performance), but would it be possible to replace the $uid argument with AccountInterface $account?

   Otherwise, we would be hard-coding an assumption that $account->uid is the ID of user accounts in this API and all consuming code — whereas AccountInterface properly defines what exactly a user is in Drupal, regardless of the actual implementation.

4. +++ b/core/modules/history/lib/Drupal/history/HistoryManagerInterface.php
   @@ -0,0 +1,73 @@
   +  public function deleteByEntity($entity_type, $entity_id);
   

   As above, is it possible to replace the two arguments with EntityInterface $entity?

   Otherwise, deleteByEntity() claims to talk about entities, but an actual entity is not part of the API contract?

5. +++ b/core/modules/node/node.install
   @@ -720,9 +720,37 @@ function node_update_8011() {
   +  db_add_field('history', 'entity_type', array(
   ...
   +    'default' => 'node',
   

   Unless I'm terribly mistaken, I think you meant 'initial' instead of 'default', no?

   'default' specifies the (permanent) default value for the column, whereas 'initial' specifies the initial value of all existing rows for a newly created column.

6. +++ b/core/modules/node/node.install
   @@ -720,9 +720,37 @@ function node_update_8011() {
   +    'length' => 255,
   

   In D8, didn't we globally limit the maximum length of entity_type to ~64 characters already?

7. +++ b/core/modules/node/node.install
   @@ -720,9 +720,37 @@ function node_update_8011() {
   +  // Create new indexes.
   +  db_add_primary_key('history', array(
   +    'uid',
   +    'entity_id',
   +    array('entity_type', 32),
   +  ));
   +  db_add_index('history', 'history_entity', array(
   +    'entity_id',
   +    array('entity_type', 32),
   +  ));
   

   These tables can quickly grow very large.

   For (MySQL) query statement performance optimization, columns that share and have the most repetitive values should always be specified first in the primary key and all other indexes.

   For the history tables, I guess it would be reasonable to expect that values will increasingly vary in this order:

   1) entity_type
   2) uid
   3) entity_id

8. +++ b/core/modules/tracker/lib/Drupal/tracker/Tests/TrackerTest.php
   @@ -154,6 +154,7 @@ function testTrackerNewNodes() {
        $this->drupalLogin($this->other_user);
   +    \Drupal::service('history.manager')->resetCache();
   

   Why is the history cache manually reset here?

   Since the test uses the internal browser to request pages and logs in another user between assertions, the history functionality should work as-is.

   The manual cache reset in the test runner appears to hide a potentially existing bug?

Wow, that was quick! :)

4) I do not really see a reason for why history/last-viewed tracking should be limited to content entities? Is there any? Oh, the integer entity ID vs. string entity ID, I guess? Bummer.

That said, is that storage related detail something we want to hard-code in the API/Interface? What if I'm replacing the entire entity history storage with a key/value store, which in turn would enable me to track views of e.g. config entities, too? :)

As a consequence, not sure whether PHP allows for that, but could the interface take a EntityInterface, but the default (SQL) implementation of History module would take a ContentEntityInterface?

5) Indeed, the 'initial' key is poorly documented, but it does exist:
https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Database!Schema.p...
https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Database!Driver!m...

In short, you can simply specify this:

array(
  'default' => '',
  'initial' => 'node',
)

Great! :)

1. +++ b/core/modules/comment/comment.module
   @@ -1162,31 +1151,18 @@ function comment_load($cid, $reset = FALSE) {
   +      $timestamp = \Drupal::service('history.manager')->getLastViewed($entity_type, array($entity_id));
   
   +++ b/core/modules/forum/lib/Drupal/forum/ForumManager.php
   @@ -329,16 +340,19 @@ protected function numberNew($nid, $timestamp) {
   +      $this->history[$nid] = $this->historyManager->getLastViewed('node', array($nid));
   
   +++ b/core/modules/history/history.module
   @@ -41,9 +42,11 @@ function history_help($path, $arg) {
    function history_read($nid) {
   -  $history = history_read_multiple(array($nid));
   +  $history = \Drupal::service('history.manager')->getLastViewed('node', array($nid));
      return $history[$nid];
    }
   
   +++ b/core/modules/history/lib/Drupal/history/HistoryManagerInterface.php
   @@ -0,0 +1,72 @@
   +   * @return array
   +   *   Array of timestamps keyed by entity ID. If a entity has been previously
   +   *   viewed by the user, the timestamp in seconds of when the last view
   +   *   occurred; otherwise, zero.
   +   */
   +  public function getLastViewed($entity_type, $entity_ids);
   

   The code was using the _read_multiple() function before; the return value is an array and not the timestamp value of the requested entity ID, so that obviously breaks :)

2. +++ b/core/modules/history/history.install
   @@ -31,11 +38,19 @@ function history_schema() {
   +    'primary key' => array(
   +      'uid',
   +      'entity_id',
   +      array('entity_type', 32),
   +    ),
        'indexes' => array(
   ...
   +      'history_entity' => array(
   +        'entity_id',
   +        array('entity_type', 32),
   +      ),
        ),
   

   If you can just move the 'entity_type' field in these indexes to come first, then I'm happy and promise to shut up :)

   (identical change in the update function)

3. +++ b/core/modules/history/history.module
   @@ -41,9 +42,11 @@ function history_help($path, $arg) {
   + * @deprecated Use \Drupal\history\HistoryManager::getLastViewed() instead.
   
   @@ -57,75 +60,33 @@ function history_read($nid) {
   + * @deprecated Use \Drupal\history\HistoryManager::getLastViewed() instead.
   ...
   + * @deprecated Use \Drupal\history\HistoryManager::updateLastViewed() instead.
   

   Do we actually have to keep those deprecated procedural functions around? It looks like the only core consumer is being converted as part of this patch already?

   If possible, let's remove those procedural wrappers directly with this patch?

4. +++ b/core/modules/history/lib/Drupal/history/Controller/HistoryController.php
   @@ -54,9 +83,10 @@ public function readNode(Request $request, NodeInterface $node) {
   +    $this->historyManager->updateLastViewed('node', $node->id());
   
   +++ b/core/modules/history/lib/Drupal/history/HistoryManagerInterface.php
   @@ -0,0 +1,72 @@
   +  public function updateLastViewed($entity_type, $entity_id, AccountInterface $account = NULL);
   

   The last remaining API tweak would be to change the signature to updateLastViewed(EntityInterface $entity), too, if possible. :)

1. +++ b/core/modules/comment/comment.module
   @@ -1162,31 +1151,18 @@ function comment_load($cid, $reset = FALSE) {
   +      $timestamp = \Drupal::service('history.manager')->getLastViewed($entity_type, array($entity_id));
   

   Still an array :)

2. +++ b/core/modules/forum/lib/Drupal/forum/ForumManager.php
   @@ -329,16 +340,19 @@ protected function numberNew($nid, $timestamp) {
   +      $this->history[$nid] = HISTORY_READ_LIMIT;
   

   I guess it would make sense to move this constant into the HistoryManagerInterface?

3. +++ b/core/modules/history/history.install
   @@ -10,16 +10,23 @@
   +      'entity_type' => array(
   ...
   +        'default' => 'node',
   
   +++ b/core/modules/node/node.install
   @@ -720,9 +720,39 @@ function node_update_8011() {
   +  db_add_field('history', 'entity_type', array(
   ...
   +    'initial' => 'node',
   

   As this column is part of the primary key, we have to specify a default value of 'default' => '' in the update function.

   Also note that the schema still defines "node" as default value.

+++ b/core/modules/history/lib/Drupal/history/HistoryManagerInterface.php
@@ -0,0 +1,70 @@
+/**
+ * Provides history manager interface.
+ */
+interface HistoryManagerInterface {

I wonder whether we can find a better description of the capabilities of a history manager, as your current one does not provide any information. "Provides the last view on entities", (maybe?)

1. +++ b/core/modules/node/node.install
   @@ -719,9 +719,40 @@ function node_update_8011() {
   +  // @todo replace with proper migrate implementation.
   

   Should this @todo be finished up before RTBC?

2. +++ b/core/modules/comment/comment.module
   @@ -69,17 +69,6 @@
   -define('COMMENT_NEW_LIMIT', REQUEST_TIME - 30 * 24 * 60 * 60);
   

   How is this being treated now, as I don't see that 30 day equation in the new service?

1. +++ b/core/modules/history/lib/Drupal/history/HistoryManagerInterface.php
   @@ -0,0 +1,70 @@
   + * Provides an interface to manage a last view timestamp of entities.
   

   Yeah "manage" does not really tell you a lot.

2. +++ b/core/modules/history/lib/Drupal/history/HistoryManagerInterface.php
   @@ -0,0 +1,70 @@
   +interface HistoryManagerInterface {
   

   /**
   * Defines an interface to store and retrieve last view timestamp information of entities.
   */
   interface HistoryRepositoryInterface {}

1. +++ b/core/modules/comment/comment.module
   @@ -1119,7 +1108,8 @@ function comment_load($cid, $reset = FALSE) {
     * @param $timestamp
   - *   Time to count from (defaults to time of last user access to node).
   + *   (optional) Time to count from (defaults to time of last user access to
   

   Since we're updating this, we can also put a type hint for $timestamp.

2. +++ b/core/modules/forum/lib/Drupal/forum/ForumManager.php
   @@ -330,16 +341,19 @@ protected function numberNew($nid, $timestamp) {
   +      // History manager service could not be available.
   

   Should we also explain in which case this service might not available?

3. +++ b/core/modules/history/history.install
   @@ -10,16 +10,23 @@
   +    'description' => 'A record of which {users} have read which {entities}s.',
   

   There is no {entities} table..

Oh, I automatically assumed that we wanna rename the class and all variables as well. Don't you think otherwise it would be confusing?

+++ b/core/modules/history/lib/Drupal/history/HistoryRepository.php
@@ -0,0 +1,150 @@
+  public function getLastViewed($entity_type, $entity_ids) {
...
+  public function updateLastViewed(EntityInterface $entity, AccountInterface $account) {

I think we should make it at least possible to pass in a user/UID to get the information from a different user.

This looks ok, but leaving for @dawehner to have one last look

Thank you for the fixes!

Back to RTBC

2081585-history-service-70.patch no longer applies.

error: patch failed: core/modules/comment/comment.module:568
error: core/modules/comment/comment.module: patch does not apply

2081585-history-service-75.patch no longer applies.

error: patch failed: core/modules/history/history.module:57
error: core/modules/history/history.module: patch does not apply

1. +++ b/core/modules/forum/lib/Drupal/forum/ForumManager.php
   @@ -322,24 +333,23 @@ protected function numberNew($nid, $timestamp) {
   @@ -499,7 +509,7 @@ public function checkNodeType(NodeInterface $node) {
   

   This could use an EXPLAIN for the new query, iirc it's pretty bad as it is and this may make it worse.

2. +++ b/core/modules/history/history.install
   @@ -31,11 +38,19 @@ function history_schema() {
   +        array('entity_type', 32),
   

   Similar to the entity caching issue, we probably want to move to table-per-entity type. This is one of the most frequently written to, read from tables and can have tens of thousands of rows if not more.

3. +++ b/core/modules/history/lib/Drupal/history/HistoryRepository.php
   @@ -0,0 +1,160 @@
   +    unset($vars['history']);
   

   Why not just $vars['history'] = array() - then we wouldn't need the __wakeup()?

#1029708: History table for any entity was marked a duplicate of this issue. But it's also in the todo here in Core in comment.module:

/**
 * The time cutoff for comments marked as read for entity types other node.
 *
 * Comments changed before this time are always marked as read.
 * Comments changed after this time may be marked new, updated, or read,
 * depending on their state for the current user. Defaults to 30 days ago.
 *
 * @todo Remove when http://drupal.org/node/1029708 lands.
 */
define('COMMENT_NEW_LIMIT', REQUEST_TIME - 30 * 24 * 60 * 60);

I removed the "beta target" tag because this is pushed to 8.1.x.

This patch stalled 2 years ago on @catch's point:

we probably want to move to table-per-entity type. This is one of the most frequently written to, read from tables and can have tens of thousands of rows if not more.

Is there a reason to block this patch on this point? Would it be better to proceed with this patch and leave table-per-entity for a different issue (by reopening #1029708: History table for any entity as no longer a duplicate)? Does this patch make the lack of table-per-entity worse than it currently is without this patch?

And now?

Is there any news with HistoryManager?

I tried again re-roll the patch #77, but not success. Can anyone help me?

As this helps #1577902: [META] Remove all usages of drupal_static() & drupal_static_reset() by removing drupal_static(), I'm adding that as a parent issue.

Anyone using this patch in Production? I'm curious how its working for you. Our use case is relatively low volume.

Re-roll for 8.8.x

Fixed tests and ForumManager constructor.

Fixed dependencies for some tests

Added update for scheme which also fixes failed tests.

My first quick review:

1. +++ b/core/modules/comment/comment.module
   @@ -57,17 +57,6 @@
   -/**
   - * The time cutoff for comments marked as read for entity types other node.
   - *
   - * Comments changed before this time are always marked as read.
   - * Comments changed after this time may be marked new, updated, or read,
   - * depending on their state for the current user. Defaults to 30 days ago.
   - *
   - * @todo Remove when https://www.drupal.org/node/1029708 lands.
   - */
   -define('COMMENT_NEW_LIMIT', REQUEST_TIME - 30 * 24 * 60 * 60);
   -
   

   This cannot be removed. It must deprecated.

2. +++ b/core/modules/comment/src/CommentManager.php
   @@ -217,20 +217,8 @@ public function getCountNewComments(EntityInterface $entity, $field_name = NULL,
   +        $timestamp = \Drupal::service('history.repository')->getLastViewed($entity->getEntityTypeId(), array($entity->id()), $this->currentUser);
   

   There are a lot of instances where "array()" is used. Please change it to "[]".

3. +++ b/core/modules/forum/src/ForumManager.php
   @@ -329,16 +344,14 @@ protected function getTopicOrder($sortby) {
   +    if (isset($this->history[$nid])) {
   +      return $this->history[$nid];
   +    }
   

   I think that this part can be removed.

4. +++ b/core/modules/history/history.services.yml
   @@ -0,0 +1,4 @@
   +services:
   +  history.repository:
   +    class: Drupal\history\HistoryRepository
   +    arguments: ['@database']
   

   Should this service not be backend overridable?

5. +++ b/core/modules/history/src/Controller/HistoryController.php
   @@ -14,17 +16,44 @@
   -    if ($this->currentUser()->isAnonymous()) {
   +    $account = $this->currentUser();
   +    if ($account->isAnonymous()) {
   
   @@ -42,16 +71,20 @@ public function getNodeReadTimestamps(Request $request) {
   -    if ($this->currentUser()->isAnonymous()) {
   +    $account = $this->currentUser();
   +    if ($account->isAnonymous()) {
   

   I do not think that these changes are necessary.

I update the patch #107 solving observations at #108

About observations at #109

1. I think that it is correctly removed. I don't find any place where is used this.
2. Solved
3. It is a validation before do something. If you remove the code made operations without reason
4. Nothing to do
5. I am unsure about remove it.

For #109.1: Because Drupal core does not use this variable any more does mean that there are contrib or other modules that depend on the variable to be there.

/**
 * The time cutoff for comments marked as read for entity types other node.
 *
 * Comments changed before this time are always marked as read.
 * Comments changed after this time may be marked new, updated, or read,
 * depending on their state for the current user. Defaults to 30 days ago.
 *
 * @deprecated in Drupal 8.8.x and will be removed before Drupal 9.0.0.
 */
define('COMMENT_NEW_LIMIT', REQUEST_TIME - 30 * 24 * 60 * 60);

For #109.3: The part can be removed, because right after we test if that variable is empty. If it is empty then we are going to set the variable. After that the value of the variable is returned. The part that you want to add does not help in any way. it only makes the code more complicated. Please remove this change.

For #109.4: The whole idea of this issue is to back history modules database queries swappable. For that to be possible the service needs to be backend overridable. See: https://www.drupal.org/node/2306083.

services:
  history.repository:
    class: Drupal\history\HistoryRepository
    arguments: ['@database']
    tags:
      - { name: backend_overridable }

For #109.5: The value for $this->currentUser() is always set. It can be for an authenticated user or for an anonymous user. Please remove this change.

For #109.1: I've added back define('COMMENT_NEW_LIMIT', REQUEST_TIME - 30 * 24 * 60 * 60);
But it's not used in any contrib module.

For #109.3: Fixed.

For #109.4: Fixed.

For #109.5: Fixed.

Also, I've set default value for entity_type column in history table as 'node'.

- Removed rejected changes after applying patch;
- Fixed hook_update_N id as it was said in #108;
- Some minor fixes;

Updated lazyBuilder callback.

My second review:

1. +++ b/core/modules/comment/src/CommentManager.php
   @@ -217,20 +217,8 @@ public function getCountNewComments(EntityInterface $entity, $field_name = NULL,
   +        $timestamp = \Drupal::service('history.repository')->getLastViewed($entity->getEntityTypeId(), [$entity->id()], $this->currentUser);
   

   Can we change the variable name to $timestamps. To make clear that it is an array of timestamps and not a single timestamp.

2. In the function history_update_8801() uses several field and table definitions. Can we use the definition from history_schema(). Single source for the table and field definitions.
3. I am not a 100 % sure, but I think there needs to be a specific test for the update from the function history_update_8801().

4. +++ b/core/modules/comment/tests/src/Functional/CommentLinksTest.php
   @@ -77,6 +77,10 @@ public function testCommentLinks() {
   +    // comment_num_new() relies on history_read(), so ensure that no one has
   +    // seen the node of this comment.
   +    \Drupal::service('history.repository')->deleteByEntity($this->node);
   +
   

   Just to be sure. If you remove this change the test will fail? Also, I am unable to find the function comment_num_new().

5. +++ b/core/modules/history/src/HistoryRepository.php
   @@ -0,0 +1,164 @@
   +    $return = [];
   ...
   +        $return[$entity_id] = $this->history[$entity_type][$account->id()][$entity_id];
   ...
   +      return $return;
   ...
   +    return $return + $entities_to_read;
   

   Can we rename the variable $return to something more descriptive like $entities. If you have a better idea that would be great.

6. +++ b/core/modules/history/src/HistoryRepositoryInterface.php
   @@ -0,0 +1,71 @@
   +   * Deletes history of the user account.
   

   Maybe better: "Deletes the history for the given user account."

1. Fixed.
4. Removed as it doesn't affect on test.
5. Fixed.
6. Fixed.

So all review points adressed except #115.3:

a specific test for the update from the function history_update_8801().

and #79.1 and #79.3

I would like to state that I agree with @andypost’s point from comment #116.

The patch looks good to me. What I still mis is a specific update test for the function history_update_8801().
There is documentation for how to write such a test. See: https://www.drupal.org/docs/8/api/update-api/writing-automated-update-te....

Added test for function history_update_8801() and fixed failed tests.

Green now.
Regarding #79:
#79.1 - I don't get what should be changed.
#79.3 - The same thing is used in ForumManager.php and TermStorage.php. Do we really need to change it?

Thank you @Vlad Bo for adding the update test.
The issue summary is updated.
The patch is now RTBC for me.

The architectural review needs to be done by a core committer.

+++ b/core/modules/history/history.install
@@ -6,22 +6,30 @@
+        'default' => 'node',

I don't think 'node' should be the default in the actual schema definition if we're trying to make the module entity-type agnostic.

1. +++ b/core/modules/history/src/HistoryRepository.php
   @@ -0,0 +1,175 @@
   +   * Array of history keyed by entity type and entity id.
   

   nit: ID.

2. +++ b/core/modules/history/src/HistoryRepositoryInterface.php
   @@ -0,0 +1,71 @@
   +  public function resetCache();
   

   Does this need to be in the interface?

   Also the protected property here would be a good case for #3047289: Standardize how we implement in-memory caches. We're already using this for some other services.

Seems you would not define the default field value so you will fill the value of entity_type field once after the field updates. I suggest moving that insert operation in the separate hook_update_N implementation.

#123.1 - fixed.
#123.2 - removed.

Default value for entity_type field was removed and update query was added to set it.

I am missing in the update test that the value of the column entity_type is equal to "node".

edit: @Vlad Bo: The changes that you made look good to me.

Added check for entity_type value in update test.

All the points from @catch are fixed. Back to RTBC.

The architectural review needs to be done by a core committer.

+++ b/core/modules/history/src/HistoryRepository.php
@@ -0,0 +1,175 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function resetCache() {
+    $this->history = [];
+  }

This is no-longer in the interface but it still has @inheritdoc. I think we should use a memory cache implementation straight off here, which would also allow us to remove this as well as the the __sleep() and __wakeup() methods.

Removed resetCache method and added memory cache implementation.

Fixed loading timestamp from cache

One more fix for tests

Everything that @catch asked for has been added to the patch.

I have 2 minor issues left for the patch:

1. +++ b/core/modules/history/src/HistoryRepository.php
   @@ -0,0 +1,189 @@
   +        $this->time->getRequestTime(), Cache::PERMANENT,
   

   Minor: This needs to be on two lines.

2. +++ b/core/modules/comment/comment.module
   @@ -65,7 +65,7 @@
   + * @deprecated in drupal:8.8.0 and is removed from drupal:9.0.0.
   

   I do not think that this patch will land in 8.8.

Thanks @Vlad Bo for your work on this patch.

Mostly fixed:

• #137.1 - fixed
• #137.2 - fixed
• #137.3 - added dependency, need to be split in #2188287: Split CommentManager in two
• #137.4 - fixed
• #137.5 - fixed
• #137.6 - fixed
• #137.7 - seems like nothing to be thrown

Fixed fails, changed format of trigger_error messages and added dependency in comment module.

Rerolling patch for 9.1.x and trying to fix test cases.

Fixing test cases!

Keeping fix the tests.
Fixed the fixture location definition in the update test.
Added missing schema installation calls in failing tests.

Patch re-rolled and manual fixed.

Fix test dependencies and code style.