Skip to content

Permit Type-Hinting from extensions #107

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

Closed
wants to merge 2 commits into from
Closed

Conversation

ofavre
Copy link

@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).

Olivier Favre added 2 commits June 14, 2012 12:10
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.
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
Copy link
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
Copy link
Author

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

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!

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

Successfully merging this pull request may close these issues.

3 participants