Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#48 | 1790920-48.drupal8.cron-last-state.patch | 10.51 KB | japicoder |
#44 | 1790920-44.drupal8.cron-last-state.patch | 10.65 KB | alexpott |
#44 | 43-44-interdiff.txt | 1.22 KB | alexpott |
#43 | 1790920-43.drupal8.cron-last-state.patch | 10.97 KB | japicoder |
#39 | 1790920-39.drupal8.cron-last-state.patch | 10.84 KB | japicoder |
Comments
Comment #1
alexpottPatch does the conversion and provides hook_update_N's for upgrade path.
No upgrade path tests (yet).
Comment #2
Lars Toomre CreditAttribution: Lars Toomre commentedI 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?
Comment #3
alexpottWhy not?
Comment #5
sunHm. 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.
Comment #6
alexpott#3: 1790920-3.drupal8.cron-last-state.patch queued for re-testing.
Comment #7
alexpottThe 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.
Comment #8
podarok#3 bot is happy and me too
yet one review to RTBC
Comment #9
Lars Toomre CreditAttribution: Lars Toomre commented@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.
Comment #10
podarok#9 agree with that
Comment #11
Lars Toomre CreditAttribution: Lars Toomre commentedTaking 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.
Comment #12
alexpottYep 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.
Comment #13
Lars Toomre CreditAttribution: Lars Toomre commentedI like everything about this patch now. RTBC!
Comment #14
Lars Toomre CreditAttribution: Lars Toomre commentedWhoops... I did not test an upgrade from D7 to D8. Someone else will need to do that.
Comment #15
sun1) 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.
Comment #16
alexpott1. 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
Comment #17
LinL CreditAttribution: LinL commentedRe-roll
Comment #18
alexpottthis could be
$expires = state()->get('mymodule.cron_last_run') ?: REQUEST_TIME;
Comment #19
alexpottthis should be
$cron_last = state()->get('system.cron_last') ?: NULL;
Comment #20
LinL CreditAttribution: LinL commentedComment #21
BerdirThis should use the new helper function and extend the tests which are now commited.
Comment #22
alexpottDone and added tests too...
Comment #23
BerdirLooks great.
Comment #24
sunExcellent work, folks. This is indeed ready to fly.
Comment #25
catchNo longer applies, quick re-roll?
Comment #26
gddHere it is. A couple lines moved in node.install is all.
Comment #27
BerdirRTBC if the tests pass.
Comment #29
alexpottBumped node_update_N... back to rtbc :)
Comment #30
catchSorry folks this no longer applies again.
Comment #31
japicoder CreditAttribution: japicoder commentedReroll and ready for RTBC :)
Comment #32
sunThe function supports multiple conversion parameters, we don't have to invoke it twice. ;)
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.
Comment #33
japicoder CreditAttribution: japicoder commentedApplied your suggestions, sun. Also, updated the patch against the last core version.
Comment #34
Berdirarray 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.
Can we use a different value here? To make sure we convert the correct thing.
Comment #35
japicoder CreditAttribution: japicoder commentedYou are right, I wasn't sure how to format that sentence, with your example it's clear now.
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.
Comment #36
alexpottThe value that is testing is set up...
In the code above...
So change it in both places to 1304208001 and then node_cron_last will be different to cron_last...
Comment #37
BerdirThe 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.
Comment #38
BerdirCrosspost.
Comment #39
japicoder CreditAttribution: japicoder commented@alexpott, @Berdir, got it now, thank you for the review :)
Added the last changes and ready to RTBC
Comment #41
japicoder CreditAttribution: japicoder commentedUps, no ready yet, working on the failed test
Comment #42
japicoder CreditAttribution: japicoder commentedComment #43
japicoder CreditAttribution: japicoder commentedCorrection for the test. Everything must be ok now
Comment #44
alexpottThis is failing because the patch that converts the install_task and install_time has been reverted...
This should work...
Comment #45
japicoder CreditAttribution: japicoder commentedYep, I figured that is the problem comparing changes between #33 and #39 ;)
Comment #46
japicoder CreditAttribution: japicoder commentedFor me, it sees good now
Comment #47
BerdirSorry, didn't notice this before.
This should only be a single sentence that contains both variables. Or be rewritten to not mention them explicitly, in case we add more.
Comment #48
japicoder CreditAttribution: japicoder commentedMade in a single sentence. Hope it's ok now :)
Comment #49
BerdirYes, 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.
Comment #50
japicoder CreditAttribution: japicoder commentedI was thinking exactly the same as you, that is the reason to move to a separate update :)
Comment #51
japicoder CreditAttribution: japicoder commentedComment #52
catchLooks good. Committed/pushed to 8.x.
Comment #53.0
(not verified) CreditAttribution: commentedUpdated issue summary.