MicrosoftDocs / PowerShell-Docs

The official PowerShell documentation sources
https://learn.microsoft.com/powershell
Other
2.01k stars 1.58k forks source link

-Force implementation on ShouldProcess page should be reviewed and changed. #11418

Closed jikuja closed 2 months ago

jikuja commented 2 months ago

Type of issue

Code doesn't work

Feedback

function Test-ShouldProcess {
    [CmdletBinding(
        SupportsShouldProcess,
        ConfirmImpact = 'High'
    )]
    param(
        [Switch]$Force
    )

    if ($Force -and -not $Confirm){
        $ConfirmPreference = 'None'
    }

    if ($PSCmdlet.ShouldProcess('TARGET')){
        Write-Output "Some Action"
    }
}

According to my testing $Confirm is never present

function Test-ShouldProcess {
    [CmdletBinding(
        SupportsShouldProcess,
        ConfirmImpact = 'High'
    )]
    param(
        [Switch]$Force
    )

    Write-Verbose "`$DebugPreference: $DebugPreference"
    Write-Verbose "`$ConfirmPreference: $ConfirmPreference"
    Write-Verbose "`$Confirm: $Confirm"
    Write-Verbose "`$Confirm: $($null -eq $Confirm ? "NULL": "NONNULL")"

    if ($Force -and -not $Confirm){
        Write-Verbose "Changing `$ConfirmPreference"
        $ConfirmPreference = 'None'
    } else {
        Write-Verbose "XXXX"
    }

    Write-Verbose "`$DebugPreference: $DebugPreference"
    Write-Verbose "`$ConfirmPreference: $ConfirmPreference"
    Write-Verbose "`$Confirm: $Confirm"
    Write-Verbose "`$Confirm: $($null -eq $Confirm ? "NULL": "NONNULL")"

    if ($PSCmdlet.ShouldProcess('TARGET')){
        Write-Output "Some Action"
    }
}

Earlier ticket/work:


The example is really hard to understand and it does not work with strict mode.

Page URL

https://learn.microsoft.com/en-us/powershell/scripting/learn/deep-dives/everything-about-shouldprocess?view=powershell-7.4

Content source URL

https://github.com/MicrosoftDocs/PowerShell-Docs/blob/main/reference/docs-conceptual/learn/deep-dives/everything-about-shouldprocess.md

Author

@sdwheeler

Document Id

530b7644-3211-0f7c-3c63-d9959a72f541

sdwheeler commented 2 months ago

This code works around the Strict Mode issue and should make more sense. The point of testing for $Confirm is to cover the case where the user actually supplies the parameter. By using $PSBoundParameters you can handle the parameter even though the $Confirm variable doesn't exist.

function Test-ShouldProcess {
    [CmdletBinding(
        SupportsShouldProcess,
        ConfirmImpact = 'High'
    )]
    param(
        [Switch]$Force
    )

    if (-not $PSBoundParameters.ContainsKey('Confirm')) {
        Write-Verbose "Setting `$Confirm = $false"
        $Confirm = $false
    } else {
        Write-Verbose "Setting `$Confirm = $($PSBoundParameters.Confirm)"
        $Confirm = $PSBoundParameters.Confirm
    }

    if ($Force -and -not $Confirm){
        $ConfirmPreference = 'None'
    }

    if ($PSCmdlet.ShouldProcess('TARGET')){
        Write-Output "Some Action"
    }
}
jikuja commented 2 months ago

That's new really pedantic and correct, although I think that checking if user has passed -Confirm might be redundant if ShouldProcess handles checking -Confirm and -Confirm:$false internally:

https://github.com/PowerShell/PowerShell/blob/master/src/System.Management.Automation/engine/MshCommandRuntime.cs#L2974-L3000

Testing:

function Test-ShouldProcess {
    [CmdletBinding(
        SupportsShouldProcess,
        ConfirmImpact = 'High'
    )]

    $ConfirmPreference = 'None'
    if ($PSCmdlet.ShouldProcess('TARGET')){
        Write-Output "Some Action"
    }
}

Results:

$ConfirmPreference="Medium" ; Test-ShouldProcess

Confirm
Are you sure you want to perform this action?
Performing the operation "Test-ShouldProcess" on target "TARGET".
[Y] Yes  [A] Yes to All  [N] No  [L] No to All  [S] Suspend  [?] Help (default is "Y"): s

Is there some other use cases to do $ConfirmPreference = 'None' when $Force -and -not $Confirm I just don't see from the code example? Maybe WhatIf?


At the moment the only use case for -Force I see is the short-circuit ShouldContinue with

if($Force -or $PSCmdlet.ShouldContinue('TARGET','OPERATION')){
        Write-Output "Some Action"
}
sdwheeler commented 2 months ago

As you point out, ShouldProcess() tests -Confirm for us so this would be the simpler way of implementing -Force. This would prefer the value of -Confirm over -Force when both are provided, but still allows using -Force to replace the longer form -Confirm:$false.

function Test-ShouldProcess {
    [CmdletBinding(
        SupportsShouldProcess,
        ConfirmImpact = 'High'
    )]
    param(
        [switch]$Force
    )

    if ($Force -and -not $PSBoundParameters.ContainsKey('Confirm')) {
        $ConfirmPreference = 'None'
    }

    if ($PSCmdlet.ShouldProcess('TARGET')) {
        Write-Output "Some Action"
    }
}
jikuja commented 2 months ago

still allows using -Force to replace the longer form -Confirm:$false.

This is the use case I could not see by reading the example code. Might be worth of adding into documentation.