drush updb does not clear caches if there are no post updates

[Berdir]

Describe the bug

It looks like drush updb was optimized to only run a cache clear between updates and post updates if there are post updates and were updates.

If there are only updates that doesn't seem to happen.

That is becoming a more serious issue due to the Update cache backend that is now in core, that ignores cache writes, and for example the cached config storage only writes to cache and doesn't delete.

To Reproduce
Check value of config with drush cget. Run an update that changes that config value. Check again with drush cget.

Expected behavior
New value is visible.

Actual behavior
The old value is still there.

Workaround
drush cr

System Configuration

Q	A
Drush version?	10.x/9.x
Drupal version?	8.x
PHP version	7.x,5.x
OS?	Mac/Linux/Windows

Additional information
Add any other context about the problem here.

[Berdir]

I think we should add an else to if ($options['post-updates']) { that adds a cacheRebuild() operation if there were upates.

[Berdir]

It is possible that this only affects drush 9, #4103 might have fixed this too, I did check the code in drush 10 but didn't look in the right place, would have assumed that cache clear to happen in the same way as the other one that we do between.

[alexpott]

To mimic core updates via the UI there must be cache clear when running updatedb - even if there are no updates. Core does a flush at the end of the batch and prior to running post updates (if there are any).

[weitzman]

We've been back and forth on this :). To make matters more complex, we now have the deploy command which takes this into account and already does an unconditional cache rebuild after updatedb. See https://github.com/drush-ops/drush/blob/master/src/Commands/core/DeployCommands.php

[davereid-pfg]

I can confirm on the latest Drush 10 that with no updates to run, there is no cache rebuild like what happens with update.php.

[bkosborne]

Actually it looks like the new deploy command has the same problem:

public function deploy()
    {
        $self = $this->siteAliasManager()->getSelf();
        $redispatchOptions = Drush::redispatchOptions();
        $manager = $this->processManager();

        $this->logger()->notice("Database updates start.");
        $options = ['no-cache-clear' => true];
        $process = $manager->drush($self, 'updatedb', [], $options + $redispatchOptions);
        $process->mustRun($process->showRealtime());

        $this->cacheRebuild($manager, $self, $redispatchOptions);

        $this->logger()->success("Config import start.");
        $process = $manager->drush($self, 'config:import', [], $redispatchOptions);
        $process->mustRun($process->showRealtime());

        $this->cacheRebuild($manager, $self, $redispatchOptions);

        $this->logger()->success("Deploy hook start.");
        $process = $manager->drush($self, 'deploy:hook', [], $redispatchOptions);
        $process->mustRun($process->showRealtime());
    }

Yes, there's an explicit cache rebuild after updatedb, but that's not the problem here. The problem is that there's no explicit cache rebuild happening within updatedb before the post_update hooks are run if there were no hook_update_N calls.

I agree this should mirror core as much as possible.

[alexpott]

@bkosborne fwiw it is kind of moot because updatedb runs prior to post updates without caches - i.e. they are all Null backends. If there are post updates then it is likely that the container will be rebuilt (because the cache key will change) and things will be rebuilt. However I agree that the whole cache clear thing here should be removed for the following reasons:

• The --no-post-updates option introduces the possibility of not running them in order to do a config-import between hook_update_N and hook_post_update_NAME - this was done because people wanted to use hook_post_update_NAME to do what should be done in drush's deploy hooks. Now we have deploy hooks we should remove this.
• The --no-cache-clear option should be removed as a result of removing --no-post-updates.

[weitzman]

Thanks. Makes sense. I just opened a PR for that #5531