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 issue where MS Dynamics CRM #686

Merged
merged 2 commits into from
Jun 13, 2018

Conversation

rkeithhill
Copy link
Contributor

redefines set-content, breaks Start-PSES

Fixes PowerShell/vscode-powershell#1331

I wish we didn't have to module qualify command names like this but
we've been bitten several times now by overridden commands.

One concern I have is testing this due to it needing to be signed.

redefines set-content, breaks Start-PSES

Fixes PowerShell/vscode-powershell#1331

I wish we didn't have to module qualify command names like this but
we've been bitten several times now by overridden commands.
@rjmholt
Copy link
Contributor

rjmholt commented Jun 12, 2018

Wow that's annoying. But it seems necessary...

@rjmholt
Copy link
Contributor

rjmholt commented Jun 12, 2018

Do we need to modify PowerShellEditorServices.psm1 as well?

@rkeithhill
Copy link
Contributor Author

Yes. Good catch! I'll get that in a bit later tonight.

@TylerLeonhardt
Copy link
Member

It might be worth double checking the scripts that we use for remoting as well. psedit and such.

I'm, for the moment, drawing the line at the *-Object commands.

I really hate having to do this but I hate dealing with
externally inflicted bugs even more.
Copy link
Member

@TylerLeonhardt TylerLeonhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - I feel like there should be a PSSA rule on redefining core cmdlets

@rkeithhill
Copy link
Contributor Author

I feel like there should be a PSSA rule on redefining core cmdlets

That's a good idea. You should post that in the PSSA repo.

@rkeithhill
Copy link
Contributor Author

This is relevant to this issue - PowerShell/PowerShell#7040 (comment)

@TylerLeonhardt
Copy link
Member

PowerShell/PSScriptAnalyzer#1023 did the thing!

@rkeithhill rkeithhill merged commit eedfad2 into master Jun 13, 2018
@rkeithhill rkeithhill deleted the rkeithhill/module-qualify-set-content branch June 13, 2018 22:21
@kilasuit
Copy link

Whilst I know this is merged I would reccommend for defensive reasons across all of PSES you Module Qualify commands to ensure no one else breaks your functionality, perhaps in a similar way to how Pester does it

@TylerLeonhardt
Copy link
Member

perhaps in a similar way to how Pester does it

can you elaborate on this?

@rjmholt
Copy link
Contributor

rjmholt commented Sep 26, 2019

can you elaborate on this?

Pester defines a table of safe commands ahead of time because it has to be safe for mocking. That affects startup time and depends on being able to scope a variable. We could do it in a way that suits PSES better, but module-qualifying things seems pretty sufficient. You're asking for trouble if you duplicate a builtin module.

I would reccommend for defensive reasons across all of PSES you Module Qualify commands to ensure no one else breaks your functionality

Precisely what this PR did, and we've done a bit more since. We also fully qualified native utils by path, since people like to install their own. There may still be some outliers left in things like C#-invoked PowerShell, but it's the kind of thing we're shaving down over time.

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.

Powershell Integrated Console Crashes On Startup
4 participants