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

Permit Type-Hinting from extensions #107

Closed
wants to merge 2 commits into
base: master
from

Conversation

3 participants
@ofavre

ofavre commented Jun 14, 2012

All extensions use ZEND_ARG_INFO, ZEND_ARG_OBJ_INFO or ZEND_ARG_ARRAY_INFO, but none use ZEND_ARG_TYPE_INFO, because of a bug.
The advantage of using the latter over the primer is that it permits giving a type hint to PHP.
The only impact of a type hint should be visible in the Reflection extension, as it merely is a hint.

However Zend emits a bug systematically if the type info is given: PHP Fatal error: Unknown typehint in file.php on line 1.
The first commit fixes this bug, enabling widespread use of ZEND_ARG_TYPE_INFO.

Currently only ReflectionParameter::__toString() permits getting back this information inside a PHP script, forcing ugly string analysis to get it back.
The second commit makes the type hint information accessible through the ReflectionParameter::getTypeHint() method, which returns, like gettype(), the string representation of the given type hint, or NULL if none is provided (yet).

ofavre added some commits Jun 14, 2012

Permit ZEND_ARG_TYPE_INFO with other types than IS_ARRAY and IS_CALLABLE
Zend/zend_execute.c:zend_verify_arg_type() enforces some type hints:
- wrong object class (assumes an implied type hint to IS_OBJECT)
- not an array
- not a callable
- disallowed nullness for the previous cases
- for any other type hints, raise an error
Those hints are used in the Reflection API
(currently only in __toString as there is no getter).

Zend/zend_API.c:zend_parse_arg_impl() uses a provided format string
and performs automatic convertion if possible.

The zend_verify_arg_type() function is called by the Zend VM
at some low level. As PHP wants more flexibility, it should not enforce
type hinting much.
Hence, I left the current rules, but removed the last one that raises
an error for non object, array or callable types.
As a consequence, ARG_INFOs of PHP_FE can now be more precise and
helpful.
See #45569 - ReflectionParameter::getTypeHint() function
Type hinting is an available information in ReflectionParameter,
however it is only used in __toString()!

Made that information available through the getTypeHint() function,
which returns the string representation of the type hint constant.

Turned ReflectionParameter::export() and __construct()
to use ZEND_ARG_TYPE_INFO() instead of ZEND_ARG_INFO(),
in order to register the actual type hinting.

Added a test to ensure getTypeHint() works fine with type hinted
function and returns NULL for non type hinted ones.

See request #45569, the second part.
@nikic

This comment has been minimized.

Show comment
Hide comment
@nikic

nikic Jun 14, 2012

Member

I don't like the idea of adding this without the corresponding userland type hinting support. If that would be added in the future then you could easily get problems because old extension code relies on scalar typehints to not do anything.

Member

nikic commented Jun 14, 2012

I don't like the idea of adding this without the corresponding userland type hinting support. If that would be added in the future then you could easily get problems because old extension code relies on scalar typehints to not do anything.

@ofavre

This comment has been minimized.

Show comment
Hide comment
@ofavre

ofavre Jun 18, 2012

Scalar typehints imply an automatic convertion too, but not exactly in the same place.
As written in the first commit's log, Zend/zend_API.c:zend_parse_arg_impl() is the place where automatic conversion is performed, only z arguments (no corresponding IS_* type) are kept untouched.
The patched place is more low level, closer to the Zend VM, and PHP do not which to enforce much type hinting there to make use of the automatic conversions.

The only thing the first commit prevents, is extensions to declare uncallable functions by using scalar typehints.

ofavre commented Jun 18, 2012

Scalar typehints imply an automatic convertion too, but not exactly in the same place.
As written in the first commit's log, Zend/zend_API.c:zend_parse_arg_impl() is the place where automatic conversion is performed, only z arguments (no corresponding IS_* type) are kept untouched.
The patched place is more low level, closer to the Zend VM, and PHP do not which to enforce much type hinting there to make use of the automatic conversions.

The only thing the first commit prevents, is extensions to declare uncallable functions by using scalar typehints.

@php-pulls

This comment has been minimized.

Show comment
Hide comment
@php-pulls

php-pulls Aug 26, 2012

Comment on behalf of lstrojny at php.net:

While I would like to see this change merged, I think having a short RFC (https://wiki.php.net/rfc/voting) would be the right thing to do here as it might be a massive change. From my POV the RFC should discuss:

  • What kind of APIs do we want to provide to extension authors? (your patch does that)
  • How is this change forward-compatible to possibly upcoming type hints?
  • What do we want to do with existing code? Do we want to port it to correctly hint types as well?

Thanks!

php-pulls commented Aug 26, 2012

Comment on behalf of lstrojny at php.net:

While I would like to see this change merged, I think having a short RFC (https://wiki.php.net/rfc/voting) would be the right thing to do here as it might be a massive change. From my POV the RFC should discuss:

  • What kind of APIs do we want to provide to extension authors? (your patch does that)
  • How is this change forward-compatible to possibly upcoming type hints?
  • What do we want to do with existing code? Do we want to port it to correctly hint types as well?

Thanks!

@php-pulls php-pulls closed this Aug 26, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment