Consider making recipes' comparison with existing config lenient by default

Created on 3 October 2024, 9 months ago

Problem/Motivation

Steps to reproduce

Proposed resolution

todo

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

โœจ Feature request
Status

Active

Version

11.0 ๐Ÿ”ฅ

Component

recipe system

Created by

๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @phenaproxima
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts

    Blocker's in!

  • ๐Ÿ‡จ๐Ÿ‡ฆCanada Liam Morland Ontario, CA ๐Ÿ‡จ๐Ÿ‡ฆ

    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:

    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.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts

    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. Either the entire recipe is 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 parts of a config entity should use config actions; thatโ€™s what theyโ€™re there for.

    That just leaves ignore and error, which we have implemented. We just call it strict and lenient. :)

    Does that make sense?

  • ๐Ÿ‡จ๐Ÿ‡ฆCanada Liam Morland Ontario, CA ๐Ÿ‡จ๐Ÿ‡ฆ

    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.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States thejimbirch Cape Cod, Massachusetts

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

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States thejimbirch Cape Cod, Massachusetts

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

  • ๐Ÿ‡ฎ๐Ÿ‡นItaly kopeboy Milan

    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?

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States thejimbirch Cape Cod, Massachusetts

    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
    
    
  • ๐Ÿ‡จ๐Ÿ‡ญSwitzerland berdir Switzerland

    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.

  • Merge request !11357Removes @TODO. โ†’ (Open) created by thejimbirch
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States thejimbirch Cape Cod, Massachusetts

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

  • Pipeline finished with Success
    4 months ago
    Total: 444s
    #438910
  • Status changed to RTBC 2 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Going to try and get in front of committers.

  • ๐Ÿ‡จ๐Ÿ‡ญSwitzerland berdir Switzerland

    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.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

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

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom alexpott ๐Ÿ‡ช๐Ÿ‡บ๐ŸŒ

    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: โœจ Recipes have no way to tell if they're set up for success Active 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?

  • ๐Ÿ‡จ๐Ÿ‡ญSwitzerland berdir Switzerland

    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.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States xjm

    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.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

    Sounds like we have consensus

Production build 0.71.5 2024