This is a sub-task of http://drupal.org/node/1775842 Convert all variables to state and/or config systems.

Files: 
CommentFileSizeAuthor
#13 8-9-interdiff.txt353 bytesalexpott
#13 update_last_check_last_emal_cmi-1810880-9.patch8.32 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 46,269 pass(es).
[ View ]
#12 update_last_check_las_emal_cmi-1810880-8.patch8.36 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 46,283 pass(es).
[ View ]
#8 update_last_check_last_email_notification_cmi-1810880-7.patch8.35 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 46,268 pass(es).
[ View ]
#6 update_last_check_cmi-1810880-6.patch4.4 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 42,594 pass(es).
[ View ]
#6 interdiff.txt1.86 KBAlbert Volkman
#4 update_last_check_cmi-1810880-4.patch3.45 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 42,329 pass(es).
[ View ]
#4 interdiff.txt3.91 KBAlbert Volkman
#1 1810880-update-last-check-2.patch3.44 KBbarbun
PASSED: [[SimpleTest]]: [MySQL] 42,175 pass(es).
[ View ]

Comments

StatusFileSize
new3.44 KB
PASSED: [[SimpleTest]]: [MySQL] 42,175 pass(es).
[ View ]

And here's the small patch.

Status:Needs review» Needs work

Looks good, but shouldn't this be state? From the state() function's comment:

Use this to store machine-generated data, local to a specific environment
that does not need deploying and does not need human editing; for example,
the last time cron was run. Data which needs to be edited by humans and
needs to be the same across development, production, etc. environments
(for example, the system maintenance message) should use config() instead.

Issue tags:+State system

I agree. Tagging.

Status:Needs work» Needs review
StatusFileSize
new3.91 KB
new3.45 KB
PASSED: [[SimpleTest]]: [MySQL] 42,329 pass(es).
[ View ]

Converting to state system.

I think we also should convert variable 'update_last_email_notification' to the state() system as well in this issue. Such a change would clean up all variable_get()'s in the Update module.

StatusFileSize
new1.86 KB
new4.4 KB
PASSED: [[SimpleTest]]: [MySQL] 42,594 pass(es).
[ View ]

Good call. I've added it to the meta issue, but I don't believe I can update the title of this issue.

Status:Needs review» Needs work

+++ b/core/modules/update/update.installundefined
@@ -162,3 +160,17 @@ function update_update_8000() {
+function update_update_8001() {
+  $variable = 'update_last_check';
+
+  if ($value = update_variable_get($variable, FALSE)) {
+    state()->set('update.last_check', $value);
+  }
+  update_variable_del($variable);
+}

This should be migrating update_last_email_notification to update.last_email_notification as well. In fact it'd be great if this could be bring the generalised method to update variables to state from #1798732: Convert install_task, install_time and install_current_batch to use the state system into this patch as that one is postponed.

Status:Needs work» Needs review
StatusFileSize
new8.35 KB
PASSED: [[SimpleTest]]: [MySQL] 46,268 pass(es).
[ View ]

Please review

Title:Convert update_last_check variable to CMI systemConvert update_last_check and update_last_email_notification variables to CMI system

Updating title

Title:Convert update_last_check and update_last_email_notification variables to CMI systemConvert update_last_check variable to CMI system

+++ b/core/includes/update.incundefined
@@ -1237,6 +1237,33 @@ function update_variables_to_config($config_name, array $variable_map) {
+  // Delete the migrated variables.
+  db_delete('variable')->condition('name', array_keys($variable_map), 'IN')->execute();

The IN does not need to be specified explicitly, this is implicit if an array is passed in.

Also, this should be changed to use multiple lines:

<?php
db_delete
()
  ->
condition()
  ->
delete();
?>

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/StateSystemUpgradePathTest.phpundefined
@@ -0,0 +1,53 @@
+  public function testSystemVariableUpgrade() {
+    $this->assertTrue($this->performUpgrade(), 'The upgrade was completed successfully.');
+
+      $expected_state = array();
+
+    $expected_state['update.last_check'] = array(

Wrong indendation of $expected_state.

+++ b/core/modules/system/tests/upgrade/drupal-7.state.system.database.phpundefined
@@ -0,0 +1,20 @@
+// Update system settings to known values.
+db_merge('variable')
+  ->key(array('name' => 'update_last_check'))->fields(array('value' => serialize(1304208000)))
+  ->execute();
+db_merge('variable')
+  ->key(array('name' => 'update_last_email_notification'))->fields(array('value' => serialize(1304208000)))
+  ->execute();
+// Add non-default system settings.

fields() should be on a separate line as well I think.

Also, there doesn't seem to be anything below the second comment?

+++ b/core/modules/update/update.installundefined
@@ -162,3 +162,16 @@ function update_update_8000() {
+/**
+ * Convert update_last_check, last_email_notification variables to state api value.
+ *
+ * @ingroup config_upgrade
+ */
+function update_update_8001() {
+  $variable_map = array(

Does this need to be Converts or not? also not sure about "state api value", maybe just "state API"? or "state system"? What do we use elsewhere?

Looks good to me otherwise.

Crosspost.

Title:Convert update_last_check variable to CMI systemConvert update_last_check and update_last_email_notification variables to CMI system
StatusFileSize
new8.36 KB
PASSED: [[SimpleTest]]: [MySQL] 46,283 pass(es).
[ View ]

Thanks for the review Berdir, Please find the patch with review fixes.

StatusFileSize
new8.32 KB
PASSED: [[SimpleTest]]: [MySQL] 46,269 pass(es).
[ View ]
new353 bytes

Removed bogus comment.

Status:Needs review» Reviewed & tested by the community

Looks good to me. Let's get this in, then the other state conversion issues can use the helper function and tests added here.

Status:Reviewed & tested by the community» Fixed

Great work! That update helper is gonna come in very handy.

Committed and pushed to 8.x.

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