Problem/Motivation

This is a follow-up to #3478332: Add a way to prevent recipes' imported config from being compared too strictly to active config.

That issue made it possible for recipes to opt out of the strict comparison between the recipe's shipped config, and the active config. This is a good thing, because it allows an entire class of bugs to be eliminated/sidestepped. However, it's not yet the default behavior -- we still use the strict checking by default.

Should we change the default behavior to be lenient, and allow recipes to opt into the strict checks?

Issue fork drupal-3478669

Command icon 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:

Comments

phenaproxima created an issue. See original summary.

phenaproxima’s picture

Title: Consider making recipes' comparison with existing config lenient by default » [PP-1] Consider making recipes' comparison with existing config lenient by default
Issue summary: View changes
Status: Active » Postponed
Related issues: +#3478332: Add a way to prevent recipes' imported config from being compared too strictly to active config
phenaproxima’s picture

Issue summary: View changes
phenaproxima’s picture

Title: [PP-1] Consider making recipes' comparison with existing config lenient by default » Consider making recipes' comparison with existing config lenient by default
Status: Postponed » Active

Blocker's in!

liam morland’s picture

There are more cases that need to be handled than just strict or not strict. When applying a recipe, I can imagine the following possible courses of action if the recipe has a config file that contains different config from what is installed in the site:

Replace
Replace the config with the version specified by the recipe.
Update
Update the config so that the config variables specified by the recipe match the recipe. Other variables are left as they are.
Ignore
Do not change this config and do not raise an error.
Error
Raise an error and halt processing.

Perhaps this should be controlled by an on-conflict key in the YML file. This would echo how some SQL databases, such as Postgres, have an ON CONFLICT clause to INSERT which takes values DO NOTHING and DO UPDATE.

phenaproxima’s picture

I’m not sure replace and update are going to fly as you describe.

Replace would be the same as the concept of “force apply” — i.e., “import the recipe’s version of the config even if it already exists”. We intend to support that in another issue but it will not likely be something that is selective. The entire recipe is either force-applied or it isn’t.

Update, on the other hand, sounds like allowing recipes to ship partial config, which is probably a non-starter. That’s just plain not what recipes are designed to do. Recipes that want to tweak individual config variables should use config actions; that’s what they’re there for. (Also, what would happen if a recipe shipped pieces of config, but the config didn’t exist already? The site would end up with broken config.) Recipes aren’t meant to be, or have, update paths for config.

Unless I’m missing something, the problem with either a selective replace, or config partials, is that they create the possibility of recipes accidentally mangling a site’s config so that it’s invalid or incoherent, which will probably break the site.

That just leaves ignore and error, which we have implemented. We just call it strict and lenient. :) And selective strictness is supported too.

Does that make sense?

liam morland’s picture

Update can be achieved with config actions, but that means changing the format of the config. Files for the config directory can be generated using Drush config export. I am not aware of any automated way of converting those into the format needed for inclusion in a YML file as an action.

What if I want a recipe that installs and configures a module and adds a button to the CKEditor toolbar? I don't want that to remove buttons that are already there.

I'm imagining that a file used for Update would always be a complete config file so installing it wouldn't create an invalid config object.

I don't think "strict" and "lenient" are clear in their meaning.

Generally speaking, if a recipe does not install the config that it comes with, it would be misleading for the recipe to report that it was successfully applied.

thejimbirch’s picture

Title: Consider making recipes' comparison with existing config lenient by default » Make recipes' comparison with existing config lenient by default
Issue tags: +Recipes initiative

Since we now have the option of importing config from modules or the config folder strict, the default should be changed to lenient.

thejimbirch’s picture

Since you can't set strict: false and then enforce individual strict configs, changing to lenient will allow the greatest flexibility.

kopeboy’s picture

Does this mean that recipes can now be applied successfully on the command line, even if the configuration they include isn't actually applied to the site? How would a site builder identify pre-existing configuration issues and distinguish them from problems with the recipe itself?

EDIT: Sorry, I just read this and it's much clearer now: #3478332: Add a way to prevent recipes' imported config from being compared too strictly to active config 👍🏻👏🏻

thejimbirch’s picture

Status: Active » Closed (works as designed)

This was discussed in a Recipes initiative meeting and we decided it was best to leave it as is. Recipe author documentation was updated with the following that allows for most of everyone's needs except for replace/override which is a deliberate decision that the recipe team has made to not be destructive to sites.

  # The recipe system compares configuration from your recipe and the modules
  # you bring in against your site's active configuration. By default it
  # requires that they be identical in every detail, or it throws an error.
  # strict: true

  # You can set this to false to have the recipe runner skip the config import
  # if it finds anything different.
  # strict: false

  # A recipe can choose to opt only certain config into the strict checking. If
  # you specify individual config files to be treated strictly, then all others
  # will be treated lenient.
  # strict:
  #   - field.storage.field_oysters
  #   - field.storage.field_quahogs

berdir’s picture

Status: Closed (works as designed) » Active

There is still a todo in the code pointing to this issue, if this doesn't happen, that should be removed.

I'd like to make a counter-argument. strict also seems to apply to config provided by modules.

I did just run into a related error in paragraphs with this workaround recipe:

name: 'Paragraphs Demo Language'
description: 'INTERNAL'
type: 'Demo'
install:
  - drupal:language
config:
  import:
    language: '*'

Trying to apply this on top of umami resulted in this error: The configuration 'language.negotiation' exists already and does not match the recipe's configuration

Another use case is that depending recipes are re-applied. Lets say you have two recipes, A and B. B depends on A. You apply A, customize it (lets say it has a node type and you add extra fields to it, resulting in changed view/form displays. Then later you apply B. This now fails because the config from A no longer applies strictly.

I really think recipes shouldn't be strict by default, I struggle to see real-world use cases where you want it to be. Maybe for specific config, but even that seems quite problematic. Sure, things that can wrong if it doesn't match. But things can also go wrong if it matches.

thejimbirch’s picture

Status: Active » Needs review

MR added that removes the @todo if we are going to keep it that way.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Going to try and get in front of committers.

berdir’s picture

FWIW, I still think we should change this, see #12 and also https://berdir.github.io/recipes/#/9/2. But up to core maintainers to make the decision to discuss further or get rid of the todo per MR.

larowlan’s picture

I'll put this to other committers to see if we can get consensus

alexpott’s picture

I'm really not sure what to do here.

On one hand it is already easy for a recipe to set the behaviour it needs. On the other, the reason this strictness exists is to give recipes a solid expected ground to work on. @berdir is obviously correct that the easy examples like the language one given where strictness just gets in the way.

I feel that if we default to lenient then no one is ever going to benefit from strict and if don't change the default the idea of inter-operable recipes than many users can benefit from might not happen because users will hit this all the time. Especially will any recipe that does multilingual.

I guess that we need to go back to the solid ground idea and why that existed. I think there are two reasons; firstly the idea a recipe is idempotent - you apply it and you get the same results and secondly when you start applying config actions on top of configuration they are often making assumptions about the starting state of that configuration. The first idea needs challenging a bit because if a recipe is applied to a site and language has already been installed and configured and the recipe is not changing language configuration then the fact that the language config no longer matches the default state does not matter at all. The second idea is sometimes correct but not always and it feels as though the solution we have is too strict to solve this particular case. I think we need to look into better solutions for this and we already have issues open for this: #3461946: Recipes have no way to tell if they're set up for success and #3463641: Create a way for recipes to check their preconditions. I think if we land the precondition issue then we can change this default - and perhaps consider deprecating the functionality and telling people to add preconditions instead.

@berdir what do you think about this idea? Does it work for you?

berdir’s picture

Generally fine with me. Do we want to postpone this on the preconditions issue (didn't check that yet but title sounds interesting) and then re-evaluate? Additionally to that, as we improve config validation, that will also help. Seems to be that config validation should _somehow_ be able to verify that the field + field storage type matches, which is the primary (sole?) use case where strict is now used in Drupal CMS. And if validation is too hard for that, then preconditions, yes.

xjm credited catch.

xjm’s picture

We discussed this with @alexpott, @catch, @larowlan, and myself (thence the above comment). While the overall response were variants of uncertainty, we did agree that "In general, about things like this, we should assume @berdir is probably right". 😉

Some other points from the discussion (these two are quotes of me):

One thing worth considering is that it's easier to keep it strict now and relax it later, versus having to revert back to strict later

Maybe it's like update hooks: People should be defensive and prepared for any data structure, regardless of the original from their own project. (They won't be, and so there will be regressions, but that doesn't mean we make update hooks fatal.) So I lean toward making the change. I like trying to solve the underlying needs it originally addressed first.

I think this is one of those things where there is going to be some pain no matter which option we choose, and so we choose which kind of pain makes sense. I like that @berdir said:

Sure, things that can wrong if it doesn't match. But things can also go wrong if it matches.

So I like the idea of going back to the original usecase for the strict matching, and trying to solve its needs in other ways.

Postponing does seem appropriate.

larowlan’s picture

Title: Make recipes' comparison with existing config lenient by default » [pp-1] Make recipes' comparison with existing config lenient by default
Status: Reviewed & tested by the community » Postponed

Sounds like we have consensus

thejimbirch’s picture

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.