Don't run hook_modules_installed during config install

Here is a patch that fixes our problems.

I think there are a few more things to do here.

1. Add $is_syncing to scheduler_install() too and not set the key on sync.
2. Add $is_syncing to $scheduler_uninstall but also file a follow-up to remove it and use an enforced dependency in the configuration. See node.type.book for an example of this.

Had a good look at what schedule_modules_installed() is doing and I think that some of it might need to be run on config import. I think that the SchedulerManager::entityUpdate() is important. But I don't think it should run during a module install. We can add an extra step to config import to sort this out after all the config is imported. See field_config_import_steps_alter() for an example. Note we might not be able to work out if we need to do anything prior to the import so the step might have to always run.

Given #4 and the complexity here we're going to need some tests.

Thanks @chr.fritsch and @alexpott. Scheduler 2.0 is planned for release very soon, the last RC has been out in the wild, just coming up to two months, and has 2,321 downloads and I've not had to fix anything so far. Do you think this problem is important enough that it should be done for 2.0 i.e. delay the release? Or if we open a MR here, then you can use the patch from it in your imports and test it properly.

Another alternative would be to commit the simple patch in #2, to be included in release 2.0 but keep the issue open for the rest of the work.

@jonathan1055 yes this is important enough in my mind to delay 2.0 - it makes sites that use scheduler really hard to install from configuration. The behaviour of affecting config during module install and uninstall has to be very carefully considered during a config sync. It's easy to get wrong and have unexpected effects that cause data loss as you can overwrite intended change.

Running an install with this code just returns an endless log of this:

[warning] Undefined array key "finished" ConfigImporter.php:518
 [warning] Undefined array key "finished" ConfigImporter.php:518

Seems to be because the new sync step doesn't tell the context that the step finished.

Needs this line added

$context['finished'] = 1;

Hi neclimdul,
That's a coincidence because I was just looking at this right now. Can you list the steps to reproduce, right from the start, so that I can try to see this for myself.

just run the install with config import. Specifically for me drush si -y --existing-config

You can see how the context is suppose to work in the sync step in this test:
https://git.drupalcode.org/project/drupal/-/blob/10.1.x/core/modules/con...

Thanks @neclimdul yes I can replicate the problem of never-ending log message using drush site-install -y --existing-config. The change you suggested does fix this, so I have added that to the PR. Also some other minor changes to comments and coding standards.

It is interesting that I was still not able to replicate what the original problem was. Take for example the early exit from scheduler_install(). Initially at the top of the function the cron key is already changed to the config exported value (even if previously set to something else manually in the site UI). If $is_syncing is ignored and we carry on to set a new random key, this is done, but then something later resets it back to the exported config value.

If the row lightweight_cron_access_key: whatever is removed from the export/import scheduler.settings.yml file the initial value is blank (over-riding the site's existing value). Then this is changed to random, but then changed back to blank again. So we end up with the required value from the export/import file in both cases.

Anyway, even I can't replicate the problem, I still totally accept that the correct thing to do is exit early, so that will stay.

Regarding the new hook scheduler_config_import_steps_alter() this is running $scheduler_manager->entityUpdate() and avoids running $scheduler_manager->viewsUpdate(). It also runs $scheduler_manager->entityRevert() but that was never done in the original hook_modules_installed(). So I'd like to know why @alexpott added that. There could be a good reason, or it could be a mistake. It almost certainly will do nothing anyway, but its confusing to call something if never required.

Three months with no reply. I'm going to commit this as-is, and then release Scheduler 2.0.0

Thank you @chr.fritsch for the initial report and @alexpott and @neclimdul for your help.

Thanks you! Sorry if you needed additional feedback, we'd been running a version of this patch since the discussion so I expect all is well.

It was only the question in #12 about why scheduler_manager->entityRevert() was added to the process when it was not called in the original hook_modules_installed(). But I'm fairly sure it will do nothing and won't cause any harm, so I decided to commit as-is. Good to hear you've been using the patch and all is OK.

Seems that this fix still broke config import when the entity does not have a bundle.
🐛 entityRevert breaks config:import if there are entity types that do not have a bundle class Fixed
But the fix is simple enough.

@neclimdul @alexpott @chr.fritsch I decided to remove the added call to scheduler_manager->entityRevert(). I do not think it is correct to add it there, and can actually cause problems when upgrading from 8.x-1.x to 2.0.0. There is a simple fix on 🐛 entityRevert breaks config:import if there are entity types that do not have a bundle class Fixed - does this look OK to you? If so I will commit and then release 2.0.1 to prevent further sites having the same difficulties.

I have released Scheduler 2.0.1 → with the necessary fix.

Cool. Not sure why it wasn't a problem for us or why it was there in the first place but it does seem like its probably not needed. Thanks!

I think it wasn't a problem for you because you didn't have any pending entity updates, or if there were you ran them before the import.

The call should not have been there in the first place. As I said in #12 above I was dubious about it being added.

All fixed now :-)

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