Closed (fixed)
Project:
Scheduler
Version:
2.x-dev
Component:
Code
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
19 Sep 2023 at 08:55 UTC
Updated:
12 Oct 2023 at 14:14 UTC
Jump to comment: Most recent
Problem/Motivation
Scheduler provides a cron mecanism to only do scheduler tasks.
It comes with a cron key in configuration. This key is generated on site install.
On a project using exported configuration, when reimporting configuration it is ok.
But before reimporting configuration or if configuration is exported differently than core config folder, like with Features or config_profile modules, the configuration is marked has changed at each reinstallation and/or for each developers.
Proposed resolution
Move lightweight_cron_access_key config key into a state. [yes]
OR
Use \Drupal::state()->get('system.cron_key') directly. [no]
Remaining tasks
- Agree on a solution
- Implement it
Issue fork scheduler-3388199
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
jonathan1055 commentedThanks for raising this. I was not aware that it could cause a problem. You say "the configuration is marked has changed at each reinstallation and/or for each developers." Can you explain a bit more why this is a problem?
However, I do not see there is any loss of security in using the core key in Scheduler. In additon to removing the code in install, it would also need to be removed from the form. The cron test would also need to be updated.
But if there is a good reason for it, and no disadvantages, then that is all quite easy to do.
Comment #3
grimreaperOf course :)
I would say more an inconvenience than a problem.
On the project I currently am, we export configuration in a custom profile with config_profile, because then this custom profile is packaged and retrieved with Composer on other client's projects as a vendor.
So we have app/profiles/custom/my_project/config/install/scheduler.settings.yml
And so when we install the website, we install it from the configuration in the install profile not from core sync directory.
So we constantly have diff on config exports, like:
Comment #4
jonathan1055 commentedThanks for explaining that. Yes I see. Do you think there is any detrimental effect to simply stop using Scheduler's own access key and just use the core one? A problem could arise that existing sites would have to change their scheduler cron url, which would be annoying, as that would not be automatable via a hook_update. Maybe it could be coded such that if config lightweight_cron_access_key has a value then that is accepted when checking the url access, but if that fails, then also allow the core access key. Only fail if neither are correct. New installations would simply use the core key, but existing sites could use either. You would erase your scheduler key from config and it would remain blank in all exports and imports.
Comment #5
grimreaperTo be honest, I never used the lightweight cron from Scheduler :), always core's one with a frequency of 5 or 10 minutes.
Ok, to not disrupt existing websites which use it with the URL, let's juste move the config into a state and this could be switched to core key in a major release.
That way people will be more attentive to changes when doing a major update. And we do not have to maintain a logic for two systes active in parallel.
I will propose a MR, also currently keeping the form to change the key.
Comment #7
grimreaperMR created.
Thanks for the review.
If you want to switch directly to core's key, I think at least a change record is needed.
Comment #8
jonathan1055 commentedThis looks good, thank you for the full works, including the hook update. I will do some local testing, as we do not currently have any tests to check hook_update implementations. Do you have any experience of that kind of test, or do you know of examples in Contrib which do that?
Comment #9
grimreaperI once did update tests on core and it was a long process... With the need to add under version control a database dump dedicated to the update test.
In contrib, I don't know, first modules I would check for examples would be Webform, Metatag, pathauto, etc. the top used modules.
Comment #10
jonathan1055 commentedJust a couple of minor changes. Fully tested locally, and I don't think we need to go to the effort of creating a phpunit test for this.
Comment #11
jonathan1055 commentedDo you know what would happen in the scenario where a full site export has been done, using Scheduler as-is now, and then it is used as import after Scheduler has been updated with this change? I would guess that this could be quite common in development and test stages.
Comment #12
grimreaperWorst case scenario: Config Inspector will say that the simple config contains an unknown entry.
I would say if "the correct" (I don't think there is another one) process of update with a full site export is done, this should not happen.
Because on a dev environment to do the update, you should be in a clean config state, with everything exported, then (giving main steps, some projects may require specific additional steps):
- composer update
- drush updb
- drush cex
- commit
- push
and when deploying, either other devs pulling the commit or other environments:
- pull
- composer install
- drush updb
- drush cim
And in the case you reimport a backup:
- drush sql:drop
- drush sql:cli < your/backup.sql
- drush updb
- drush cim
No problem here too, because the hook_update will be triggered.
Comment #13
jonathan1055 commentedThank you for thinking this through. Yes I agree it will be ok. I have even tested updating to the new Scheduler code, then importing from old config. We get the "undefined, missing schema" message, but the value is still in config. So eventually when db-update is run, the value is moved to the state (and hence not lost). Then the config is cleared, and the status report warning no longer shows.
One minor change I am going to push is move the
container->get('state')into the test that needs it. The property does not need to be available for all tests, becuase it is only used in that one LightweightCron test.Comment #15
jonathan1055 commentedCommitted. Thanks @Grimreaper for doing the work on this.
Comment #16
grimreaperThanks @jonathan1055 for the merge :)