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 to make it clear what to do with existing config.

We'll support a new strict key under the config section of recipe.yml. It be one of three things:

  • true, meaning that all of the config listed in the import section, and in the recipe's config directory, must match what's in active config.
  • false, meaning that the config listed in import, and in the recipe's config directory, does not need to match what's in active config. It's fine if it does, but it doesn't have to.
  • an array of config names that need to match what's in active config

For example:

config:
  import:
    user:
      - user.role.anoymous
    metatag: '*'
  strict:
    - metatag.defaults.global

This says:

  • We're going to get the anonymous role from the User module, if it doesn't already exist. If it does exist, we'll just use what we already have.
  • We're going to get everything we can from the Metatag module. If we already have some stuff from Metatag in the DB, we'll keep those things intact.
  • But metatag.defaults.global, if we already have it in the database, HAS to be identical to what Metatag provides by default! If it's not, then that's an error.

To clarify the difference here:

  • import tells us what config to bring in, and where to find it.
  • strict tells us what to do if any of that config already exists in the database.

Issue fork drupal-3478332

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:

About issue forks

Comments

phenaproxima created an issue. See original summary.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Issue summary: View changes
thejimbirch’s picture

thejimbirch commented
Issue tags: +Recipes initiative

I believe you captured all that we discussed.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Assigned: Unassigned » phenaproxima

Jealously grabbing this issue for myself. :)

phenaproxima opened merge request !9734

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Active » Needs review
Issue tags: +Needs tests

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

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Issue tags: +Starshot blocker

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

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

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. I don't want recipes to start looking like shell scripts. 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 would be a synonym for ?all.

Thoughts?

quietone’s picture

quietone commented
Version: 11.0.x-dev » 11.x-dev
nicxvan’s picture

nicxvan commented

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: !

nicxvan’s picture

nicxvan commented

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 are no additional bash like special characters.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

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. "Pay attention, this matters to me!"

It's the opposite of the question mark, which conveys ambiguity. "I might need this, I might not." 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.

You're right that using single characters like ! and ? does add a little cognitive load. We could maybe do something like this instead, for clarity:

config:
  import:
    user:
      - user.role.anonymous
      - 'strict:user.role.authenticated'
nicxvan’s picture

nicxvan commented

! 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.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

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.

nicxvan’s picture

nicxvan commented

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.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

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 (more or less as you suggested)? You'd be looking at syntax like this:

config:
  import:
    user: '*'
  strict:
    # Everything in this list MUST be exactly the same as it is in the active config, regardless of whether it's coming from an extension or from the recipe's config directory.
    - user.role.authenticated

This would get us where we need to go, with no special new syntax. It lets the intent be explicit. And it would be extremely simple to implement.

I kinda love this idea.

nicxvan’s picture

nicxvan commented

Deleted: Duplicate comment of 15

nicxvan’s picture

nicxvan commented

I like the structure in 16!

Only thought is maybe import and strictImport?

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

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.

mandclu’s picture

mandclu commented

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

ironnuts’s picture

ironnuts commented

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. So I am speculating that the word 'force' could be used in this new scheme. It is short and sweet?

Maybe not as strict is not the same concept. But could a flag be passed like --strict? Characters like '!' and '?' are used in different contexts like now in PHP 8 in typehinting. Maybe they are overused and best avoided as the brain tries to revolve through all the different contexts trying to figure out which might apply. Even though as they say 'brevity is the source of wit.'

nicxvan’s picture

nicxvan commented

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.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

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.

ironnuts’s picture

ironnuts commented

@phenaproxima May the "force" be with you!

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Needs work

Still needs work.

phenaproxima credited alexpott.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Issue summary: View changes

@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.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Issue summary: View changes
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Assigned: phenaproxima » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests

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.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

Wrote a change record: https://www.drupal.org/node/3478662

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Title: Recipes' imported config is validated too strictly by default » Recipes' imported config is compared to active config too strictly by default
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Title: Recipes' imported config is compared to active config too strictly by default » Add a way to prevent recipes' imported config from being compared too strictly to active config
thejimbirch’s picture

thejimbirch commented
Status: Needs review » Reviewed & tested by the community

All threads resolved, well commented and has tests.

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

Marking as RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Added a couple of comments to the MR.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs work » Needs review
thejimbirch’s picture

thejimbirch commented
Status: Needs review » Reviewed & tested by the community

Feedback addressed. Back to Alex.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 11.x-dev » 10.4.x-dev
Status: Reviewed & tested by the community » Fixed

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...
liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented

One problem related to this is that #3480248: Drupal does not recognize when the config is identical. 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: [pp-1] Make recipes' comparison with existing config lenient by default.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
Related issues: +#3480248: Drupal does not recognize when the config is identical, +#3478669: [pp-1] Make recipes' comparison with existing config lenient by default

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

thejimbirch’s picture

thejimbirch commented
Issue tags: +11.1.0 release highlights