Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

Status: Active » Needs review
FileSize
9.75 KB

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

No upgrade path tests (yet).

Lars Toomre’s picture

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?

alexpott’s picture

Status: Needs review » Needs work

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

sun’s picture

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.

alexpott’s picture

Status: Needs work » Needs review
alexpott’s picture

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

podarok’s picture

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

Lars Toomre’s picture

@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.

podarok’s picture

#9 agree with that

Lars Toomre’s picture

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.

alexpott’s picture

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.

Lars Toomre’s picture

Status: Needs review » Reviewed & tested by the community

I like everything about this patch now. RTBC!

Lars Toomre’s picture

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.

sun’s picture

+++ 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.

alexpott’s picture

LinL’s picture

alexpott’s picture

+++ 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;

alexpott’s picture

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;

LinL’s picture

Status: Needs work » Needs review
FileSize
9.29 KB
Berdir’s picture

Status: Needs review » Needs work

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

alexpott’s picture

Status: Needs work » Needs review
FileSize
10.56 KB
2.86 KB

Done and added tests too...

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Looks great.

sun’s picture

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

catch’s picture

Status: Reviewed & tested by the community » Needs work

No longer applies, quick re-roll?

gdd’s picture

Status: Needs work » Needs review
FileSize
10.52 KB

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

Berdir’s picture

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.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
10.52 KB
346 bytes

Bumped node_update_N... back to rtbc :)

catch’s picture

Status: Reviewed & tested by the community » Needs work

Sorry folks this no longer applies again.

japicoder’s picture

Status: Needs work » Needs review
FileSize
10.7 KB

Reroll and ready for RTBC :)

sun’s picture

+++ 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.

japicoder’s picture

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

Berdir’s picture

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.

japicoder’s picture

Status: Needs work » Needs review
FileSize
10.84 KB

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.

alexpott’s picture

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...

Berdir’s picture

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.

Berdir’s picture

Status: Needs review » Needs work

Crosspost.

japicoder’s picture

Assigned: Unassigned » japicoder
Status: Needs work » Needs review
FileSize
10.84 KB

@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.

japicoder’s picture

Status: Needs work » Needs review

Ups, no ready yet, working on the failed test

japicoder’s picture

Status: Needs review » Needs work
japicoder’s picture

Status: Needs work » Needs review
FileSize
10.97 KB

Correction for the test. Everything must be ok now

alexpott’s picture

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

This should work...

japicoder’s picture

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

japicoder’s picture

For me, it sees good now

Berdir’s picture

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.

japicoder’s picture

Status: Needs work » Needs review
FileSize
10.51 KB

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

Berdir’s picture

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.

japicoder’s picture

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 :)

japicoder’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

Status: Reviewed & tested by the community » Fixed

Looks good. Committed/pushed to 8.x.

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.