Compat bug in System.ComponentModel.Annotations tests for validating PhoneAttributes

[hughbe]

The following phone numbers (obviously invalid) are valid with .NET core, and invalid with netfx.

public static IEnumerable<object[]> TestData()
{
    yield return new object[] { "+4+2+5+-+5+5+5+-+1+2++1+2++" };
    yield return new object[] { "425-555-1212    " };
    yield return new object[] { " \r \n 1  \t " };
    yield return new object[] { "1-.()" };
    yield return new object[] { "(425555-1212" };
    yield return new object[] { ")425555-1212" };
}


[Theory]
[MemberData(nameof(TestData))]
public static void Validate(string value)
{
    Assert.False(new PhoneAttribute().IsValid(value));
}

On netfx: test passes
On netcoreapp: test fails

This regression was likely caused by dotnet/corefx#4319

I suggest this is triaged as a .NET core bug.

[danmoseley]

Since we almost bought the code over verbatim, this is curious?

[karelz]

@divega @lajones @ajcvickers can you please comment?

Great catch @hughbe! Out of curiosity, how did you catch it? A new test you just added? Or did you do netfx test runs on your own? (in which case I would be curious why we missed it in our runs)

[hughbe]

@karelz

I ran the netfx tests on my own:

cd src/System.ComponentModel.Annotations/tests
msbuild /T:BuildAndTest /P:TargetGroup=netfx

I've submitted dotnet/corefx#17874 to get the tests passing, and fix #20883

@danmosemsft

I suspect it's due to the PR feedback commit for the security bulletin fix that was copied verbatim from netfx:
dotnet/corefx@eb43ce0

Or could've been when porting-to-core. There have only been a couple of changes to PhoneAttribute in corefx: https://github.com/dotnet/corefx/commits/master/src/System.ComponentModel.Annotations/src/System/ComponentModel/DataAnnotations/PhoneAttribute.cs

[karelz]

@danmosemsft @joperezr it is a bit surprising to me that we didn't catch it in our recent Desktop runs, yet @hughbe did catch it in his. Any idea why? Any differences in how we run Desktop tests? Could there be more?

[danmoseley]

@karelz I assume we did/would catch it. We just didn't open al lthe desktop issues yet.

[karelz]

Ah, I thought we're mostly done already. OK, that makes sense now.

[lajones]

I agree with #20883, but this one worries me a bit more. We had very long discussions around the deliberate change to allow what were previously invalid phone numbers to appear as valid according to this attribute (along with similar discussions for EmailAddressAttribute and UrlAttribute). Partly this was motivated by the security issue (which, of course, we must still address) but partly it was a feeling that fully checking the regular expressions for what is allowed and not allowed was overkill for something that was mainly meant to prevent people making typing errors. I think we should include @Eilon and @danroth27 in this discussion as they were there for the original discussion.

[Eilon]

This change was definitely by design because of concerns with the rather high degree of complexity of the regexes that were used, compared to the rather low value that they provide.

Given that that change shipped some time ago, changing it again would of course be another breaking change and/or interop challenge.

I personally much prefer the new behavior because it seems a lot closer to what a person would expect such a validation attribute to do The old attributes were more difficult to describe to a human.

[hughbe]

Sure - it's just a bit odd as I thought the .NET framework already had this hotfix so would have the same behaviour as .NET core. Let's close this then as by-design?