Make HISTORY_READ_LIMIT configurable

Nice idea

We need to get rid of that define() anyway.

1. @@ -0,0 +1,5 @@
   +history_mark_read: '2592000'
   

   this should have schema for introduced config https://drupal.org/node/1905070 and #1910624: [META] Introduce and complete configuration schemas in all of core

2. @@ -71,8 +63,9 @@ function history_write($nid, $account = NULL) {
   +  $history_read_limit = REQUEST_TIME - config('history.settings')->get('history_mark_read');
   
   @@ -79,18 +79,21 @@ public function render(ResultRow $values) {
   +      $history_read_limit = REQUEST_TIME - config('history.settings')->get('history_mark_read');
   
   @@ -67,7 +67,7 @@ public function query() {
   +    $limit = REQUEST_TIME - config('history.settings')->get('history_mark_read');
   

   s/config/Drupal::config, see #1957142: Replace config() with Drupal::config()

Talked with Crell, Drupal::config is not ok when we're in a namespace. Switched over in the .module file, trying to use the dependency injection for the views plugins.

However, this patch fails locally with "Declaration of Drupal\history\Plugin\views\field\HistoryUserTimestamp::create() must be compatible with that of Drupal\Core\Plugin\ContainerFactoryPluginInterface::create()". Not sure how it's incompatible though, they look pretty identical to me.

Hopefully someone else can run with it or let me know what's wrong. Still need to provide a schema for the config settings as well.

I disabled currently only the cron job, because we want not to delete the information when user has read the node.
Patch works on 8.5.x

We should it make configurable via GUI in admin/config/content/history or similar.
If is that a need, it make sense to "redesign" the module so that the above can be configured.

If there's a patch to review, status should be "Needs review".

The patch is only a fix for those which doesn't want delete the history after 30day. "Make HISTORY_READ_LIMIT configurable" is not solved with that patch above

Ah, I understand, thanks. Should still be "needs work" then.

for patch #6

Rerolled for 8.6.x, also, i've checked other modules and services, changed there as well

Fixed some parts

1. +++ b/core/modules/comment/src/CommentManager.php
   @@ -82,6 +89,7 @@ public function __construct(EntityManagerInterface $entity_manager, ConfigFactor
   +    $this->historyMarkRead = $config_factory->get('history.settings')->get('history_mark_read');
   
   +++ b/core/modules/comment/src/Plugin/views/field/NodeNewComments.php
   @@ -46,18 +54,21 @@ public function usesGroupBy() {
   +    $this->historyReadLimit = REQUEST_TIME - $configFactory->get('history.settings')->get('history_mark_read');
   

   Do not cache config inside service

2. +++ b/core/modules/comment/src/CommentManager.php
   @@ -198,7 +206,8 @@ public function getCountNewComments(EntityInterface $entity, $field_name = NULL,
   +      $history_read_limit = REQUEST_TIME - $this->historyMarkRead;
   
   +++ b/core/modules/history/history.module
   @@ -125,8 +117,9 @@ function history_write($nid, $account = NULL) {
   +  $history_read_limit = REQUEST_TIME - \Drupal::config('history.settings')->get('history_mark_read');
   
   +++ b/core/modules/history/src/Plugin/views/field/HistoryUserTimestamp.php
   @@ -18,7 +20,43 @@
   +      REQUEST_TIME - $container->get('config.factory')->get('history.settings')->get('history_mark_read')
   
   +++ b/core/modules/history/src/Plugin/views/filter/HistoryUserTimestamp.php
   @@ -16,10 +18,44 @@
   +      REQUEST_TIME - $container->get('config.factory')->get('history.settings')->get('history_mark_read')
   
   +++ b/core/modules/node/node.module
   @@ -246,6 +246,7 @@ function node_title_list(StatementInterface $result, $title = NULL) {
   +  $history_read_limit = REQUEST_TIME - \Drupal::config('history.settings')->get('history_mark_read');
   

   REQUEST_TIME is deprecated

Errors and notes are fixed, also, i've provided post update to set config for existing instances.

+++ b/core/modules/comment/src/CommentManager.php
@@ -77,19 +85,22 @@
+    $this->historyConfig = $config_factory->get('history.settings');

this is wrong - never store config object in protected properties

1. +++ b/core/modules/comment/src/CommentManager.php
   @@ -57,6 +58,20 @@ class CommentManager implements CommentManagerInterface {
   +  protected $historyConfig;
   
   @@ -70,13 +85,17 @@ class CommentManager implements CommentManagerInterface {
   +    $this->historyConfig = $config_factory->get('history.settings');
   
   @@ -193,7 +212,8 @@ public function getCountNewComments(EntityInterface $entity, $field_name = NULL,
   +      $history_read_limit = $this->time->getRequestTime() - $this->historyConfig->get('history_mark_read');
   
   +++ b/core/modules/comment/src/Plugin/views/field/NodeNewComments.php
   @@ -35,6 +37,20 @@ public function usesGroupBy() {
   +   * @var \Drupal\Core\Config\Config|\Drupal\Core\Config\ImmutableConfig
   ...
   +  protected $historyConfig;
   
   @@ -46,18 +62,24 @@ public function usesGroupBy() {
   +    $this->historyConfig = $configFactory->get('history.settings');
   
   @@ -127,14 +149,15 @@ public function preRender(&$values) {
   +      $history_read_limit = $this->time->getRequestTime() - $this->historyConfig->get('history_mark_read');
   

   Do not store config object in protected properties

2. +++ b/core/modules/history/src/Plugin/views/field/HistoryUserTimestamp.php
   @@ -18,7 +20,43 @@
   +  protected $historyReadLimit;
   
   +++ b/core/modules/history/src/Plugin/views/filter/HistoryUserTimestamp.php
   @@ -16,10 +18,44 @@
   +  protected $historyReadLimit;
   

   No reason to store value in protected property

3. +++ b/core/modules/history/src/Plugin/views/field/HistoryUserTimestamp.php
   @@ -18,7 +20,43 @@
   +      REQUEST_TIME - $container->get('config.factory')->get('history.settings')->get('history_mark_read')
   

   REQUEST_TIME is deprecated

4. +++ b/core/modules/history/src/Plugin/views/filter/HistoryUserTimestamp.php
   @@ -16,10 +18,44 @@
   +
   +  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
   

   missing doc block

Rerolled and notes are fixed

Reroll for 8.8.x

Fixed tests

Added needed .yml files from #32 and removed irrelevant core update strings from #36.

Mostly nitpicks:

1. +++ b/core/modules/comment/src/CommentManager.php
   @@ -116,6 +133,13 @@ public function __construct(EntityTypeManagerInterface $entity_type_manager, Con
   +    if (!$time) {
   +      @trigger_error('Invoking the CommentManager constructor without the time service parameter is deprecated in drupal:8.8.0 and will no longer be supported in drupal:9.0.0. The time service parameter is now required in the CommentManager constructor. See https://www.drupal.org/node/2785211', E_USER_DEPRECATED);
   +      $time = \Drupal::service('datetime.time');
   +    }
   +    $this->time = $time;
   

   I like this pattern! Is this documented anywhere as a way of adding injected services to something that might be used or subclassed elsewhere? I've create #3062100: document techniques for changing dependency injection in a backwardsly-compatible way to start a discussion about it.

2. +++ b/core/modules/comment/src/CommentManager.php
   @@ -232,7 +256,10 @@ public function getCountNewComments(EntityInterface $entity, $field_name = NULL,
   +      $historyConfig = $this->configFactory->get('history.settings');
   

   Method vars should be snake case.

3. +++ b/core/modules/comment/src/Plugin/views/field/NodeNewComments.php
   @@ -163,14 +195,15 @@ public function preRender(&$values) {
   +      $history_read_limit = $this->time->getRequestTime() - $this->configFactory->get('history.settings')->get('history_mark_read');
   

   I feel this line would be more readable if the expressions on the right were assigned to variables.

4. +++ b/core/modules/history/history.module
   @@ -15,14 +15,6 @@
   -define('HISTORY_READ_LIMIT', REQUEST_TIME - 30 * 24 * 60 * 60);
   

   We need to leave this constant in for BC, as custom code may be making use of it.

   (Though I don't think there's a way to trigger a deprecation warning for a global constant?)

5. +++ b/core/modules/history/history.post_update.php
   @@ -0,0 +1,13 @@
   + ¶
   

   Whitespace.

6. +++ b/core/modules/history/src/Plugin/views/field/HistoryUserTimestamp.php
   @@ -92,13 +143,14 @@ public function render(ResultRow $values) {
   +      $history_read_limit = $this->time->getRequestTime() - $this->configFactory->get('history.settings')->get('history_mark_read');
   

   Same here.
   Though on second thoughts, this calculation is repeated at least twice. Could it be moved to a helper method getHistoryReadLimist() on a service somewhere?

Fix nits, btw it needs upgrade path tests (config could change in future so this upgrade should add only "read-mark" one)

Mean time it sounds like great candidate to foundation of history.repository service #2081585: [META] Modernize the History module

No reason to prefix config property as it already namespaced

Also it exposed issues in node and comment modules integration (no dependency on history one)

So it needs proper integration a-la
- is service history.repository exists and fall back to old default
- if exists call its method

PS: I think there's no reason to provide UI for this setting so it could be container parameter instead of config, so in rare cases when it needs override it will be overridable with settings.php or services.yml

Seems like good idea. So if I got it right, we need to postpone current issue, re-roll #2081585: [META] Modernize the History module and then move HISTORY_READ_LIMIT to history.repository as container parameter.

1. +++ b/core/modules/history/config/install/history.settings.yml
   @@ -0,0 +1,5 @@
   +mark_read: 2592000
   
   +++ b/core/modules/history/history.module
   @@ -15,14 +15,6 @@
   -define('HISTORY_READ_LIMIT', REQUEST_TIME - 30 * 24 * 60 * 60);
   

   The new config "mark_read" is in a lot of places directly swapped with the constant HISTORY_READ_LIMIT, only they are not the same.

2. +++ b/core/modules/history/history.module
   @@ -15,14 +15,6 @@
   -/**
   - * Entities changed before this time are always shown as read.
   - *
   - * Entities changed within this time may be marked as new, updated, or read,
   - * depending on their state for the current user. Defaults to 30 days ago.
   - */
   -define('HISTORY_READ_LIMIT', REQUEST_TIME - 30 * 24 * 60 * 60);
   

   We cannot just remove this constant. It need to be properly deprecated first.

3. Update test for the new config setting "mark_read"