Skip to content

Revert the PRs that made DBNull.Value and NullString.Value treated as $null#11584

Merged
adityapatwardhan merged 5 commits into
PowerShell:release/v7.0.0-rc.2from
daxian-dbw:revert
Jan 15, 2020
Merged

Revert the PRs that made DBNull.Value and NullString.Value treated as $null#11584
adityapatwardhan merged 5 commits into
PowerShell:release/v7.0.0-rc.2from
daxian-dbw:revert

Conversation

@daxian-dbw

@daxian-dbw daxian-dbw commented Jan 14, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

PR Summary

Revert the PRs that made DBNull.Value and NullString.Value treated as $null:

Also update code for Coalescing and Null-conditional operators to use 'IsNull' instead.

PR Checklist

PoshChan
PoshChan reviewed Jan 14, 2020
View reviewed changes
Comment thread src/System.Management.Automation/engine/LanguagePrimitives.cs
adityapatwardhan
adityapatwardhan approved these changes Jan 14, 2020
View reviewed changes

@adityapatwardhan adityapatwardhan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approved with nitpicks

Comment thread src/System.Management.Automation/engine/LanguagePrimitives.cs
Comment thread src/System.Management.Automation/engine/LanguagePrimitives.cs
Comment thread src/System.Management.Automation/engine/LanguagePrimitives.cs
Comment thread src/System.Management.Automation/engine/LanguagePrimitives.cs
@daxian-dbw

daxian-dbw commented Jan 14, 2020

Copy link
Copy Markdown
Member Author

@adityapatwardhan You just need to review the 3rd commit. The previous 2 are from git revert those 2 PRs.

PoshChan
PoshChan reviewed Jan 14, 2020
View reviewed changes
Comment thread src/System.Management.Automation/engine/LanguagePrimitives.cs
PoshChan
PoshChan reviewed Jan 14, 2020
View reviewed changes
Comment thread src/System.Management.Automation/engine/LanguagePrimitives.cs
@daxian-dbw

daxian-dbw commented Jan 14, 2020

Copy link
Copy Markdown
Member Author

@adityapatwardhan @TravisEz13 Also, the windows build failed at the packaging phase with a "calling method on null-value" error:

You cannot call a method on a null-valued expression.
at Invoke-CIFinish, D:\a\1\s\tools\ci.psm1: line 452
at <ScriptBlock>, D:\a\_temp\00413339-7785-4f63-ab8a-bab4a33c6cbe.ps1: line 4
at <ScriptBlock>, <No file>: line 1
You cannot call a method on a null-valued expression.
At D:\a\1\s\tools\ci.psm1:452 char:9
+         $previewLabel = $previewVersion[1].replace('.','')
+         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : InvalidOperation: (:) [], RuntimeException
    + FullyQualifiedErrorId : InvokeMethodOnNull

See https://dev.azure.com/powershell/PowerShell/_build/results?buildId=43557&view=logs&j=1057fe42-c88b-5deb-88ad-59245c714665&t=1888043b-254d-5761-80ec-47d0d8adc59a

@SteveL-MSFT SteveL-MSFT added this to the GA-consider milestone Jan 15, 2020
@adityapatwardhan adityapatwardhan merged commit d99b325 into PowerShell:release/v7.0.0-rc.2 Jan 15, 2020
@daxian-dbw daxian-dbw deleted the revert branch January 15, 2020 00:47
@adityapatwardhan

adityapatwardhan commented Jan 15, 2020

Copy link
Copy Markdown
Member

@SteveL-MSFT The PR is targetting RC.2 branch so it is auto approved for RC.

Merged this even though there were failures in Windows packaging as we have differences in content in metadata.json in RC.2 branch. It was fixed to unblock CI with PR #11372
We do not need to take this change to the RC.2 branch.

@daxian-dbw daxian-dbw added CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log CL-Engine Indicates that a PR should be marked as an engine change in the Change Log and removed CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log labels Jan 15, 2020
@ghost

ghost commented Jan 16, 2020

Copy link
Copy Markdown

🎉v7.0.0-rc.2 has been released which incorporates this pull request.:tada:

Handy links:

@ili101

ili101 commented Jan 17, 2020

Copy link
Copy Markdown

Why was this reverted?
Is this final or are you planing to change it again in the future?
BTW probably should have been listed under the "Breaking Changes" category in the v7.0.0-rc.2 Release note.

@vexx32

vexx32 commented Jan 17, 2020 •
edited
Loading

Copy link
Copy Markdown
Collaborator

@ili101 the change was never included in a stable release, so reverting it is not a breaking change. Please see this issue for discussion -- depending on what is decided by the PS team and community, the change may be reintroduced, likely with some alterations if it is, in a later release, likely at least 7.1 🙂

daxian-dbw added a commit to daxian-dbw/PowerShell that referenced this pull request Jan 21, 2020
@daxian-dbw
Revert the PRs that made DBNull.Value and NullString.Value treate…
b90e530 
…d as `$null` (PowerShell#11584)
@daxian-dbw daxian-dbw mentioned this pull request Jan 21, 2020
Merged
14 tasks
@TravisEz13 TravisEz13 mentioned this pull request Oct 12, 2022
Merged
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adityapatwardhan adityapatwardhan adityapatwardhan approved these changes
@anmenaga anmenaga Awaiting requested review from anmenaga
@TravisEz13 TravisEz13 Awaiting requested review from TravisEz13
+1 more reviewer
@PoshChan PoshChan PoshChan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@adityapatwardhan adityapatwardhan

Labels

CL-Engine Indicates that a PR should be marked as an engine change in the Change Log

Projects

None yet

Milestone

7.0.0-rc.2

Development

Successfully merging this pull request may close these issues.

6 participants

@daxian-dbw @adityapatwardhan @ili101 @vexx32 @PoshChan @SteveL-MSFT