Updated: Comment #0

Problem/Motivation

History module implemented as straight d7 port and hardcoded queries to history table all over core

Proposed resolution

Introduce swappable HistoryManager service to allow more performant implementations in contrib to figure out actual needs and help to make improvements in newer version of core.
Incorporate table structure from #1029708: History table for any entity

Remaining tasks

Implement HistoryManager and HistoryManagerInterface
Replace all usages of history table with proper service calls

User interface changes

No

API changes

Deprecated functions:
history_read($nid) and history_read_multiple($nids) - HistoryRepositoryInterface::getLastViewed($entity_type, $entity_ids, AccountInterface $account)
history_write($nid, AccountInterface $account = NULL) - updateLastViewed(EntityInterface $entity, AccountInterface $account)

#1029708: History table for any entity
#2078753: Add history_read_multiple()
#1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user

Files: 
CommentFileSizeAuthor
#77 2081585-history-service-77.patch29.75 KBandypost
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,051 pass(es).
[ View ]
#70 interdiff.txt4.64 KBandypost

Comments

andypost’s picture

Status:Active» Needs review
Issue tags:+Needs tests
StatusFileSize
new21.06 KB
FAILED: [[SimpleTest]]: [MySQL] 56,521 pass(es), 489 fail(s), and 472 exception(s).
[ View ]
new11.64 KB

Initial implementation on top of #1029708-60: History table for any entity

The method names are need discussion and tests

Status:Needs review» Needs work

The last submitted patch, drupal8.history-module.2081585-1.patch, failed testing.

dawehner’s picture

+++ 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?

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new19.36 KB
FAILED: [[SimpleTest]]: [MySQL] 56,624 pass(es), 511 fail(s), and 415 exception(s).
[ View ]
new7.35 KB

Removed changes to history_read() and write() and mark them deprecated.
Renamed service name to 'history.manager' and should fix most of comment related tests.
No idea how to fix upgrade tests, forum manager depends on new history module...

@dawehner The reason is that scope of current_user service is limited by request scope and using strict=false is ugly here

andypost’s picture

StatusFileSize
new19.43 KB
FAILED: [[SimpleTest]]: [MySQL] 57,823 pass(es), 464 fail(s), and 341 exception(s).
[ View ]
new771 bytes

Should fix upgrade tests

dawehner’s picture

+++ 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?

andypost’s picture

StatusFileSize
new19.53 KB
FAILED: [[SimpleTest]]: [MySQL] 57,914 pass(es), 464 fail(s), and 341 exception(s).
[ View ]
new1.62 KB

Makes sense

dawehner’s picture

What about marking the HistoryManagerInterface on the constructor = NULL?

andypost’s picture

StatusFileSize
new19.53 KB
FAILED: [[SimpleTest]]: [MySQL] 57,959 pass(es), 463 fail(s), and 338 exception(s).
[ View ]
new1.02 KB

Yep, this will document that the dependency is optional

andypost’s picture

StatusFileSize
new19.63 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.history-module.2081585-10.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new664 bytes

fix exceptions, enough for today!

Status:Needs review» Needs work

The last submitted patch, drupal8.history-module.2081585-10.patch, failed testing.

larowlan’s picture

StatusFileSize
new20.62 KB

Test results for posterity:

Screenshot 2013-10-02 09.06.02.png

larowlan’s picture

Status:Needs work» Needs review
Issue tags:-Needs tests, -WSCCI

Status:Needs review» Needs work
Issue tags:+Needs tests, +WSCCI

The last submitted patch, drupal8.history-module.2081585-10.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new19.64 KB
FAILED: [[SimpleTest]]: [MySQL] 58,725 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new1.99 KB

Re-roll and fixes

Status:Needs review» Needs work

The last submitted patch, drupal8.history-module.2081585-15.patch, failed testing.

larowlan’s picture

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new23.27 KB
PASSED: [[SimpleTest]]: [MySQL] 58,634 pass(es).
[ View ]
new9.13 KB
  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.

Status:Needs review» Needs work
Issue tags:-WSCCI

The last submitted patch, history-manager-2081585.17.patch, failed testing.

larowlan’s picture

Status:Needs work» Needs review
Issue tags:+WSCCI

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

dawehner’s picture

Status:Needs review» Needs work
+++ 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.

damiankloip’s picture

Status:Needs work» Needs review

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?

andypost’s picture

This issue is mostly ready to replace #1029708-54: History table for any entity so once all nitpicks addressed probably one of them should be closed as duplicate. I've started this one to figure out how the service should operate and then move to #1029708-61: History table for any entity

Could just inject the current user service instead.

Having current_user injected breaks a lot of tests that changes users so better have optional argument

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

Makes no sense, because we should pass their entity types as well.

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

Makes sense

damiankloip’s picture

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

andypost’s picture

The reason is #2102027: Add back the request scope to the current user service so service should be agnostic to current_user that could be limited to request scope

+++ b/core/modules/history/lib/Drupal/history/HistoryManager.php
@@ -0,0 +1,143 @@
+  public function read($entity_type, $entity_ids) {
...
+      ->condition('uid', \Drupal::currentUser()->id())
...
+  public function write($entity_type, $entity_id, AccountInterface $account = NULL) {
+    if (!isset($account)) {
+      $account = \Drupal::currentUser();

Seems we need to add $account to read() and check the usage

andypost’s picture

Issue summary:View changes
StatusFileSize
new3.19 KB
new23.28 KB
FAILED: [[SimpleTest]]: [MySQL] 59,279 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

re-roll with fixes for #21

Status:Needs review» Needs work

The last submitted patch, 25: history-manager-2081585.25.patch, failed testing.

sun’s picture

  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?

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new3.97 KB
new22.92 KB
PASSED: [[SimpleTest]]: [MySQL] 59,463 pass(es).
[ View ]

Should fix failure, also trying to address #27.8

@sun thanx a lot for the review, I'm not so good in naming so should think a bit more about 1 and 2,

3) makes sense
4) yep, and convert to ContentEntityInterface
5) not found any references about "initial" but the purpose exactly the same, so added hunk to remove this and @todo about migrate
6) not found any issues about, it there any please point me, it makes sense to file separate issue for that (comment field affected too)
7) probably, no idea how to test this

EDIT suppose defaults for entity_type and entity_id should be removed like #1029708-69: History table for any entity (70)

PS: related about 1 #1961548: [Policy, no patch] Decide how the module-specific services are made available as static methods (similar to \Drupal::$service())

sun’s picture

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:

<?php
array(
 
'default' => '',
 
'initial' => 'node',
)
?>
andypost’s picture

StatusFileSize
new7.67 KB
new25.22 KB
FAILED: [[SimpleTest]]: [MySQL] 59,336 pass(es), 0 fail(s), and 300 exception(s).
[ View ]

Fixed 3,4,5 and remaining @todo in comment module

andypost’s picture

StatusFileSize
new6.35 KB
new25.36 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

just go with getLastViewed() and updateLastViewed().

This names better explains and could be grep'ed easily

The last submitted patch, 30: history-manager-2081585.30.patch, failed testing.

The last submitted patch, 30: history-manager-2081585.30.patch, failed testing.

sun’s picture

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

andypost’s picture

StatusFileSize
new4.22 KB
new25.36 KB
FAILED: [[SimpleTest]]: [MySQL] 59,416 pass(es), 6 fail(s), and 8 exception(s).
[ View ]

@sun Thanx a lot for point!
1) fixed
2) changed as proposed
3) No api change to make this in, and related #2042165: Add a 'deprecated' module that includes deprecated functions only when enabled
4) makes sense, fixed

The last submitted patch, 31: history-manager-2081585.31.patch, failed testing.

sun’s picture

  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.

Status:Needs review» Needs work

The last submitted patch, 35: history-manager-2081585.35.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new1.54 KB
new25.5 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

1) fixed
2) this is not constant define('HISTORY_READ_LIMIT', REQUEST_TIME - 30 * 24 * 60 * 60); suppose we need separate issue to deal with it
3) fixed, set to empty as 'node' makes no sense as default

andypost’s picture

StatusFileSize
new774 bytes
new25.48 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch history-manager-2081585.40.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Fixes broken tests

The last submitted patch, 39: history-manager-2081585.39.patch, failed testing.

andypost’s picture

40: history-manager-2081585.40.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 40: history-manager-2081585.40.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
Issue tags:+DX (Developer Experience)
StatusFileSize
new25.43 KB
PASSED: [[SimpleTest]]: [MySQL] 59,371 pass(es).
[ View ]

re-roll

andypost’s picture

Issue tags:+beta target

Can someone RTBC this?

dawehner’s picture

+++ 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?)

joelpittet’s picture

  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?

andypost’s picture

StatusFileSize
new1.1 KB
new25.46 KB
PASSED: [[SimpleTest]]: [MySQL] 63,376 pass(es).
[ View ]

#46 fixed

#47.1 fixed
#47.2 HISTORY_READ_LIMIT is the same constant

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

@@ -1160,20 +1150,8 @@ function comment_num_new($entity_id, $entity_type, $field_name = NULL, $timestam
-          // @todo Remove once http://drupal.org/node/1029708 lands.
-          $timestamp = COMMENT_NEW_LIMIT;

@@ -1160,20 +1150,8 @@ function comment_num_new($entity_id, $entity_type, $field_name = NULL, $timestam
     $timestamp = ($timestamp > HISTORY_READ_LIMIT ? $timestamp : HISTORY_READ_LIMIT);

HISTORY_READ_LIMIT

dawehner’s picture

  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 {}

amateescu’s picture

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

andypost’s picture

StatusFileSize
new6.4 KB
new25.55 KB
PASSED: [[SimpleTest]]: [MySQL] 63,336 pass(es).
[ View ]

@dawehner thatx for idea, just fixed to 80 chars limit

andypost’s picture

StatusFileSize
new3.58 KB
new25.53 KB
FAILED: [[SimpleTest]]: [MySQL] 63,364 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Let's see if upgrade tests still affected...

dawehner’s picture

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

Status:Needs review» Needs work

The last submitted patch, 52: history-manager-2081585.52.patch, failed testing.

andypost’s picture

Suppose this service needs current user injected otherwise the static cache is invalid
OTOH the history module storage assumes that it should be user to track the last authorized

+++ b/core/modules/history/lib/Drupal/history/HistoryManager.php
@@ -0,0 +1,144 @@
+  public function getLastViewed($entity_type, $entity_ids) {
...
+    $result = $this->connection->select('history', 'h')
...
+      ->condition('uid', \Drupal::currentUser()->id())
...
+      ->condition('entity_id', array_keys($entities_to_read), 'IN')
...
+  public function updateLastViewed(EntityInterface $entity, AccountInterface $account = NULL) {
...
+    if ($account->isAuthenticated()) {
+      $this->connection->merge('history')
...
+          'uid' => $account->id(),
+          'entity_id' => $entity->id(),

$account should be injected or passed as required argument

andypost’s picture

To track anonymous node views there's statistics module so "history" should track authorized

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new14.63 KB
new24.58 KB
FAILED: [[SimpleTest]]: [MySQL] 62,170 pass(es), 24 fail(s), and 0 exception(s).
[ View ]

Re-roll and clean-up:
- user account is required
- s/manager/repository
- fix forum manager test

Status:Needs review» Needs work

The last submitted patch, 57: 2081585-history-service-57.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new1.42 KB
new24.6 KB
PASSED: [[SimpleTest]]: [MySQL] 63,660 pass(es).
[ View ]

Fix tests

andypost’s picture

StatusFileSize
new1.82 KB
new24.9 KB
PASSED: [[SimpleTest]]: [MySQL] 63,664 pass(es).
[ View ]

To keep static cache valid the current user should be injected

dawehner’s picture

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

andypost’s picture

StatusFileSize
new11.26 KB
new29.42 KB
FAILED: [[SimpleTest]]: [MySQL] 63,280 pass(es), 394 fail(s), and 22 exception(s).
[ View ]

Account added as required argument, now we should get rid the static cache and purge functions

Status:Needs review» Needs work

The last submitted patch, 62: 2081585-history-service-62.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new2.54 KB
new29.44 KB
FAILED: [[SimpleTest]]: [MySQL] 64,136 pass(es), 9 fail(s), and 1,336 exception(s).
[ View ]

fixed remains of "manager"

Status:Needs review» Needs work

The last submitted patch, 64: 2081585-history-service-64.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new1.75 KB
new29.45 KB
PASSED: [[SimpleTest]]: [MySQL] 64,174 pass(es).
[ View ]

one more round of bugfixes caused by rename

larowlan’s picture

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

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

Thank you for the fixes!

andypost’s picture

Title:Introduce HistoryManager service» Introduce HistoryRepository service
Issue summary:View changes
StatusFileSize
new29.47 KB
PASSED: [[SimpleTest]]: [MySQL] 64,410 pass(es).
[ View ]
andypost’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new4.64 KB
new29.71 KB
PASSED: [[SimpleTest]]: [MySQL] 64,533 pass(es).
[ View ]

1) Removed current_user service dependency according #2180109-96: Change the current_user service to a proxy
2) fixed static cache to cache per user.
3) extended service from DependencySerialization to prevent serialization of static cache and database connection
4) filed follow-up for forum manager service #2201659: Fix serialization of the forum manager and dependency on current_user service

andypost’s picture

There's issue with entity_id now according to #2195915-7: Cannot save text filter config using PostgreSQL if Comment is enabled [blocks installation!]
So this kind of storage is fragile, trying to re-think this into field...

otoh this out-of-bound data that does not coupled to entity, that means update of history does not lead to update of entity

larowlan’s picture

Status:Needs review» Reviewed & tested by the community

Back to RTBC

andypost’s picture

About consistency for entity_id (could be integer or string) suppose we have follow-up #2100203: Make config entities use dots in machine names consistently

Suppose this issue ready, @catch I think for the storage we can at least in 8.0.1 add second table field {bundle_id} with logic return history->id() ?: history->bundle_id

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

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
andypost’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new29.75 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,694 pass(es).
[ View ]
alexpott’s picture

Status:Reviewed & tested by the community» Needs work

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
andypost’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new29.75 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,051 pass(es).
[ View ]
Sutharsan’s picture

Issue tags:-Needs reroll
catch’s picture

Status:Reviewed & tested by the community» Needs work
  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()?

andypost’s picture

It's nice idea about "table-per-entity type" but there's no core api to generate this tables.
Suppose we need to create/drop this tables manually when entity types is defined or make history a configurable field to inherit storage
The same applies to comment table

mgifford’s picture

#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);
joelpittet’s picture

Assigned:catch» Unassigned
Crell’s picture

Issue tags:-WSCCI
Crell’s picture

Version:8.0.x-dev» 8.1.x-dev
cilefen’s picture

Issue tags:-beta target

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