Problem/Motivation

There are currently 3 sources for default values when it comes to input collection for recipes: value, config, and env. My original idea on how their usage could be improved to make recipes more generically applicable was a chain of fallbacks like this:

input:
  name:
    default:
      - source: env
        env: NAME
      - source: config
        config: ['some.settings', 'name']
      - source: value
        value: My name is something

This could be used in automatic scripts where env variables would be available, but it could also be used interactively, and to re-apply a recipe, without having to maintain a recipe in different flavours for those different use cases. It would use the first source that provides a value in the order env/config/value.

But in a Slack thread I learned that this is too complex but that a fallback value for at least the config source would make sense, e.g. like this:

input_name:
  default:
    source: config
    config: ['foo.settings', 'baz']
    fallback: bar

So, if a config object foo.settings doesn't exist (yet), instead of failing in InputConfigurator.php line 191, it could fall back to the given value.

Introduced terminology

The term "fallback" is introduced to describe a default value for something.

Issue fork drupal-3537627

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

jurgenhaas created an issue. See original summary.

thejimbirch’s picture

thejimbirch commented
Issue tags: +Recipes initiative
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Title: Add default value for config source on input defaults for recipes » If a recipe takes a default input value from config, it should be able to fall back to a static value if that config doesn't exist

Re-titling for clarity.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

We could implement this by replacing the unconditional \RuntimeException with this:

if ($config->isNew()) {
  return $settings['value'] ?? throw new \RuntimeException("The '$name' config object does not exist.");
}

Nice one-line fix and easy to test. I'd be okay with making this allowance for the config value source specifically.

thejimbirch opened merge request !12818

thejimbirch’s picture

thejimbirch commented
Issue tags: +Needs tests

I created a MR with the change. Added Needs tests tag is someone wants to update/add a test for this.

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented
Status: Active » Needs review

Ready for a review, the nightwatch failure doesn't seem to be related.

phenaproxima’s picture

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

This looks good to me but I don't think we need to bring the console-specific input collection stuff into it.

phenaproxima’s picture

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

thejimbirch commented
Issue summary: View changes

Updated issue summary.

thejimbirch’s picture

thejimbirch commented
Assigned: Unassigned » alexpott
Issue tags: +Needs subsystem maintainer review

We're going to park this for @alexpott to review.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Needs work

I think we also should support the fallback value for env too. The differences between the env and the config implementations are interesting. Env with return an empty string when the env var does not exist but config will throw an exception. I wonder what the right the thing to do here is...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue tags: -Needs subsystem maintainer review

FWIW the concept of a fallback value seems go to me.

phenaproxima’s picture

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

I like the idea of making it consistent.

thejimbirch’s picture

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

Looks great, Love the additional test and the comment updates.

I added a draft change record and am moving to RTBC.

thejimbirch’s picture

thejimbirch commented
Related issues: +#3540326: Update Recipe Input documentation to add Fallbacks
alexpott’s picture

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

I think we should allow NULL as a fallback value. See review on 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

Suggestions added, all threads resolved.

I added the null possibility to the change record, and am moving this back to RTBC.

alexpott’s picture

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

Committed c961ea3 and pushed to 11.x. Thanks!

  • alexpott committed c961ea39 on 11.x 
    Issue #3537627 by phenaproxima, jurgenhaas, thejimbirch, alexpott: If a...
damienmckenna’s picture

damienmckenna
Location TN, USA
commented

Out of interest why was the attribute name "fallback" used rather than e.g. "default_value" or just "default", a naming convention used in other subsystems?

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Issue summary: View changes

I opened #3541371: Rename "fallback" recipe attribute to "default_value" to fix the naming convention.

Status: Fixed » Closed (fixed)

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