Support from Acquia helps fund testing for Drupal Acquia logo

Comments

barbun’s picture

And here's the small patch.

rbayliss’s picture

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.

Albert Volkman’s picture

Issue tags: +State system

I agree. Tagging.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
3.91 KB
3.45 KB

Converting to state system.

Lars Toomre’s picture

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.

Albert Volkman’s picture

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

alexpott’s picture

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.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
8.35 KB

Please review

alexpott’s picture

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

Updating title

Berdir’s picture

Title: Convert update_last_check and update_last_email_notification variables to CMI system » Convert 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:

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.

Berdir’s picture

Crosspost.

vijaycs85’s picture

Title: Convert update_last_check variable to CMI system » Convert update_last_check and update_last_email_notification variables to CMI system
FileSize
8.36 KB

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

alexpott’s picture

Removed bogus comment.

Berdir’s picture

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.

webchick’s picture

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.