Recipes' imported config is validated too strictly by default

Created on 2 October 2024, 16 days ago

Problem/Motivation

Let's say you have a recipe like this one:

name: Test
install:
  - user
config:
  import:
    user:
      - user.role.anonymous
  actions:
    user.role.anonymous:
      grantPermission: 'do something cool'

If you try to apply this on an installed site, and the anonymous user role doesn't look exactly like what's in core/modules/user/config/install/user.role.anonymous.yml, you'll get a validation error.

This is because the recipe system is currently way too strict about comparing the active config to the config that the recipe wants to import, to the point where it interferes with the recipe author's intent.

This example recipe doesn't really care about what the anonymous role looks like. It will grant a permission to it, but its other details are irrelevant. It just needs to be sure the anonymous role exists. But there's no way to do that, right now, except to have the recipe define the anonymous role using the createIfNotExists config action, which is a crappy, inelegant workaround.

If you'll permit me a little role-play here: it's already possible to say this:

  • If the anonymous role exists, it has to look EXACTLY like what the User module ships.
  • Assuming it does, we're all good! Let's proceed.

But what we really want here is:

  • If the anonymous role exists, great! I don't care what it looks like; we're all good. Let's proceed.
  • If it doesn't exist, please import it from the User module so we can proceed.

Proposed resolution

Let's introduce some lightweight, explicit syntax for making the intention clear.

This will mean "hey - I need the anonymous role to exist, and it needs to look exactly the way the User module ships it, or I can't proceed":

config:
  import:
    user:
      - !user.role.anonymous

And this will mean "I need the anonymous role to exist - if it doesn't, please import it from the User module":

config:
  import:
    user:
      - ?user.role.anonymous

For better authoring experience, and to maintain backwards compatibility with recipes that already exist in the wild, the current supported syntax will have the more behavior. So this will also mean "I need the anonymous role, import it from User if it doesn't exist already":

config:
  import:
    user:
      - user.role.anonymous

This is a change from the current behavior, which acts more like the strict ! operator.

This won't affect the way we treat config that ships in a recipe's config directory. That is always checked strictly - if a recipe wants to be non-strict about it, that's precisely what the createIfNotExists config action is for.

πŸ› Bug report
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 thejimbirch Cape Cod, Massachusetts

    I believe you captured all that we discussed.

  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

    Jealously grabbing this issue for myself. :)

  • Merge request !9734Strip prefix operators β†’ (Open) created by phenaproxima
  • Pipeline finished with Failed
    16 days ago
    Total: 708s
    #299216
  • Pipeline finished with Success
    16 days ago
    Total: 523s
    #299226
  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

    Needs tests, but the overall approach is ready for review.

  • Pipeline finished with Canceled
    16 days ago
    Total: 125s
    #299236
  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

    This is affecting Drupal CMS's recipe construction and testing, so tagging as a Starshot blocker.

  • Pipeline finished with Success
    16 days ago
    Total: 450s
    #299237
  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

    One thing I realized we haven't thought about is what to do with lines like:

    config:
      import:
        tour: '*'
    

    Should all that config be treated strictly? Or more relaxed?

    I wonder if we should do something like this to opt it into strict mode:

    config:
      import:
        tour: '!*'
    

    My only concern is that this syntax starts to look arcane and, well, Unix-ey. In a way that is not intuitive for recipe authors. I wonder if we could do something like this, then:

    config:
      import:
        tour: '!all' # Or '?all' for the non-strict version
    

    And then later on we could change the '*' token to just 'all', which is a synonym for '?all'.

    Thoughts?

  • πŸ‡³πŸ‡ΏNew Zealand quietone
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    One tangential concern I have about this syntax is the !include convention is used by at least one prominent python project in order to include additional files.

    I can see wanting to split actions into separate files in the future to keep things organized and it might be nice to match that syntax if a parser is written.

    https://community.home-assistant.io/t/use-of-include-of-additional-yaml-... as an example.

    I agree this issue is super helpful, just thinking out loud about the use of: !

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    What if we added another layer
    (Sorry on my phone)

    Import:
      Strict:
        tour: '*'
    

    Or

    Import:
      Lenient:
        tour: '*'
    

    Default or no key could be lenient for backwards compatibility.

    But then there is no bash like special characters.

  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

    I'm open to considering a different syntax.

    The reason I went for ! is because it's reminiscent of !important in CSS. To me, it conveys urgency and primacy. "This is important to me!"

    It's the opposite of the question mark, which conveys ambiguity. "Maybe I need this." I think the same reasoning is why PHP has the ? for nullable types, and ?-> as the nullsafe operator.

    If recipes ever were able to include other files, then I'm guessing we'd go for a syntax like `@include` or something like that. Dunno.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    ! And ? Make more sense spelled out but I think nesting like my second comment is more explicit and easier to remember.

    My first thought when seeing ! was the not operator so people might think it's an exclusion.

  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

    My first thought when seeing ! was the not operator so people might think it's an exclusion.

    Oooh, that's a great point - definitely a strong reason to consider some other way to opt into strict comparison.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    I think the only benefit for:

    import:
      strict:
        tour: '*'
    

    Over

    import:
        strict:tour: '*'
    

    Is not having to type strict for each line of you have multiple.

    But it's probably safer to need to specify strict on every config you need strict validation.

  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

    How about this as an idea:

    What if we made the comparisons lenient by default, but then allowed an explicit list of config to be compared strictly? You'd be looking at syntax like this:

    config:
      import:
        user: '*'
      strict:
        - user.role.authenticated
    

    This would get us where we need to go, with no special syntax. And it would be extremely simple to implement.

    I kinda love this idea.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    I think the only benefit for:

    import:
      strict:
        tour: '*'
    

    Over

    import:
        strict:tour: '*'
    

    Is not having to type strict for each line of you have multiple.

    But it's probably safer to need to specify strict on every config you need strict validation.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    I like the structure in 16!

    Only thought is maybe import and strictImport?

  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

    The only argument I can think of against strict_import is that it would apply to stuff in the recipe's config directory too, not just the stuff listed in import. But I don't feel strongly.

  • πŸ‡¨πŸ‡¦Canada mandclu

    One idea: maybe instead of import and strict_import, we could say import (always strict) and ensure (which is lenient)?

  • πŸ‡¬πŸ‡§United Kingdom andrew.farquharson

    Yes and in bash shell there is -f ie --force. Which is pretty self-explanatory. So the command might or might not work but with --force it will work for sure.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    I'm not sure ensure is as clear as strict.

    Also @oily, force implies that it will impose the structure no matter what, where in this case if it can't it will fail. Also this isn't about running the command so you can pass it, it's for the recipe authors to determine the importance of particular config.

  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

    We're sort of derailing the issue, but we've already got plans for what "force" means, and it will be added in another issue.

  • πŸ‡¬πŸ‡§United Kingdom andrew.farquharson

    @phenaproxima May the "force" be with you!

  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

    Still needs work.

  • Pipeline finished with Success
    15 days ago
    Total: 444s
    #300018
  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ
  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

    @nicxvan, @alexpott, @thejimbirch, and I discussed this in Slack and we found a better syntax. I'm updating the issue summary to reflect the proposal.

  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts
  • Pipeline finished with Success
    15 days ago
    Total: 965s
    #300069
  • Pipeline finished with Failed
    15 days ago
    Total: 676s
    #300141
  • Pipeline finished with Success
    15 days ago
    Total: 1239s
    #300142
  • Pipeline finished with Canceled
    15 days ago
    Total: 645s
    #300169
  • Pipeline finished with Failed
    15 days ago
    Total: 528s
    #300176
  • Pipeline finished with Failed
    15 days ago
    Total: 1169s
    #300186
  • Pipeline finished with Canceled
    15 days ago
    Total: 278s
    #300208
  • Pipeline finished with Success
    15 days ago
    Total: 846s
    #300213
  • Pipeline finished with Canceled
    15 days ago
    Total: 224s
    #300243
  • Pipeline finished with Canceled
    15 days ago
    Total: 250s
    #300246
  • Pipeline finished with Success
    15 days ago
    Total: 507s
    #300250
  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

    We're happy with the design and tests are written (and passing!)...I think this is truly ready for review.

    The one thing to note: at the moment, this MR keeps the existing strict-by-default behavior. Should we change that now? Or in a follow-up? It feels a little bit like follow-up material since it is a behavior change and most likely warrants a change record. But, happy to go either way.

  • Pipeline finished with Canceled
    14 days ago
    Total: 293s
    #300259
  • Pipeline finished with Success
    14 days ago
    Total: 480s
    #300263
  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts
  • Pipeline finished with Success
    14 days ago
    Total: 1761s
    #300279
  • Pipeline finished with Canceled
    14 days ago
    Total: 188s
    #300317
  • Pipeline finished with Canceled
    14 days ago
    Total: 475s
    #300319
  • Pipeline finished with Failed
    14 days ago
    Total: 414s
    #300325
  • Pipeline finished with Success
    14 days ago
    Total: 1638s
    #300348
  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts
  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts
  • Pipeline finished with Success
    14 days ago
    Total: 822s
    #300957
  • πŸ‡ΊπŸ‡ΈUnited States thejimbirch Cape Cod, Massachusetts

    All threads resolved, well commented and has tests.

    This is a great step forward for recipe authors and leniency.

    Marking as RTBC.

  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    Added a couple of comments to the MR.

  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts
  • πŸ‡ΊπŸ‡ΈUnited States thejimbirch Cape Cod, Massachusetts

    Feedback addressed. Back to Alex.

  • Pipeline finished with Success
    11 days ago
    #303070
  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    Committed and pushed f0fda63b58 to 11.x and fcf7d526b3 to 10.4.x. Thanks!

    • alexpott β†’ committed fcf7d526 on 10.4.x
      Issue #3478332 by phenaproxima, nicxvan, thejimbirch, alexpott: Add a...
    • alexpott β†’ committed f0fda63b on 11.x
      Issue #3478332 by phenaproxima, nicxvan, thejimbirch, alexpott: Add a...
  • πŸ‡¨πŸ‡¦Canada Liam Morland Ontario, CA πŸ‡¨πŸ‡¦

    One problem related to this is that πŸ› Drupal does not recognize when the config is identical Active . I have applied a recipe and then immediately applied it again was told that the config was not identical.

    There are more cases that need to be handled than just strict or not strict. I wrote about four possibilities, replace, update, ignore, and error, in #3478669-5: Consider making recipes' comparison with existing config lenient by default β†’ .

  • πŸ‡¨πŸ‡¦Canada Liam Morland Ontario, CA πŸ‡¨πŸ‡¦
Production build 0.71.5 2024