The statistics module settings (admin/config/system/statistics) need to be converted to use the new configuration system.

Tasks

  • Create a default config file and add it to the statistics module.
  • Convert the admin UI in statistics.admin.inc to read to/write from the appropriate config.
  • Convert any code that currently accesses the variables used to use the config system.
  • Write an upgrade path from D7 -> D8 to migrate existing data to the new system and drop the appropriate variables.

Patch attached that covers the first 3 points. Waiting for #1497132: Provide a generalised function to update variables from Drupal 7 to Drupal 8's configuration management system to cover the upgrade.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, statistics_config_settings.patch, failed testing.

pcambra’s picture

Status: Needs work » Needs review
FileSize
19.42 KB

Tests fixed, resubmitting

marcingy’s picture

Status: Needs review » Needs work

Should we be renaming variables such as statistics_enable_access_log to enable_access_log because effectively we are repeating data because the configuration is now namespace. I think we need to come up with a general agreement on this as it seems to be a question that is commonly being asked.

Also there is a call to update_variables_to_config which is not yet committed #1497132: Provide a generalised function to update variables from Drupal 7 to Drupal 8's configuration management system so this issue needs to be postponed on that going in.

Can't this

$statistics_count_content_views = config('statistics.settings')->get('statistics_count_content_views');
if (!empty($statistics_count_content_views)) {

simply be

 if (config('statistics.settings')->get('statistics_count_content_views')) {

As NULL will evaluate as FALSE.

gdd’s picture

+++ b/core/modules/statistics/statistics.moduleundefined
@@ -73,7 +78,7 @@ function statistics_exit() {
@@ -230,20 +235,20 @@ function statistics_user_predelete($account) {

@@ -230,20 +235,20 @@ function statistics_user_predelete($account) {
  * Implements hook_cron().
  */
 function statistics_cron() {
-  $statistics_timestamp = variable_get('statistics_day_timestamp', '');
+  $statistics_timestamp = config('statistics.settings')->get('statistics_day_timestamp');
 
   if ((REQUEST_TIME - $statistics_timestamp) >= 86400) {
     // Reset day counts.
     db_update('node_counter')
       ->fields(array('daycount' => 0))
       ->execute();
-    variable_set('statistics_day_timestamp', REQUEST_TIME);
+    config('statistics.settings')->set('statistics_day_timestamp', REQUEST_TIME)->save();
   }
 
   // Clean up expired access logs (if applicable).
-  if (variable_get('statistics_flush_accesslog_timer', 259200) > 0) {
+  if (config('statistics.settings')->get('statistics_flush_accesslog_timer') > 0) {
     db_delete('accesslog')
-      ->condition('timestamp', REQUEST_TIME - variable_get('statistics_flush_accesslog_timer', 259200), '<')
+      ->condition('timestamp', REQUEST_TIME - config('statistics.settings')->get('statistics_flush_accesslog_timer'), '<')
       ->execute();
   }

This should be changed to use one $config object throughout the function to prevent unnecessary object instantiation.

+++ b/core/modules/statistics/statistics.testundefined
@@ -324,8 +328,8 @@ class StatisticsAdminTestCase extends DrupalWebTestCase {
-    $this->assertFalse(variable_get('statistics_enable_access_log', 0), t('Access log is disabled by default.'));
-    $this->assertFalse(variable_get('statistics_count_content_views', 0), t('Count content view log is disabled by default.'));
+    $this->assertFalse(config('statistics.settings')->get('statistics_enable_access_log'), t('Access log is disabled by default.'));
+    $this->assertFalse(config('statistics.settings')->get('statistics_count_content_views'), t('Count content view log is disabled by default.'));
 
     $this->drupalGet('admin/reports/pages');
     $this->assertRaw(t('No statistics available.'), t('Verifying text shown when no statistics is available.'));
@@ -334,8 +338,8 @@ class StatisticsAdminTestCase extends DrupalWebTestCase {

@@ -334,8 +338,8 @@ class StatisticsAdminTestCase extends DrupalWebTestCase {
     $edit['statistics_enable_access_log'] = 1;
     $edit['statistics_count_content_views'] = 1;
     $this->drupalPost('admin/config/system/statistics', $edit, t('Save configuration'));
-    $this->assertTrue(variable_get('statistics_enable_access_log'), t('Access log is enabled.'));
-    $this->assertTrue(variable_get('statistics_count_content_views'), t('Count content view log is enabled.'));
+    $this->assertTrue(config('statistics.settings')->get('statistics_enable_access_log'), t('Access log is enabled.'));
+    $this->assertTrue(config('statistics.settings')->get('statistics_count_content_views'), t('Count content view log is enabled.'));

Same here

I don't want to worry too much about nitpicking over the key names. It will make it easier for users moving to Drupal 8 if we have the same names, and I don't really think the extra explicitness really hurts anything. I also find it hard to get too worked up over setting a variable and checking it, rather than checking the return of the function. Setting then checking is easier to read and I don't think it really hurts anything.

3 days to next Drupal core point release.

pcambra’s picture

Status: Needs work » Needs review
FileSize
19.4 KB

Here are some of the changes implemented.

Regarding testStatisticsSettings, I've found something strange (from my understanding), I couldn't turn

  function testStatisticsSettings() {
    $this->assertFalse(config('statistics.settings')->get('statistics_enable_access_log'), t('Access log is disabled by default.'));
    $this->assertFalse(config('statistics.settings')->get('statistics_count_content_views'), t('Count content view log is disabled by default.'));

    $this->drupalGet('admin/reports/pages');
    $this->assertRaw(t('No statistics available.'), t('Verifying text shown when no statistics is available.'));

    // Enable access log and counter on content view.
    $edit['statistics_enable_access_log'] = 1;
    $edit['statistics_count_content_views'] = 1;
    $config = config('statistics.settings');
    $this->drupalPost('admin/config/system/statistics', $edit, t('Save configuration'));
    $this->assertTrue(config('statistics.settings')->get('statistics_enable_access_log'), t('Access log is enabled.'));
    $this->assertTrue(config('statistics.settings')->get('statistics_count_content_views'), t('Count content view log is enabled.'));

Into:

  function testStatisticsSettings() {
    $config = config('statistics.settings');
    $this->assertFalse($config->get('statistics_enable_access_log'), t('Access log is disabled by default.'));
    $this->assertFalse($config->get('statistics_count_content_views'), t('Count content view log is disabled by default.'));

    $this->drupalGet('admin/reports/pages');
    $this->assertRaw(t('No statistics available.'), t('Verifying text shown when no statistics is available.'));

    // Enable access log and counter on content view.
    $edit['statistics_enable_access_log'] = 1;
    $edit['statistics_count_content_views'] = 1;
    $config = config('statistics.settings'); // even forcing the config again.
    $this->drupalPost('admin/config/system/statistics', $edit, t('Save configuration'));
    $this->assertTrue($config->get('statistics_enable_access_log'), t('Access log is enabled.'));
    $this->assertTrue($config->get('statistics_count_content_views'), t('Count content view log is enabled.'));

Test fails as the values aren't retrieved correctly, but if I instantiate the config for each element as the patch attached, it works.

gdd’s picture

Status: Needs review » Needs work

This is currently in need of a reroll, it no longer applies. Also

+++ b/core/modules/statistics/statistics.installundefined
@@ -134,3 +120,10 @@ function statistics_schema() {
+function statistics_update_8000() {
+  update_variables_to_config('statistics.settings');
+}

The update functions for 7.x to 8.x should be contained in a special defgroup, see system.install for an example.

12 days to next Drupal core point release.

webchick’s picture

Assigned: pcambra » webchick

Yoinking for a re-roll.

webchick’s picture

Status: Needs work » Needs review
FileSize
19.14 KB

Let's try this on for size. Included the @defgroup as well. Followed instructions at http://learndrupal.org/lesson/b93deb54-264f-19a4-49eb-db74bc6c2712

Status: Needs review » Needs work

The last submitted patch, cmi-statistics-reroll.patch, failed testing.

alexpott’s picture

The patch in #8 is missing the configuration XML file. Creating patches that contain new files is always a bit more tricky as git has to be told about them.

Reading http://learndrupal.org/lesson/b93deb54-264f-19a4-49eb-db74bc6c2712 there needs to be a step between 10 and 11 to review any files that have been added or removed by the patch probably using "git status" and doing "git add/rm" as appropriate.

webchick’s picture

Status: Needs work » Needs review
FileSize
19.91 KB

Nuts. Thanks, Alex. I'm a little rusty. :D

Let's try this on for size!

Status: Needs review » Needs work

The last submitted patch, config-statistics-reroll.patch, failed testing.

webchick’s picture

Your mom, testbot! :P

webchick’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, config-statistics-reroll.patch, failed testing.

webchick’s picture

Assigned: webchick » Unassigned

Well, I give up for now. If someone else wants a shot, go for it!

webchick’s picture

Status: Needs work » Needs review
FileSize
20.51 KB

I lied. I think I got it this time. modules/statistics/statstics.php is a new file since this patch was rolled and had not been converted to the new config system.

Status: Needs review » Needs work

The last submitted patch, config-one-more-blasted-time.patch, failed testing.

webchick’s picture

Status: Needs work » Needs review
FileSize
20.47 KB

I should not attempt fancy git things I do not understand.

Status: Needs review » Needs work

The last submitted patch, config-got-you-now-sucker.patch, failed testing.

webchick’s picture

Assigned: Unassigned » webchick

All right, testbot. IT IS ON.

webchick’s picture

Ok, so all of these tests are failing in the same basic way.

Without the patch, the flow goes like this:

- View node/1. There are no views shown.
- View node/1 again. It shows "Viewed 1 times."
- View node/1 again. It shows "Viewed 2 times."

So the count displayed on the node is always one behind the number of times it's actually been viewed.

With the patch, it does this:

- View node/1. There are no views shown.
- View node/1 again. It shows "Viewed 2 times."
- View node/1 again. It shows "Viewed 3 times."

So after the first view, the count displayed on the node is always the same as the number of times it's actually been viewed.

It's trivial to update the tests to account for this new behaviour, but I'm not sure if it's actually behaving as intended. I suspect not. Will go digging.

webchick’s picture

Status: Needs work » Needs review
Issue tags: -Configuration system, -Config novice

#19: config-got-you-now-sucker.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Configuration system, +Config novice

The last submitted patch, config-got-you-now-sucker.patch, failed testing.

timmillwood’s picture

I don't think the patch should be adding this into statistics.module

+  if (!empty($statistics_count_content_views)) {
+    // We are counting content views.
+    if (arg(0) == 'node' && is_numeric(arg(1)) && arg(2) == NULL) {
+      // A node has been viewed, so update the node's counters.
+      db_merge('node_counter')
+        ->key(array('nid' => arg(1)))
+        ->fields(array(
+          'daycount' => 1,
+          'totalcount' => 1,
+          'timestamp' => REQUEST_TIME,
+        ))
+        ->expression('daycount', 'daycount + 1')
+        ->expression('totalcount', 'totalcount + 1')
+        ->execute();
+    }
+  }
webchick’s picture

Component: configuration system » statistics.module
Status: Needs work » Needs review
FileSize
19.7 KB

I agree. That probably slipped in with the merge from pre-statistics.php. Also, switching components so this is on Tim's radar.

webchick’s picture

Also, found two other places in the tests where we could streamline $config->get().

Status: Needs review » Needs work

The last submitted patch, now-you-are-mine-statistics-module-mwahahahaha-again.patch, failed testing.

webchick’s picture

Er. Apparently not. ;) #26 it is. Woohoo!

Well, at least I learned a bunch and documented this at http://drupal.org/patch/reroll

webchick’s picture

Assigned: webchick » Unassigned
webchick’s picture

Status: Needs work » Needs review
sun’s picture

Status: Needs review » Needs work
+++ b/core/modules/search/search.test
@@ -450,7 +450,7 @@ class SearchRankingTestCase extends SearchWebTestCase {
+    config('statistics.settings')->set('statistics_count_content_views', 1)->save();

+++ b/core/modules/statistics/config/statistics.settings.xml
@@ -0,0 +1,10 @@
+  <statistics_enable_access_log>0</statistics_enable_access_log>
+  <statistics_flush_accesslog_timer>259200</statistics_flush_accesslog_timer>

I'm sorry, but it looks like the existing config conversions totally set a wrong standard for configuration object key names.

If we don't fix that immediately, then we'll have to convert everything once more :(

So here's the thing:

  1. It's pointless to repeat "statistics" in any key within the object.
  2. Stuff like 'enable_access_log' + 'flush_accesslog_timer' should be rewritten/re-keyed into 'access_log.enabled' + 'access_log.flush_timer'.

    This means that we naturally have a parent key of 'access_log', and you're able to retrieve all related/sub setting values via ->get('access_log'). And of course, they're nicely grouped (even more so after switching to YAML).

    This also applies to the block* settings, which should actually be structured one level deeper:

    block.top_day.num
    block.top_all.num
    block.top_last.num (might even make sense to rename this one to "recent" instead of "top_last")

gdd’s picture

Status: Needs work » Needs review

There was o standard used for variable names other than "keep what they are". We should not be blocking full functional patches while we nitpick and bike shed over variable names. We have lived with these names to now, and there is no current functional need behind changing them. It is more important that these patches land than it is that they land 100% to spec with what someone finds aesthetically pleasing. Take cleanups like this to follow ups, or if you're proposing a new variable naming convention then make a new meta issue.

sun’s picture

jesus. All I'm asking for is this. :(

sun’s picture

Status: Needs review » Needs work
 function statistics_cron() {
...
+    $config->set('statistics_day_timestamp', REQUEST_TIME)->save();

Furthermore, I did not touch this one, because this variable is not configuration.

Thus, we either revert that conversion, or this patch is blocked on finding a solution for non-configuration variables in #1175054: Add a storage (API) for persistent non-configuration state

ZenDoodles’s picture

Issue tags: -Config novice

Concerns from sun make this less of a novice issue.

webchick’s picture

Assigned: Unassigned » webchick

Hello from Core Initiative Windsprints!

This patch needs to be updated for YAML. I'll go ahead and take that on, probably sometime this weekend.

gdd’s picture

Since we have now agreed on naming conventions (http://drupal.org/node/1667896) it looks like #34 is the base patch, but the failures need to be investigated. I assume it also needs a reroll to new HEAD since there have been a ton of changes since May.

tobiasb’s picture

Assigned: webchick » Unassigned
Status: Needs work » Needs review
FileSize
21.73 KB

Updated agains HEAD and the new naming conventions.
It seems that drupal_array_get_nested_value() is not available in cached pages, or I dont know why the tests do not work without the include_once in Config.php.

sun’s picture

tobiasb’s picture

Status: Needs review » Needs work
aspilicious’s picture

+++ b/core/lib/Drupal/Core/Config/Config.phpundefined
@@ -121,6 +121,12 @@ class Config {
+        /* not a full bootstrap, cached page ?
+         * like in Drupal\statistics\Tests\StatisticsLoggingTest->testLogging()
+         */
+        if (!function_exists('drupal_array_get_nested_value')) {

Yes tis should get removed now

+++ b/core/modules/statistics/config/statistics.settings.ymlundefined
@@ -0,0 +1,11 @@
\ No newline at end of file

newline missing at the end of the file

28 days to next Drupal core point release.

aspilicious’s picture

Issue tags: +Config novice

adding tag

alexpott’s picture

+++ b/core/modules/statistics/config/statistics.settings.ymlundefined
@@ -0,0 +1,11 @@
+access_log:
+  enable: '0'
+  flush_timer: '259200'
+count_content_views: '0'
+block:
+  top_day:
+    num: '0'
+  top_all:
+    num: '0'
+  top_last:

Yaml indentation is 4 spaces

+++ b/core/modules/statistics/config/statistics.settings.ymlundefined
@@ -0,0 +1,11 @@
+block:
+  top_day:
+    num: '0'
+  top_all:
+    num: '0'
+  top_last:
+    num: '0'

This level of hierarchy looks unnecessary and block.top_* name could be improved. How about:

block_display:
    top_days: '0'
    top_all: '0'
    top_last: '0'
+++ b/core/modules/statistics/statistics.admin.incundefined
@@ -282,9 +282,9 @@ function statistics_access_log($aid) {
  * Form constructor for the statistics administration form.
  *
  * @ingroup forms
- * @see system_settings_form()
  */
-function statistics_settings_form() {

Needs @see statistics_settings_form_submit()

+++ b/core/modules/statistics/statistics.admin.incundefined
@@ -312,9 +312,22 @@ function statistics_settings_form() {
+/**
+ * Form submission handler for statistics_settings_form().
+ *
+ * @see statistics_settings_form()

Unnecessary @see...

oriol_e9g’s picture

Out of scope:

-    $this->assertText('All time', t('Found the alll time popular content.'));
+    $this->assertText('All time', t('Found the all time popular content.'));

This need to be resolved in a separated issue and remove de t() function for the assert messages.

sun’s picture

@alexpott: see #32

tobiasb’s picture

Status: Needs work » Needs review
FileSize
20.1 KB

Updated agains HEAD.

tobiasb’s picture

* added newline

sun’s picture

Status: Needs review » Needs work
+++ b/core/modules/statistics/config/statistics.settings.yml
@@ -0,0 +1,11 @@
+    enable: '0'

+++ b/core/modules/statistics/statistics.admin.inc
@@ -293,13 +294,13 @@ function statistics_settings_form() {
+    '#default_value' => $config->get('access_log.enable'),

This should be using past-tense; i.e., 'enabled'.

+++ b/core/modules/statistics/config/statistics.settings.yml
@@ -0,0 +1,11 @@
+    flush_timer: '259200'

+++ b/core/modules/statistics/statistics.admin.inc
@@ -293,13 +294,13 @@ function statistics_settings_form() {
   $form['access']['statistics_flush_accesslog_timer'] = array(
     '#type' => 'select',
     '#title' => t('Discard access logs older than'),
-    '#default_value' => variable_get('statistics_flush_accesslog_timer', 259200),
+    '#default_value' => $config->get('access_log.flush_timer'),
...
     '#description' => t('Older access log entries (including referrer statistics) will be automatically discarded. (Requires a correctly configured <a href="@cron">cron maintenance task</a>.)', array('@cron' => url('admin/reports/status'))),

I had to look up what this setting means and what it actually is for.

It looks like something with "expire" and "entries" (and perhaps even "cron") would make some more sense.

+++ b/core/modules/statistics/config/statistics.settings.yml
@@ -0,0 +1,11 @@
+        num: '0'
...
+        num: '0'
...
+        num: '0'

In other config objects, we're using 'limit' as key name for settings like this, and it looks like these 'num' settings have the purpose (the maximum limit of items/entries to show in the block).

+++ b/core/modules/statistics/lib/Drupal/statistics/Tests/StatisticsAdminTest.php
@@ -178,8 +182,8 @@ class StatisticsAdminTest extends WebTestBase {
+    // statistics_cron will subtract the statistics.settings.accesslog.flush_timer

1) Let's add some () after "statistics_cron" while touching this comment. :)

2) We don't really have a standard for referencing keys in config objects in PHP comments yet, but so far we used config.object:key.sub-key (i.e., delimited by a colon). A dot as delimiter is definitely bogus though.

3) The new comment line exceeds 80 chars.

+++ b/core/modules/statistics/lib/Drupal/statistics/Tests/StatisticsLoggingTest.php
@@ -61,7 +63,6 @@ class StatisticsLoggingTest extends WebTestBase {
-

+++ b/core/modules/statistics/lib/Drupal/statistics/Tests/StatisticsTestBase.php
@@ -33,11 +33,10 @@ class StatisticsTestBase extends WebTestBase {
-

+++ b/core/modules/statistics/statistics.module
@@ -347,20 +351,20 @@ function statistics_block_save($delta = '', $edit = array()) {
-

Let's revert all unnecessary removals of newlines in this patch, please.

+++ b/core/modules/statistics/lib/Drupal/statistics/Tests/StatisticsTestBase.php
@@ -33,11 +33,10 @@ class StatisticsTestBase extends WebTestBase {
+    config('statistics.settings')->set('access_log.enable', 1)
+      ->set('count_content_views', 1)

The first ->set() should be on its own line :)

+++ b/core/modules/statistics/statistics.admin.inc
@@ -282,9 +282,10 @@ function statistics_access_log($aid) {
+function statistics_settings_form($form, $form_state) {

@@ -312,9 +313,20 @@ function statistics_settings_form() {
+function statistics_settings_form_submit($form, $form_state) {

&$form_state always needs to taken by reference.

+++ b/core/modules/statistics/statistics.admin.inc
@@ -293,13 +294,13 @@ function statistics_settings_form() {
     '#title' => t('Enable access log'),
...
     '#description' => t('Log each page access. Required for referrer statistics.'),

@@ -312,9 +313,20 @@ function statistics_settings_form() {
     '#title' => t('Count content views'),
...
+    '#default_value' => $config->get('count_content_views'),
     '#description' => t('Increment a counter each time content is viewed.'),

Personally, Statistics module is unfortunately still a black magic box for me.

I have absolutely no idea whether it would be appropriate, but based on the configuration form elements and their descriptions, I really wonder whether something along the lines of the following keys wouldn't be more concise...?

track.page.access = 1
track.content.view = 1

Mind you, I might be completely off here.

+++ b/core/modules/statistics/statistics.php
@@ -14,8 +14,10 @@ chdir('../../..');
 include_once DRUPAL_ROOT . '/core/includes/bootstrap.inc';
+#include_once DRUPAL_ROOT . '/core/includes/common.inc';

Looks like we can revert this now. :)

sun’s picture

@alexpott just clarified his point in #44 in IRC to me:

Statistics module only provides a single block called "popular", so I was horribly mistaken there and the suggested split of block settings makes no sense. :)

Instead, the second-level key should be block.popular, e.g.:

block:
    popular:
        top_all_limit: ...
        top_day_limit: ...
        top_recent_limit: ...
alexpott’s picture

Still think...

block:
    popular:
        top_all_display: ...
        top_day_display: ...
        top_recent_display: ...

... is better.

Since when they are set to 0 then the part of the block controlled by the configuration will not display at all. But this is just bikehedding :)... As this configuration is used to do 2 things... whether to display to information and to set the range on a db_query in statistics_title_list()

+++ b/core/modules/statistics/statistics.module
@@ -347,20 +351,20 @@ function statistics_block_save($delta = '', $edit = array()) {
 function statistics_block_view($delta = '') {
   if (user_access('access content')) {
     $content = array();
-
-    $daytop = variable_get('statistics_block_top_day_num', 0);
+    $config = config('statistics.settings');
+    $daytop = $config->get('block.top_day.num');
     if ($daytop && ($result = statistics_title_list('daycount', $daytop)) && ($node_title_list = node_title_list($result, t("Today's:")))) {
       $content['top_day'] = $node_title_list;
       $content['top_day']['#suffix'] = '<br />';
     }
kbasarab’s picture

Assigned: Unassigned » kbasarab
Status: Needs work » Needs review
FileSize
17.07 KB
20.43 KB

Lots of updates from the last few comments. As far as 50 and 51 go I kept as limit but I could go either way on it.

Status: Needs review » Needs work
Issue tags: -Configuration system, -Config novice

The last submitted patch, 1497310-52.patch, failed testing.

kbasarab’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system, +Config novice

#52: 1497310-52.patch queued for re-testing.

kbasarab’s picture

Rerunning as the last failure was failing out on LanguageUpgradePath module which this patch doesn't touch. Test was postponed originally due to HEAD issues so we'll see what happens here.

tobiasb’s picture

Status: Needs review » Needs work
+++ b/core/modules/statistics/statistics.installundefined
@@ -134,3 +120,19 @@ function statistics_schema() {
+function statistics_update_8000() {
+  update_variables_to_config('statistics.settings', array(
+    'statistics_count_content_views' => 'count_content_views',
+    'statistics_enable_access_log' => 'access_log.enabled',
+    'statistics_flush_accesslog_timer' => 'access_log.expire_timer',
+    'statistics_block_popular_top_day_limit' => 'block.popular.top_day_limit',
+    'statistics_block_popular_top_all_limit' => 'block.popular.top_all_limit',
+    'statistics_block_popular_top_recent_limit' => 'block.popular.top_recent_limit',
+  ));

This function needs the old keys ;-)

kbasarab’s picture

Status: Needs work » Needs review
FileSize
950 bytes
20.4 KB

Blah. Thanks tobiasb. Totally was in the zone of changing those and forgot I was in side the update function.

Status: Needs review » Needs work
Issue tags: -Configuration system, -Config novice

The last submitted patch, 1497310-57.patch, failed testing.

sun’s picture

Status: Needs work » Needs review

#57: 1497310-57.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Configuration system, +Config novice

The last submitted patch, 1497310-57.patch, failed testing.

aspilicious’s picture

"error: Call to undefined function Drupal\system\Tests\Upgrade\language_negotiation_include() in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/system/lib/Drupal/system/Tests/Upgrade/LanguageUpgradePathTest.php"

means the yml file is missing ;)
I'll add it...

aspilicious’s picture

Status: Needs work » Needs review
FileSize
21.85 KB
kbasarab’s picture

Thanks aspilicious. Was it just the stats yaml? Not sure how that didn't end up in my patch. The language part was throwing me.

aspilicious’s picture

yes only the .yml file :)

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me... (I only added the missing .yml file, so I'm not rtbc'ing my own patch)

sun’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/statistics/statistics.admin.inc
@@ -99,7 +99,7 @@ function statistics_top_pages() {
+  drupal_set_title(t('Top pages in the past %interval', array('%interval' => format_interval(config('statistics.settings')->get('access_log.expire_timer')))), PASS_THROUGH);

@@ -161,7 +161,7 @@ function statistics_top_visitors() {
+  drupal_set_title(t('Top visitors in the past %interval', array('%interval' => format_interval(config('statistics.settings')->get('access_log.expire_timer')))), PASS_THROUGH);

@@ -183,7 +183,7 @@ function statistics_top_visitors() {
+  drupal_set_title(t('Top referrers in the past %interval', array('%interval' => format_interval(config('statistics.settings')->get('access_log.expire_timer')))), PASS_THROUGH);

@@ -293,13 +294,13 @@ function statistics_settings_form() {
     '#title' => t('Discard access logs older than'),
-    '#default_value' => variable_get('statistics_flush_accesslog_timer', 259200),
+    '#default_value' => $config->get('access_log.expire_timer'),
     '#options' => array(0 => t('Never')) + drupal_map_assoc(array(3600, 10800, 21600, 32400, 43200, 86400, 172800, 259200, 604800, 1209600, 2419200, 4838400, 9676800), 'format_interval'),
     '#description' => t('Older access log entries (including referrer statistics) will be automatically discarded. (Requires a correctly configured <a href="@cron">cron maintenance task</a>.)', array('@cron' => url('admin/reports/status'))),

+++ b/core/modules/statistics/statistics.module
@@ -241,9 +241,10 @@ function statistics_cron() {
   // Clean up expired access logs (if applicable).
...
+  $expire_timer = config('statistics.settings')->get('access_log.expire_timer');
+  if ($expire_timer > 0) {
     db_delete('accesslog')
...
+      ->condition('timestamp', REQUEST_TIME - $expire_timer, '<')
       ->execute();

Hm. The "timer" part in "expire_timer" sounds a bit odd...

The value appears to be something like an interval, expressed in seconds; but it is not a frequency of something being invoked repetitively (in the future), but instead, a maximum time-frame in the past, for which records are kept.

I just had a look into default.settings.php, which apparently contains a similar PHP ini setting for cookie handling and garbage collection of sessions:

/**
 * Set session cookie lifetime (in seconds), i.e. the time from the session is
 * created to the cookie expires, i.e. when the browser is expected to discard
 * the cookie. The value 0 means "until the browser is closed".
 */
ini_set('session.cookie_lifetime', 2000000);

/**
 * Set session lifetime (in seconds), i.e. the time from the user's last visit
 * to the active session may be deleted by the session garbage collector. When
 * a session is deleted, authenticated users are logged out, and the contents
 * of the user's $_SESSION variable is discarded.
 */
ini_set('session.gc_maxlifetime', 200000);

Given those examples, how about

accesslog.max_lifetime

?

kbasarab’s picture

FileSize
6.71 KB
20.85 KB

That makes sense Sun. Changes attached.

alexpott’s picture

Status: Needs review » Needs work

Nearly there...

 // module, it is likely to also have 'statistics_enable_access_log' enabled,

We need to update the statistics_exit() code comments. Replace statistics_enable_access_log with statistics.settings:access_log.enabled

alexpott’s picture

+++ b/core/modules/statistics/statistics.moduleundefined
@@ -241,9 +241,10 @@ function statistics_cron() {
   // Clean up expired access logs (if applicable).

This comment should could read something like:

  // Delete access logs if applicable.
kbasarab’s picture

Status: Needs work » Needs review
FileSize
921 bytes
21.3 KB

Updated. Thanks alexpott.

kbasarab’s picture

FileSize
574 bytes
21.4 KB

Caught at same time there alexpott. I'm loading this and I'm off for the night. Will followup in morning with any other changes.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +New files

This is good to go!

Thanks to everyone for all the work!

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks all.

Automatically closed -- issue fixed for 2 weeks with no activity.