Drupal update task misses post_update hooks if modules are installed from config with weight different than 0

[marcoscano]

Our drupal:update task relies on the following order of steps when building the site:

• drush updatedb --no-post-updates
• drush config:import
• drush updatedb --no-cache-clear
• drush cache:rebuild

However, there is a scenario where this set of steps fails:

• Create a hook_post_update_NAME() implementation on a module that is already installed
• Enable a module by adding it to core.extension.yml with a weight higher than 0 (for example add ban: 200 to the bottom of the list)
• Run task drupal:update

Expected: Your update hook is executed
Actual: your update hook is not executed

In my testing, \Drupal\Core\Update\UpdateRegistry::onConfigSave() enters the if condition on line 348 :

• Once per new extension being installed in core.extension
  • After this execution the drush command prints Synchronized extensions: [name-of-the-module]
  • This makes Drupal register as "non-pending" all post_update hooks in the newly-installed module, which is expected behavior
• And then another time to "update" the core.extension config object
  • After this execution the drush command prints Syncronized configuration: update core.extension
  • This makes Drupal register as "non-pending" all post_update hooks in all previously-enabled modules (which is wrong).

As a result, it's impossible to deploy a release that includes:

• A new module providing functionality that
  • Needs to have a weight different than 0
  • Needs to update content in the same deployment

This may be a bug in core, but by the discussions in:

• https://www.drupal.org/project/drupal/issues/2762235#comment-12117319 (and subsequent comments)
• https://www.drupal.org/project/drupal/issues/2901418#comment-13874930 (and subsequent comments)
• https://www.drupal.org/project/drupal/issues/3206497

I have the feeling that the consensus in the community may be gravitating towards using Drush's deploy command to build the site, and always have post_update_NAME() immediately follow update_N() hooks, such as:

• drush updatedb (which runs sequentially update_N() and post_update_NAME() )
• drush config:import
• drush deploy:hook

So I am not confident it would be realistic to try to chase and fix this issue in core. Maybe the easiest could perhaps be to switch to the deploy:hook command that is already in Drush by now? Any other ideas?

[e0ipso]

I agree that we should gravitate towards deploy:hook. That was a very nice sleuthing @marcoscano!

[marcoscano]

@justafish @deviantintegral if you want more context on the problem that originated this, there is more (including screenshots that demonstrate the bug) in this internal slack thread: https://lullabot.slack.com/archives/C0EFBKKK6/p1680079433850549
(keeping it internal because the screenshots contain client info, but we can continue the discussion of the issue in Drainpipe here).

[baluv3]

@mrdavidburns When we last spoke about this ticket, we agreed that a good next step was for you to revisit the deploy drush command and add more context here. We may need to split this into multiple tickets.

[mrdavidburns]

https://www.drush.org/latest/deploycommand/

It looks like it still runs both hook_update_N() and hook_post_update_N() before running config:import.

drush updatedb --no-cache-clear
drush cache:rebuild
drush config:import
drush cache:rebuild
drush deploy:hook

[deviantintegral]

I don't think we can literally use the drush command unless it has a workaround or flag for config imports failing on limited memory environments like Pantheon such as:

vendor/bin/drush config:import -y || true
vendor/bin/drush config:import -y

[marcoscano]

@mrdavidburns @deviantintegral I may be missing part of the context for your concerns here, would you mind elaborating a bit more?

Drush has incorporated several commands in the deploy:* namespace. If we run simply drush deploy , then all commands described in the documentation will be run. This is an equivalent of our task drupal:update and is not what we are suggesting to switch to.

The suggestion to avoid the bug described here is to move to the drush deploy:hook command after importing config, which is a new command that executes implementations of HOOK_deploy_NAME(). These are basically identical to HOOK_post_update_NAME() , except that they are handled by Drush and hence not subject to the core bug described here. We can place this anywhere we want, but it makes the most sense to place it instead of the second drush updatedb we have in our task drupal:update :

      - ./vendor/bin/drush {{.site}} --yes cache:clear plugin
      - ./vendor/bin/drush {{.site}} --yes updatedb
      # Run config:import twice to make sure we catch any config that didn't declare
      # a dependency correctly. This is also useful when importing large config sets
      # as it can sometimes hit an out of memory error.
      - ./vendor/bin/drush {{.site}} --yes config:import || true
      - ./vendor/bin/drush {{.site}} --yes config:import
      - ./vendor/bin/drush {{.site}} --yes deploy:hook
      - ./vendor/bin/drush {{.site}} --yes cache:rebuild

The most important impact this would have in sites using Drainpipe is a shift in the mental model for developers, because now "the code I want to run after importing config" will need to be placed in a hook_deploy_NAME() , instead of a hook_post_update_NAME(). Apart from this (which unfortunately would be a BC-breaking change), all the rest is the same AFAICT.

Actually it feels to me that this gets developers closer to what these hooks were intended for...

• hook_update_N() -> Use for schema and config changes that don't require Drupal's APIs
• hook_post_update_NAME() -> Use for updating content or changes that need all Drupal's APIs available
• hook_deploy_NAME() -> Use for changes that expect config to be imported from YAML files first

[e0ipso]

Drush has incorporated several commands in the deploy:* namespace. If we run simply drush deploy , then all commands described in the documentation will be run. This is an equivalent of our task drupal:update and is not what we are suggesting to switch to.

What would be the downside to align ourselves with upstream and adopt drush deploy instead? Sorry for the n00b question, I have never been in a project that uses Drainpipe yet.

[marcoscano]

They import config only once, and experience has demonstrated it's worth having it twice for several reasons.

[e0ipso]

Are there other reasons beyond Config Split? If I remember correctly, we have an ADR that recommends avoiding Config Split. Perhaps we cannot avoid it when we inherit a codebase though. 🤔

EDIT: https://architecture.lullabot.com/adr/20211026-use-settings-not-splits/

[marcoscano]

We've seen config import fail in environments where memory limit may be an issue (or when there is an unreasonably large number of config changes). In those cases a subsequent import resumes the import without significant downsides.
I think @deviantintegral was referring to this here.

I also discovered recently that some hosting solutions (such as Acquia Site Factory) require Config Split when managing configuration across sites, so even if it's not our preferred option, we can't rule it out.

[marcoscano]

Our docs in Drainpipe also mention the possibility of a dependency mis-declared, although I haven't experienced this one myself:

      # Run config:import twice to make sure we catch any config that didn't declare
      # a dependency correctly. This is also useful when importing large config sets
      # as it can sometimes hit an out of memory error.

[e0ipso]

Thanks for the clarifications! I understand now. We might make an argument in Drush's issue queue to this effect, but I agree that the solution proposed above is more practical. 👍

[justafish]

This has been merged and will be in the next major release. Thanks all!

[deviantintegral]

Would someone who's participated in this issue be willing to volunteer to write an update in the ADR repo to deprecate https://architecture.lullabot.com/adr/20210924-drupal-build-steps/ and replace it with a new one matching what we've decided on here?

[marcoscano]

With this change, the only thing preventing us from completely switching to a single drush deploy execution is the need to import config twice. It would be great if we could collaborate to get that option in drush itself... I posted drush-ops/drush#2449 (comment) a few weeks back, but didn't get an answer from the maintainer. Does anyone else feel the same? Should we try to clarify if this is a viable option before writing a new ADR?

[deviantintegral]

I commented there as well. Let's give a few days for a response, and then ping in Slack? That issue is closed so the maintainers may not read the comments.