Re: hasType() for internal function parameters?

On Mon, Jun 22, 2015 at 6:57 PM, Rasmus Lerdorf <rasmus@lerdorf.com> wrote: > On 06/22/2015 07:54 PM, Stanislav Malyshev wrote: >> Hi! >> >> So I tried to remove the checks for ZEND_ACC_HAS_TYPE_HINTS: >> https://github.com/php/php-src/pull/1354 >> >> Turns out there is a lot of tests that assume function with wrong >> arguments throws Error, but ZPP and type checks work differently - ZPP >> first checks argument number (and doesn't throw if number is wrong but >> instead creates warning), while type checks (that shouldn't really >> happen on internals but do) check types first and not check argument number. >> >> So I wonder which way is the best to proceed here. Looks like this comes >> from 5.6 where internal function types were verified in executor: >> >> https://github.com/php/php-src/blob/PHP-5.6/Zend/zend_vm_def.h#L1974 >> >> I'm not sure what to do with it - on one side, I think ZPP should handle >> it for internal functions, otherwise we're doing the same work twice. On >> the other side, that would be pretty substantial, even if maybe correct, >> BC break. Should we keep the ZEND_ACC_HAS_TYPE_HINTS type checks and >> accept the fact that this way we're checking everything twice, or should >> be clean it up? > > Is it a BC break against real code though? Fixing tests isn't a big > deal. What kind of real code would break by turning calls with the wrong > number of args from an error to a warning? > > I suppose it is a bit late in the game to fix this, but it really does > seem very broken to the point where I wonder if we shouldn't just > disable reflection on internal functions entirely since the arginfo is > so woefully incomplete. Plus the double-checking is super messy. One thing I really want to work towards in the future is consistency between user-land and internal functions. There are currently multiple observable differences. Maybe it would be best to post-pone these changes until 8.0 where we can do a large consistency pass and handle it then.