This path will move the variables:

  • cron_last
  • node_cron_last
  • common_test_cron

to state system.

Tasks

  • Move variable_get/_set/_delete to state()->get/set/delete
  • Provide upgrade path and test
Files: 
CommentFileSizeAuthor
#48 1790920-48.drupal8.cron-last-state.patch10.51 KBjapicoder
PASSED: [[SimpleTest]]: [MySQL] 47,599 pass(es).
[ View ]
#44 1790920-44.drupal8.cron-last-state.patch10.65 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 47,613 pass(es).
[ View ]
#44 43-44-interdiff.txt1.22 KBalexpott
#43 1790920-43.drupal8.cron-last-state.patch10.97 KBjapicoder
FAILED: [[SimpleTest]]: [MySQL] 47,568 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#39 1790920-39.drupal8.cron-last-state.patch10.84 KBjapicoder
FAILED: [[SimpleTest]]: [MySQL] 47,604 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#35 1790920-35.drupal8.cron-last-state.patch10.84 KBjapicoder
FAILED: [[SimpleTest]]: [MySQL] 47,596 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#33 1790920-33.drupal8.cron-last-state.patch10.68 KBjapicoder
PASSED: [[SimpleTest]]: [MySQL] 47,567 pass(es).
[ View ]
#31 1790920-31.drupal8.cron-last-state.patch10.7 KBjapicoder
PASSED: [[SimpleTest]]: [MySQL] 46,843 pass(es).
[ View ]
#29 26-29-interdiff.txt346 bytesalexpott
#29 1790920-29.drupal8.cron-last-state.patch10.52 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 46,454 pass(es).
[ View ]
#26 1790920-26.drupal8.cron-last-state.patch10.52 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/node/node.install.
[ View ]
#22 20-22-interdiff.txt2.86 KBalexpott
#22 1790920-22.drupal8.cron-last-state.patch10.56 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 46,322 pass(es).
[ View ]
#20 1790920-20.drupal8.cron-last-state.patch9.29 KBLinL
PASSED: [[SimpleTest]]: [MySQL] 46,247 pass(es).
[ View ]
#17 1790920-17.drupal8.cron-last-state.patch9.35 KBLinL
PASSED: [[SimpleTest]]: [MySQL] 46,234 pass(es).
[ View ]
#16 1790920-16.drupal8.cron-last-state.patch10.13 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 41,918 pass(es).
[ View ]
#12 1790920-12.drupal8.cron-last-state.patch10.16 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 41,898 pass(es).
[ View ]
#7 1790920-3.drupal8.cron-last-state.patch | Drupal Quality Assurance.png68.13 KBalexpott
#3 1790920-3.drupal8.cron-last-state.patch9.86 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 41,438 pass(es).
[ View ]
#3 1-3-interdiff.txt1.9 KBalexpott
#1 1790920-1.drupal8.cron-last-state.patch9.75 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 41,442 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new9.75 KB
PASSED: [[SimpleTest]]: [MySQL] 41,442 pass(es).
[ View ]

Patch does the conversion and provides hook_update_N's for upgrade path.

No upgrade path tests (yet).

I noticed that several of the assertions touched by this patch still have t() wrapped around the assertion text. Does it make sense to correct those errors in this patch as well?

StatusFileSize
new1.9 KB
new9.86 KB
PASSED: [[SimpleTest]]: [MySQL] 41,438 pass(es).
[ View ]

Why not?

Status:Needs review» Needs work

The last submitted patch, 1790920-3.drupal8.cron-last-state.patch, failed testing.

Issue tags:+State system

Hm. I'd really like to have a discussion about new + proper key names in the state store first. :-/

Didn't create an issue yet, but I already outlined a couple of concerns in comments in the original issue.

Status:Needs work» Needs review

The single failure on the first test of #3 was in an unrelated test and it passed locally. Image from qa.drupal.org in case we have another random test failure.
Single test failure

#3 bot is happy and me too
yet one review to RTBC

@sun re: #5 - Cannot the possible rename of variables be left to a follow-up issue? The current patch looks like it converts the three existing variables correctly to the new state() system.

#9 agree with that

Taking another look at this patch, I think that each of the variables should be deleted after their values have been set in the state() ssytem. There is no reason to leave those entries in $conf array after the update.

StatusFileSize
new10.16 KB
PASSED: [[SimpleTest]]: [MySQL] 41,898 pass(es).
[ View ]

Yep Lars you're right - we need to clean up :)

Added update_variable_del() calls to both hook_update_N functions and rerolled the patch to apply cleanly. Interdiff is confusing due to reroll.

Status:Needs review» Reviewed & tested by the community

I like everything about this patch now. RTBC!

Status:Reviewed & tested by the community» Needs review

Whoops... I did not test an upgrade from D7 to D8. Someone else will need to do that.

+++ b/core/includes/common.inc
@@ -5044,7 +5044,7 @@ function drupal_cron_run() {
+    state()->set('cron_last', REQUEST_TIME);
+++ b/core/modules/node/node.install
@@ -482,9 +482,11 @@ function node_uninstall() {
+  state()->delete('node_cron_last');
+++ b/core/modules/system/system.api.php
@@ -128,11 +128,14 @@ function hook_admin_paths_alter(&$paths) {
+  state()->set('mymodule_cron_last_run', REQUEST_TIME);
+++ b/core/modules/system/system.install
@@ -272,9 +272,9 @@ function system_requirements($phase) {
+      $cron_last = state()->get('install_time', 0);
+++ b/core/modules/system/tests/modules/common_test_cron_helper/common_test_cron_helper.module
@@ -13,5 +13,5 @@
+  state()->set('common_test_cron', 'success');

1) At minimum, we should properly namespace the state item key names; i.e.:

system.cron_last
system.install_time
node.cron_last
common_test.cron

2) There's no upgrade path for the install_time variable.

StatusFileSize
new10.13 KB
PASSED: [[SimpleTest]]: [MySQL] 41,918 pass(es).
[ View ]

1. Done
2. Didn't mean to convert install_time - that's handled by #1798732: Convert install_task, install_time and install_current_batch to use the state system

StatusFileSize
new9.35 KB
PASSED: [[SimpleTest]]: [MySQL] 46,234 pass(es).
[ View ]

Re-roll

+++ b/core/modules/system/system.api.phpundefined
@@ -128,11 +128,14 @@ function hook_admin_paths_alter(&$paths) {
+  $expires = state()->get('mymodule.cron_last_run');
+  if (!$expires) {
+    $expires = REQUEST_TIME;
+  }

this could be
$expires = state()->get('mymodule.cron_last_run') ?: REQUEST_TIME;

Status:Needs review» Needs work

+++ b/core/modules/system/system.moduleundefined
@@ -3679,7 +3679,7 @@ function system_run_automated_cron() {
-    $cron_last = variable_get('cron_last', NULL);
+    $cron_last = state()->get('system.cron_last');
     if (!isset($cron_last) || (REQUEST_TIME - $cron_last > $threshold)) {
       drupal_cron_run();
     }

this should be $cron_last = state()->get('system.cron_last') ?: NULL;

Status:Needs work» Needs review
StatusFileSize
new9.29 KB
PASSED: [[SimpleTest]]: [MySQL] 46,247 pass(es).
[ View ]

Status:Needs review» Needs work

This should use the new helper function and extend the tests which are now commited.

Status:Needs work» Needs review
StatusFileSize
new10.56 KB
PASSED: [[SimpleTest]]: [MySQL] 46,322 pass(es).
[ View ]
new2.86 KB

Done and added tests too...

Status:Needs review» Reviewed & tested by the community

Looks great.

Excellent work, folks. This is indeed ready to fly.

Status:Reviewed & tested by the community» Needs work

No longer applies, quick re-roll?

Status:Needs work» Needs review
StatusFileSize
new10.52 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/node/node.install.
[ View ]

Here it is. A couple lines moved in node.install is all.

Status:Needs review» Reviewed & tested by the community

RTBC if the tests pass.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 1790920-26.drupal8.cron-last-state.patch, failed testing.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new10.52 KB
PASSED: [[SimpleTest]]: [MySQL] 46,454 pass(es).
[ View ]
new346 bytes

Bumped node_update_N... back to rtbc :)

Status:Reviewed & tested by the community» Needs work

Sorry folks this no longer applies again.

Status:Needs work» Needs review
StatusFileSize
new10.7 KB
PASSED: [[SimpleTest]]: [MySQL] 46,843 pass(es).
[ View ]

Reroll and ready for RTBC :)

+++ b/core/modules/node/node.install
@@ -721,11 +723,13 @@ function node_update_8009() {
function node_update_8010() {
   update_variables_to_state(array('node_access_needs_rebuild' => 'node.node_access_needs_rebuild'));
+  update_variables_to_state(array('node_cron_last' =>'node.cron_last'));
}

The function supports multiple conversion parameters, we don't have to invoke it twice. ;)

+++ b/core/modules/system/system.install
@@ -2204,6 +2204,7 @@ function system_update_8032() {
  * Converts active_menus_default variable to config.
+ * Moves cron last run time from variable to state.
@@ -2211,6 +2212,7 @@ function system_update_8033() {
+  update_variables_to_state(array('cron_last' => 'system.cron_last'));

It is often OK to merge state updates into existing updates (if their values will be regenerated automatically).

However, this state update here is mixed into a completely different update.

StatusFileSize
new10.68 KB
PASSED: [[SimpleTest]]: [MySQL] 47,567 pass(es).
[ View ]

Applied your suggestions, sun. Also, updated the patch against the last core version.

Status:Needs review» Needs work

+++ b/core/modules/node/node.installundefined
@@ -721,11 +723,16 @@ function node_update_8009() {
function node_update_8010() {
-  update_variables_to_state(array('node_access_needs_rebuild' => 'node.node_access_needs_rebuild'));
+  update_variables_to_state(
+    array(
+      'node_access_needs_rebuild' => 'node.node_access_needs_rebuild',
+      'node_cron_last' =>'node.cron_last',
+    ));

array should start on the previous line and it should be indented two spaces less. See http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul... for an example (this is config and not state, but it's just an additional argument that differs.

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/StateSystemUpgradePathTest.phpundefined
@@ -39,6 +39,14 @@ public function testSystemVariableUpgrade() {
+    $expected_state['node.cron_last'] = array(
+      'value' => 1304208000,
+      'variable_name' => 'node_cron_last',
+    );
+    $expected_state['system.cron_last'] = array(
+      'value' => 1304208000,
+      'variable_name' => 'cron_last',

Can we use a different value here? To make sure we convert the correct thing.

Status:Needs work» Needs review
StatusFileSize
new10.84 KB
FAILED: [[SimpleTest]]: [MySQL] 47,596 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

array should start on the previous line and it should be indented two spaces less

You are right, I wasn't sure how to format that sentence, with your example it's clear now.

Can we use a different value here? To make sure we convert the correct thing.

I'm not sure what value to use here, it depends on the system on where it's running. Probably we can use the variable table value if it wasn't removed yet.

I upload a new patch with the node.install change.

Status:Needs review» Needs work

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/StateSystemUpgradePathTest.phpundefined
@@ -39,6 +39,14 @@ public function testSystemVariableUpgrade() {
+    $expected_state['node.cron_last'] = array(
+      'value' => 1304208000,
+      'variable_name' => 'node_cron_last',
+    );

The value that is testing is set up...

+++ b/core/modules/system/tests/upgrade/drupal-7.state.system.database.phpundefined
@@ -23,3 +23,11 @@
+db_merge('variable')
+  ->key(array('name' => 'node_cron_last'))
+  ->fields(array('value' => serialize(1304208000)))
+  ->execute();

In the code above...

So change it in both places to 1304208001 and then node_cron_last will be different to cron_last...

Status:Needs work» Needs review

The actual value is totally irrelevant for the test. The test just needs to confirm that we convert value X correctly from a variable to a state value. You can use a random number or make something up, like the time this issue was created :)

The problem if it's the same is that we could theoretically mess up the upgrade path and use cron_last for both node.cron_last and system.cron_last. Then the upgrade test would pass because we don't know the difference.

Status:Needs review» Needs work

Crosspost.

Assigned:Unassigned» japicoder
Status:Needs work» Needs review
StatusFileSize
new10.84 KB
FAILED: [[SimpleTest]]: [MySQL] 47,604 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

@alexpott, @Berdir, got it now, thank you for the review :)

Added the last changes and ready to RTBC

Status:Needs review» Needs work

The last submitted patch, 1790920-39.drupal8.cron-last-state.patch, failed testing.

Status:Needs work» Needs review

Ups, no ready yet, working on the failed test

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new10.97 KB
FAILED: [[SimpleTest]]: [MySQL] 47,568 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Correction for the test. Everything must be ok now

StatusFileSize
new1.22 KB
new10.65 KB
PASSED: [[SimpleTest]]: [MySQL] 47,613 pass(es).
[ View ]

This is failing because the patch that converts the install_task and install_time has been reverted...

This should work...

Yep, I figured that is the problem comparing changes between #33 and #39 ;)

For me, it sees good now

Status:Needs review» Needs work

Sorry, didn't notice this before.

+++ b/core/modules/node/node.installundefined
@@ -721,11 +723,15 @@ function node_update_8009() {
  * Moves node_access_needs_rebuild from variable to state.
+ * Moves node cron last run time from variable to state.

This should only be a single sentence that contains both variables. Or be rewritten to not mention them explicitly, in case we add more.

Status:Needs work» Needs review
StatusFileSize
new10.51 KB
PASSED: [[SimpleTest]]: [MySQL] 47,599 pass(es).
[ View ]

Made in a single sentence. Hope it's ok now :)

Status:Needs review» Reviewed & tested by the community

Yes, I think it is. It's actually a separate update method now, which wasn't what I meant but this works too, those two settings aren't really related anyway so is probably actually better.

Status:Reviewed & tested by the community» Needs review

I was thinking exactly the same as you, that is the reason to move to a separate update :)

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Fixed

Looks good. Committed/pushed to 8.x.

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

Issue summary:View changes

Updated issue summary.