Johannes,

> Some comments on the "error behavior" part:
>
>    E_WARNING - When CRYPT is not included in core (was disabled
>    compile-time, or is listed in disabled_functions declaration)
>
> Disabling a different function should have no effect. This is not
> intuitive. If crypt is a dependency and is not available this function
> shouldn't be available either.

Hrm. Well, then I guess I could re-implement against crypt internally.
That would take either a slight re-implementation of the crypt()
internals, or slight refactoring of the PHP_FUNCTION(crypt) function
to enable c calls to it (even if it's disabled).

I don't like the concept of core functions disappearing if they are
not implemented. I think that does nothing but make it harder on the
developers (now having to inject a function_exists(), etc).
Additionally, since this is a security issue, I think that always
defining the function is the better approach. Otherwise, you can wind
up in a situation where someone else comes in and implements function
password_verify($password, $hash) { return true; }, which would be all
sorts of bad...  I can see the static linking to the function (instead
of the dynamic call that's there), So in this case, I personally think
the warning is appropriate.

>    E_WARNING - When supplied an incorrect number of arguments.
>    E_WARNING - When supplied a non-string first parameter (password)
>
> This should follow common semantics of zend_parse_parameters(... "s").
> i.e. it has to support objects with __toString(). Also other scalars are
> fine. (if they can be casted to string)
>
>    E_WARNING - If a non-string salt option is provided
>
> As above.

Yeah, I guess that's fair. Is there a macro or function like
Z_REPRESENTABLE_AS_STRING_P() or something? To make it easier to
check?

>    If any error is raise, false is returned by the function.
>
> In http://de.php.net/functions.internal it is documented that internal
> functions return NULL on error during parameter parsing. New exceptions
> for that should have a good reason.

The good reason is consistency. Otherwise there would be three return
values, `null`, `false` and `true` for password_verify(). Therefore, a
check of `false === password_verify($foo)` would actually fail
inadvertantly.

My opinion is that it should do what's appropriate.  The other 2 I can
live with returning null (although I disagree with it significantly),
but password_verify I think really should only ever return a
boolean...

> These things are all minor and you might consider them bad, but then
> change it globally, not by adding new inconsistencies.

Fair enough...

Thanks,

Anthony

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

Reply via email to