Add RecipeRunner::isApplying() to determine if a recipe is being applied, without confusing it with a config sync

I never created an issue about this, but have been questioning this in Slack for a very long time.

A common problem we've faced and this would solve: in a config preSave() we "heal" some configuration entity, e.g. after adding a new config schema prop.

For an existing site, an upgrade/post_update would run on dev/staging, and will be deployed to prod. That means we want to gate this on isSyncing(), so it doesn't run on a staging → prod deployment.

But if a module is providing shipped config, we want to heal that on installation. We can do that on preSave().

But what about recipes? Currently, that has isSyncing(), so there's no way to discern both. And as we cannot know which version was used for creating a config that is part of a recipe, we can't really attempt to self-heal recipes' provided config.

This is a problem in the context of Drupal CMS / Site templates, specially in combination with Drupal Canvas.

@penyaskito cross-posted with me!

Another impact: update paths. If config entity types need update paths and they follow the best practices in #3521618: Add generic interface + base class for upgrade paths that require config changes, then the preSave()-triggered update paths in there:

• "pure config sync" should not need to apply update paths (the site would've applied it already)
• "install recipe" should need to apply it (because exported config does not keep track metadata for A) extension versions, B) applied post-update hooks).

Currently both trigger isSyncing() === TRUE.

Further complication: Recipes installation is sometimes syncing, sometimes not

The syncing flag's value during recipe application appears to depend on which part of recipe installation/application is happening! Quoting a snippet of Canvas code (which I do not know nor claim to be fully accurate, but I'm hoping it might be a helpful bit of information in pushing this issue forward):

    // @todo Fix upstream core bug in Recipes: it inconsistently claims to be
    // syncing when installing modules, but not when installing configuration.
    // Even though it is listed under `import`, and that should hence match the
    // behavior of the /admin/config/development/configuration/single/import UI.
    if (in_array('installRecipeConfig', array_column(debug_backtrace(), 'function'), TRUE)) {
      // Assert the bug is still present. This will start failing as soon as the
      // upstream bug is fixed.
      assert(!$this->configInstaller->isSyncing());
      return;
    }

Ideally, core would have an actual API to determine if a recipe is being applied. That is being added over in #3613607: Add RecipeRunner::isApplying() to determine if a recipe is being applied, without confusing it with a config sync.

But until that's there, could Canvas broaden its check by looking for RecipeRunner anywhere in the backtrace, regardless of which method is being called? The presence of that class is a clear-as-a-bell indicator that a recipe is being applied.

Changing

    if (in_array('installRecipeConfig', array_column(debug_backtrace(), 'function'), TRUE)) {

to instead check a class being present seems fine to me 🤓 As long as it contains

// @todo Remove when https://www.drupal.org/project/drupal/issues/3613607 is fixed

!

Oh, hah, look at that: I related the issue to itself.

FYI, @phenaproxima tried leaping ahead of this landing in https://git.drupalcode.org/project/canvas/-/work_items/3591985

We need a new method to determine if a recipe is being applied: RecipeRunner::isApplying().

We can still use config sync -- not sure we can really undo that now -- but at least this will allow us to cleanly separate the concepts.

Discussed with @alexpott over Zoom.

I'm of the opinion that a totally separate flag is the right thing to do here. Although recipes have config sync-like behavior at certain points, they are not a config sync, and in the real world you are never actually syncing config while applying a recipe.

In other words, our API is describing a bogus situation, and developers are left holding the bag because they need to awkwardly account for that. For example, the $is_syncing flag in hook_modules_installed() is a damned lie when it is TRUE during recipe application. A module is being installed, but you're not really syncing!

We agreed that the path forward here is:

1. Add RecipeRunner::isApplying() in this issue as an immediate escape hatch for modules like Canvas, which currently have to implement torturous workarounds for recipes.
2. In a follow-up issue, decide whether we want to stop going into config sync mode entirely while applying a recipe. This would cleanly separate the concepts, but may also introduce enough disruption to be something that's only appropriate for Drupal 12.

IMHO, ship it. The test proves that every step that could be called by a batch job is covered by the flag. The implementation is simple and I like it.

Trying for a concise title for the git commit message.

Had one nit on the naming, but I see Gabor already also pointed out a similar thing and improved it to its current state. I don't have an improvement so let's leave it how it is - private method so could always be changed later.

After committing I realised the issue summary still mentions an enum, this could use an update for anyone who lands on this issue.

Apart from that I can't see anything to complain about. It might be good if the change record mentioned the use cases in #3 and #4 though since the example in there is a bit arbitrary.

Committed/pushed to main and 11.x, thanks!

Per #18, this is a bug fix and should ideally be backported to 11.4.x.

I'd also agree it's a bug because the status quo, where config sync is confused with recipe application, causes problems. This isn't the full fix to that problem, but it's an important step in the right direction, and it enables other related, and very important, bugs to be fixed.

Changing status accordingly.

Discussed with @catch on Slack, and I think I'm OK with an 11.4.x backport. There is a new method on RecipeRunner, but the class is final and can't be extended, so I think it's fine.

I had not thought about the class being final, only the new method, so yes this seems fine for backport, was on the fence otherwise.

Seems like a good backport for 11.4.x

Going through the patch to be ported issues.

Committed/pushed to 11.4.x, thanks!