Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(install-env): Avoid automatic expansion of %% in env #44

Merged
merged 4 commits into from
Feb 28, 2023

Conversation

WHYBBE
Copy link
Contributor

@WHYBBE WHYBBE commented Feb 22, 2023

Use a method of obtaining environment variables that does not expand %%, while maintaining the original type of environment variables, such as REG_EXPAND_SZ, REG_SZ.

Same issue related to ScoopInstaller/Scoop#5394
Some important content in ScoopInstaller/Scoop#5395

@rashil2000
Copy link
Member

I think we can avoid making another function by just doing:

function Get-Env {
    param(
        [String] $name,
        [Switch] $global
    )

    $RegisterKey = if ($global) {
        Get-Item -Path 'HKLM:\SYSTEM\CurrentControlSet\Control\Session Manager'
    } else {
        Get-Item -Path 'HKCU:'
    }

    $EnvRegisterKey = $RegisterKey.OpenSubKey('Environment')
    $RegistryValueOption = [Microsoft.Win32.RegistryValueOptions]::DoNotExpandEnvironmentNames
    $EnvRegisterKey.GetValue($name, $null, $RegistryValueOption)
}

function Set-Env {
    param(
        [String] $name,
        [String] $val,
        [Switch] $global
    )

    $RegisterKey = if ($global) {
        Get-Item -Path 'HKLM:\SYSTEM\CurrentControlSet\Control\Session Manager'
    } else {
        Get-Item -Path 'HKCU:'
    }

    $EnvRegisterKey = $RegisterKey.OpenSubKey('Environment', $true)
    $RegistryValueKind = if ($EnvRegisterKey.GetValue($name)) {
        $EnvRegisterKey.GetValueKind($name)
    } else {
        [Microsoft.Win32.RegistryValueKind]::ExpandString
    }
    $EnvRegisterKey.SetValue($name, $val, $RegistryValueKind)
}

@WHYBBE
Copy link
Contributor Author

WHYBBE commented Feb 22, 2023

I think we can avoid making another function by just doing:

function Get-Env {
    param(
        [String] $name,
        [Switch] $global
    )

    $RegisterKey = if ($global) {
        Get-Item -Path 'HKLM:\SYSTEM\CurrentControlSet\Control\Session Manager'
    } else {
        Get-Item -Path 'HKCU:'
    }

    $EnvRegisterKey = $RegisterKey.OpenSubKey('Environment')
    $RegistryValueOption = [Microsoft.Win32.RegistryValueOptions]::DoNotExpandEnvironmentNames
    $EnvRegisterKey.GetValue($name, $null, $RegistryValueOption)
}

function Set-Env {
    param(
        [String] $name,
        [String] $val,
        [Switch] $global
    )

    $RegisterKey = if ($global) {
        Get-Item -Path 'HKLM:\SYSTEM\CurrentControlSet\Control\Session Manager'
    } else {
        Get-Item -Path 'HKCU:'
    }

    $EnvRegisterKey = $RegisterKey.OpenSubKey('Environment', $true)
    $RegistryValueKind = if ($EnvRegisterKey.GetValue($name)) {
        $EnvRegisterKey.GetValueKind($name)
    } else {
        [Microsoft.Win32.RegistryValueKind]::ExpandString
    }
    $EnvRegisterKey.SetValue($name, $val, $RegistryValueKind)
}

Originally, I wanted to do the same, but the definition of RegisterKey was repeated. I think it was not good. At the same time, I don't want to add another parameter to the original Get-Env, which is the same as the env in Scoop, so I modified it like this. What do you think?

install.ps1 Show resolved Hide resolved
@rashil2000
Copy link
Member

Originally, I wanted to do the same, but the definition of RegisterKey was repeated. I think it was not good.

RegisterKey variable here has function scope, so they won't interfere with each other.

At the same time, I don't want to add another parameter to the original Get-Env, which is the same as the env in Scoop, so I modified it like this. What do you think?

Get-Env in the installer still has only 2 parameters, right? (same as before)

@WHYBBE
Copy link
Contributor Author

WHYBBE commented Feb 23, 2023

Originally, I wanted to do the same, but the definition of RegisterKey was repeated. I think it was not good.

RegisterKey variable here has function scope, so they won't interfere with each other.

At the same time, I don't want to add another parameter to the original Get-Env, which is the same as the env in Scoop, so I modified it like this. What do you think?

Get-Env in the installer still has only 2 parameters, right? (same as before)

Yes, there will be no interference between them, but I am just afraid that there will be a next revision, so the next revision will have to change two places, which doesn't look very good.

No, what I mean by that sentence is to make GetEnv the same as the env function in Scoop, and use the third parameter to distinguish whether it is get or set, but it still needs to change the name, which is not very good.

All in all, hurry up and merge these two PRs. I don’t think there will be a better solution if we continue to discuss. Just choose a method that everyone agrees with. (I think the code you provided above is ok, as long as others think it's ok too.)

@r15ch13
Copy link
Member

r15ch13 commented Feb 23, 2023

Adding Get-Env and Set-Env as separate functions should be preferred over copying over the old hacky way with $val -eq '__get'.

@chawyehsu
Copy link
Member

I'll review this in these two days. Thanks for the contribution.

@chawyehsu
Copy link
Member

The implementation looks good to me. CI code linting failed because of the name of the newly introduced function. To fix it either change to use a verb that does not require UseShouldProcessForStateChangingFunctions or implement ShouldProcess for Set-Env.

@WHYBBE WHYBBE changed the title fix(install-env): Avoid automatic expansion of %% in env fix(install-env): Avoid automatic expansion of %% in env Feb 26, 2023
@chawyehsu
Copy link
Member

Put is not an approved Cmdlet verb, you may use one from the Get-Verb list. Use might be a proper one in this case.

@WHYBBE
Copy link
Contributor Author

WHYBBE commented Feb 27, 2023

Put is not an approved Cmdlet verb, you may use one from the Get-Verb list. Use might be a proper one in this case.

I chose Write, and if other people think it is not suitable, you can also give some suggestions from the few verbs I listed.

e.g., Enter, Join, Write, Edit, Publish, Save, Install, Register, Submit...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants