Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistency between Assert::alpha and Assert::alnum #197

Open
zerkms opened this issue May 30, 2020 · 5 comments
Open

Inconsistency between Assert::alpha and Assert::alnum #197

zerkms opened this issue May 30, 2020 · 5 comments

Comments

@zerkms
Copy link
Contributor

zerkms commented May 30, 2020

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
Copy link
Contributor Author

zerkms commented May 30, 2020

/cc @Ocramius

@Ocramius
Copy link
Contributor

Ocramius commented May 30, 2020 via email

@zerkms
Copy link
Contributor Author

zerkms commented May 30, 2020

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

@BackEndTea
Copy link
Collaborator

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)

@zerkms
Copy link
Contributor Author

zerkms commented Jun 11, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants