assert icon indicating copy to clipboard operation
assert copied to clipboard

Inconsistency between Assert::alpha and Assert::alnum

Open zerkms opened this issue 5 years ago • 5 comments

The documentation states

alpha($value, $message = '') | Check that a string contains letters only
alnum($value, $message = '') | Check that a string contains letters and digits only

But then implementations are:

    public static function alpha($value, $message = '')
    {
        static::string($value);

        $locale = \setlocale(LC_CTYPE, 0);
        \setlocale(LC_CTYPE, 'C');
        $valid = !\ctype_alpha($value);
        \setlocale(LC_CTYPE, $locale);

        if ($valid) {
            static::reportInvalidArgument(\sprintf(
                $message ?: 'Expected a value to contain only letters. Got: %s',
                static::valueToString($value)
            ));
        }
    }

    public static function alnum($value, $message = '')
    {
        $locale = \setlocale(LC_CTYPE, 0);
        \setlocale(LC_CTYPE, 'C');
        $valid = !\ctype_alnum($value);
        \setlocale(LC_CTYPE, $locale);

        if ($valid) {
            static::reportInvalidArgument(\sprintf(
                $message ?: 'Expected a value to contain letters and digits only. Got: %s',
                static::valueToString($value)
            ));
        }
    }

Note that alpha accepts mixed and checks if it's a string explicitly, but alnum expects it's a string already and does not make a runtime check.

I think both documentation and implementation should be changed to be consistent.

zerkms avatar May 30 '20 00:05 zerkms

/cc @Ocramius

zerkms avatar May 30 '20 00:05 zerkms

IMO, changing these would be a BC break: it's worth doing it in a 2.0, but otherwise leave the inconsistency there and we keep the issue Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

On Sat, May 30, 2020 at 2:41 AM Ivan Kurnosov [email protected] wrote:

/cc @Ocramius https://github.com/Ocramius

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/webmozart/assert/issues/197#issuecomment-636248391, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABFVECGTUG7DO2RG3XGH6DRUBI2FANCNFSM4NOORCRA .

Ocramius avatar May 30 '20 01:05 Ocramius

Indeed, would be helpful if milestone was added here then.

zerkms avatar May 30 '20 02:05 zerkms

ctype is funny in that it just returns false if the entered value is not a string. Even an object that could be cast to a string is not.

So the end result should be the same (except for integers between -128 and 255)

BackEndTea avatar Jun 11 '20 16:06 BackEndTea

Yep, but isn't the general idea behind assertion libraries (in general) is: we pass you any arbitrary rubbish and if it's not what we expect - throw.

As of now - if you want to check it's an alnum in a variable - you must assert it's a string first.

zerkms avatar Jun 11 '20 21:06 zerkms