assert icon indicating copy to clipboard operation
assert copied to clipboard

alnum() misleading, should use ctype_alnum()

Open webmozart opened this issue 10 years ago • 11 comments

Currently, Assertion::alnum() tests that a value consists of alphabetic and numeric characters only and starts with a letter. I think this is misleading, as it is different to what ctype_alnum() does.

I think that alnum() should be changed to use ctype_alnum() internally, moreover helpers for the other ctype_*() functions should be implemented.

We can add another helper testing that a string starts with a letter: startsWithLetter (using ctype_alpha($string[0]) internally) - analogous to startsWith

What do you think?

webmozart avatar Dec 18 '14 14:12 webmozart

ctype is locale dependent, all our code does not depend on environment right now.

beberlei avatar Dec 18 '14 15:12 beberlei

Really? Got to do some research then.

webmozart avatar Dec 18 '14 15:12 webmozart

@webmozart See http://php.net/manual/en/function.ctype-alnum.php its reference to "default locale" and the link to setlocale.

beberlei avatar Dec 19 '14 08:12 beberlei

I agree with @webmozart that this is confusing. I would expect that an alphanumeric check would mean that something like 123abc would also pass because the content of it is alphanumeric.

Maybe the check that it starts with a letter could be removed @beberlei?

mdavis1982 avatar Jul 20 '15 15:07 mdavis1982

@mdavis1982 as far as I see, the problem is that this would break backwards compatibility of the library. You can find a very similar library where this and some other small issues are fixed here though: https://github.com/webmozart/assert

webmozart avatar Jul 20 '15 15:07 webmozart

@webmozart Of course. This could be fixed and tagged as a different version though? I'd like to contribute to the library to fix issues such as these (I'm going through all of the issues now). Do you think you could possibly contribute some of your other assertions to this library?

mdavis1982 avatar Jul 20 '15 15:07 mdavis1982

@mdavis1982 That's what I tried, but @beberlei said he didn't want to make breaking changes.

webmozart avatar Jul 20 '15 15:07 webmozart

its not really a breaking change, because we are making the validation less strict. Making it stricter would be a problem though. I consider this a bug, if alnum actually means starting with 123 is allwoed as well.

beberlei avatar Aug 21 '15 17:08 beberlei

Hi,

I'd like to draw your attention to this issue again since we're using the library a lot in our applications and find the nuance problematic when using alnum check from time to time. It says that "123" is not alphanumeric which, of course, is not true and is simply a bug. This forces us to write unnecessary regexes or manually use ctype_alnum

Any plans on fixing it soon?

ghost avatar Sep 26 '19 10:09 ghost

Hi. I'm happy to accept PRs on this. I'm sort of the de-facto maintainer now, but the library is still @beberlei's.

As for BCs, I'm using semantic versioning (and nearly fully understanding it!!! Yay! Please excuse the hiccough in the V3.2 version .. should all be sorted now!), so a v4 and a clear differentiation would be nice, but things would really need to be tested as we don't all work in all environments.

rquadling avatar Oct 10 '19 10:10 rquadling

Also, as you are able to subclass the Assert library now, you may find it "simpler" to implement your own assertions on top of Assert.

rquadling avatar Oct 10 '19 10:10 rquadling