[abo3losh1]

What type of PR is this?

• bug
• documentation

What this PR does / why we need it:

An environment variable that is set but empty marks a flag as set even when the flag cannot use the empty value. PostParse already skips parsing "" for anything that is not a string or a bool, since there is nothing to parse, but it still sets hasBeenSet. So the flag ends up marked as set while holding only its default:

os.Setenv("PORT", "")
cmd := &cli.Command{
    Flags: []cli.Flag{&cli.IntFlag{Name: "port", Required: true, Sources: cli.EnvVars("PORT")}},
    Action: func(_ context.Context, c *cli.Command) error {
        fmt.Println(c.Int("port"), c.IsSet("port"))
        return nil
    },
}
cmd.Run(context.Background(), []string{"app"})

Before: 0 true, no error. After: Required flag "port" not set.

The same applies to every source in the chain, so an empty file behaves like an empty environment variable. This has three visible effects for any flag other than string and bool (int, uint, float, duration, timestamp, slice, map and GenericFlag):

• Required is satisfied with a zero value. Docker Compose and Kubernetes pass an undefined variable through as an empty one, so a required port silently becomes 0 exactly when the config is missing.
• IsSet returns true for a flag that only holds its default.
• An empty variable earlier in the chain hides a later one. With EnvVars("LEGACY_PORT", "PORT"), LEGACY_PORT="" and PORT=8080, the flag keeps its default of 80.

Changes:

• flag_impl.go: PostParse walks Sources.Chain itself and skips an empty value for flags that cannot parse it, so it behaves like an unset variable and the next source gets a turn. String flags still take "" and bool flags still read it as false, so c.IsSet returns true for empty env vars #517 / Consider empty environment variables as set #530 (an empty string variable counts as set) is unchanged. A non-empty value that fails to parse is still an error, rather than being skipped.
• flag_test.go: TestFlagsFromEmptyEnv, a table test run through Command.Run, and TestFlagFromEmptyFileSource for an empty file.
• docs/v3/examples/flags/value-sources.md: one paragraph, under the general note on Sources order, on how an empty value from any source is treated.

With a single source the flag's value never changes, only IsSet and Required now agree with it.

Which issue(s) this PR fixes:

No open issue. Related history: #517 and #530 decided that an empty variable counts as set for a string flag. That still holds; this only covers flag types that have no way to hold an empty value.

Special notes for your reviewer:

#2449 added f.fromSource = true next to f.hasBeenSet = true in the same block of PostParse. main is merged into this branch with both lines kept inside the loop, and the tests from #2449 pass.

Testing

• TestFlagFromEmptyFileSource fails on main, where the flag is set to 0 with no error.
• TestFlagsFromEmptyEnv: five cases fail on main (required int, required duration, IsSet for an int and a string slice that kept their defaults, and the shadowed second variable). Three guard cases pass before and after: an unparsable value still errors, and string and bool flags still treat an empty value as set.
• go vet ./... and go test ./... pass, and v3diff shows no public API change.

Release Notes

An empty value from any source, such as an environment variable that is set but empty or an empty file, is now skipped for flags other than string and bool, including GenericFlag. It no longer satisfies Required, IsSet stays false, and a later source in the same Sources list is used instead. String and bool flags are unchanged.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Juneezee]

Thanks, the fix looks right to me. The five new cases fail on main (f6f38ed) and pass here, and go test ./... passes.

1. This conflicts with main now that #2449 is merged. Keeping both lines resolves it:

			f.hasBeenSet = true
			f.fromSource = true
			break

cli/flag_impl.go

Lines 158 to 159 in f6f38ed

	f.hasBeenSet = true
	f.fromSource = true

I tried that merge locally and go test ./... passes. I can push it to this branch if that is easier for you.

2. The skip covers every source, not only env vars, since the loop checks val == "" for anything in Sources.Chain:

cli/flag_impl.go

Lines 149 to 153 in 4d537c9

	for _, source := range f.Sources.Chain {
	val, found := source.Lookup()
	if !found || (val == "" && !emptyIsValue) {
	continue
	}

For example, an empty file now leaves a required int flag unset too:

p := filepath.Join(t.TempDir(), "port")
os.WriteFile(p, nil, 0o600)
cmd := &cli.Command{
	Flags: []cli.Flag{&cli.IntFlag{Name: "port", Required: true, Sources: cli.Files(p)}},
}
err := cmd.Run(context.Background(), []string{"app"})
// main: err is nil, port is 0, IsSet is true
// this PR: Required flag "port" not set

I think that is the right behavior, but the docs paragraph is under "Values from the Environment" and the release note only mentions environment variables. GenericFlag is also affected (IsSet goes from true to false) but is not in the release note's list. Could you word both as something like "an empty value from any source is skipped for flags other than string and bool"?

cli/docs/v3/examples/flags/value-sources.md

Lines 65 to 68 in 4d537c9

	An environment variable that is set but empty only counts as a value for string and
	bool flags, where it means an empty string and `false`. For other flags it is skipped,
	so it does not satisfy `Required`, does not make `IsSet` true, and does not hide a
	later variable in the list.

[abo3losh1]

Thanks for checking it so carefully.

1. Merged main and kept both lines, as you suggested. go test ./... passes, including the tests from fix: let a subcommand's command-line value replace a persistent flag's env/file value #2449.
2. Good catch, the skip does cover every source. I agree that's the right behavior, so I made it explicit: the docs paragraph now sits under the general note on Sources order and says "an empty value from any source", TestFlagFromEmptyFileSource covers the empty file case (it fails on main), and the release note now covers any source and lists GenericFlag.

[Juneezee]

Thanks for the quick update, LGTM.

Verified on 72a6646:

• TestFlagsFromEmptyEnv and TestFlagFromEmptyFileSource fail on main (f6f38ed) and pass here.
• go test ./... passes with and without -tags urfave_cli_no_template.

cli/flag_test.go

Lines 677 to 768 in 72a6466

	func TestFlagsFromEmptyEnv(t *testing.T) {
	testCases := []struct {
	name string
	env map[string]string
	fl Flag
	wantValue any
	wantIsSet bool
	errContains string
	}{
	{
	name: "required int from an empty env var is not set",
	env: map[string]string{"PORT": ""},
	fl: &IntFlag{Name: "port", Required: true, Sources: EnvVars("PORT")},
	errContains: `Required flag "port" not set`,
	},
	{
	name: "required duration from an empty env var is not set",
	env: map[string]string{"TIMEOUT": ""},
	fl: &DurationFlag{Name: "timeout", Required: true, Sources: EnvVars("TIMEOUT")},
	errContains: `Required flag "timeout" not set`,
	},
	{
	name: "int keeps its default and is not set",
	env: map[string]string{"PORT": ""},
	fl: &IntFlag{Name: "port", Value: 80, Sources: EnvVars("PORT")},
	wantValue: 80,
	},
	{
	name: "string slice keeps its default and is not set",
	env: map[string]string{"TAGS": ""},
	fl: &StringSliceFlag{Name: "tags", Value: []string{"a"}, Sources: EnvVars("TAGS")},
	wantValue: []string{"a"},
	},
	{
	name: "an empty env var does not hide a later one",
	env: map[string]string{"LEGACY_PORT": "", "PORT": "8080"},
	fl: &IntFlag{Name: "port", Value: 80, Sources: EnvVars("LEGACY_PORT", "PORT")},
	wantValue: 8080,
	wantIsSet: true,
	},
	{
	name: "an unparsable env var still fails before a later one",
	env: map[string]string{"LEGACY_PORT": "abc", "PORT": "8080"},
	fl: &IntFlag{Name: "port", Sources: EnvVars("LEGACY_PORT", "PORT")},
	errContains: `could not parse "abc" as int value from environment variable "LEGACY_PORT"`,
	},
	{
	name: "an empty string is still a value",
	env: map[string]string{"NAME": ""},
	fl: &StringFlag{Name: "name", Value: "def", Sources: EnvVars("NAME")},
	wantValue: "",
	wantIsSet: true,
	},
	{
	name: "an empty bool still reads as false",
	env: map[string]string{"DEBUG": ""},
	fl: &BoolFlag{Name: "debug", Value: true, Sources: EnvVars("DEBUG")},
	wantValue: false,
	wantIsSet: true,
	},
	}
	for _, tc := range testCases {
	t.Run(tc.name, func(t *testing.T) {
	for k, v := range tc.env {
	t.Setenv(k, v)
	}
	name := tc.fl.Names()[0]
	var gotValue any
	var gotIsSet bool
	cmd := &Command{
	Flags: []Flag{tc.fl},
	Action: func(_ context.Context, cmd *Command) error {
	gotValue = cmd.Value(name)
	gotIsSet = cmd.IsSet(name)
	return nil
	},
	}
	err := cmd.Run(buildTestContext(t), []string{"run"})
	if tc.errContains != "" {
	require.ErrorContains(t, err, tc.errContains)
	return
	}
	require.NoError(t, err)
	assert.Equal(t, tc.wantValue, gotValue)
	assert.Equal(t, tc.wantIsSet, gotIsSet)
	})
	}
	}

cli/flag_test.go

Lines 772 to 782 in 72a6466

	func TestFlagFromEmptyFileSource(t *testing.T) {
	path := filepath.Join(t.TempDir(), "port")
	require.NoError(t, os.WriteFile(path, nil, 0o600))
	cmd := &Command{
	Flags: []Flag{&IntFlag{Name: "port", Required: true, Sources: Files(path)}},
	}
	err := cmd.Run(buildTestContext(t), []string{"run"})
	require.ErrorContains(t, err, `Required flag "port" not set`)
	}