Ensure -PipelineVariable is set for all output from script cmdlets #12766
Conversation
|
Codefactor and Codacy issues are with untouched code areas / not applicable. 🙂 |
vexx32
force-pushed
the
Functions/PipelineVariableFix
branch
2 times, most recently
from
1ef6559 to
3a12833
Compare
|
I think this falls into a bucket 3 breaking change and I think we should take the change. |
TravisEz13
added
CL-BreakingChange
|
@PowerShell/powershell-committee reviewed this, we agree that the previous behavior was broken and falls under bucket 3 as a breaking change, so approve the intended new behavior |
SteveL-MSFT
added
Committee-Reviewed
There was a problem hiding this comment.
i'm a little concerned that there isn't enough validation. Can you think of additional edges here that we should be poking?
There was a problem hiding this comment.
There are probably numerous odd cases where this might crop up, but I can't think of any that are really different from the test case here. At the very least, this does cover the test case that we know is currently broken.
I've not seen any other cases crop up myself, nor had someone drop in the community channels with an issue that came down to this. I suspect that that's more due to -PipelineVariable being a bit more of a niche concept from the start, though.
Without knowing for sure about other broken cases, the best I can do is test a few other cases with multiple script cmdlets and various mixes of script / compiled cmdlets to see if the logic breaks down anywhere.
The only real unknown where I don't know what to actually expect is what is meant to happen if multiple cmdlets in the same pipeline declare the same pipeline variable. I'm not sure the expected behaviour there is even recorded anywhere. Best guess - the nearest command to the variable's usage will have its output in there at any given point...
If anyone else has cases they know to be broken / likely to break, I'd appreciate having those as well.
ghost
added
Waiting on Author
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
|
@JamesWTruher can you respond to @vexx32 |
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
|
@JamesWTruher please update your review / let me know if you have additional cases to cover with tests. Thanks! 💖 |
JamesWTruher
left a comment
JamesWTruher
left a comment
There was a problem hiding this comment.
we can always add more validation if we find additional cases.
Previously, for script functions -PipelineVariable would only take effect for the duration of the first output, and then stopped working. Borrowed some code from the compiled cmdlets' implementation which has always worked just fine. This appears to resolve the issue. :bug: Fix NRE with dynamicparam Dynamicparam blocks also do enter/exit scope, but seem to lack a state. Workaround is to check for null state and fallback to previous behaviour for dynamicparam blocks. This should not cause any issues since output can't be passed from dynamicparam, so -PipelineVariable can't be set from this block.
vexx32
force-pushed
the
Functions/PipelineVariableFix
branch
from
581f2c5 to
7b0a58b
Compare
|
@TravisEz13 is there anything left pending here? 🙂 |
|
Open PRs should not be assigned to milestone, so they are not assigned to the wrong milestone after they are merged. For backport consideration, use a |
|
Merged this so we can include it in the next preview in order to validate it better |
iSazonov
added
the
CL-General
|
🎉 Handy links: |
8 participants
PR Summary
Previously, -PipelineVariable was only set for the first occurence of output from a script cmdlet. This change borrows some code from the handling for compiled commands to ensure -PipelineVariable is always set correctly and should behave as expected even for script cmdlets.
PR Context
This PR resolves #10932, see this issue for further discussion.
We will need to document the bugged behaviour for downlevel versions of PowerShell in docs so that users are aware. Most folks probably won't have noticed it, but people may still run into it from time to time on downlevel versions.
@SteveL-MSFT I'm not 100% sure, but this feels like the kind of fix y'all might want in a servicing release for 7.0. 🙂
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.