thrift-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [thrift] rlerdorf edited a comment on pull request #2288: THRIFT-5318: Update PHP thrift_protocol extension for PHP 8
Date Wed, 03 Feb 2021 23:48:25 GMT

rlerdorf edited a comment on pull request #2288:
URL: https://github.com/apache/thrift/pull/2288#issuecomment-772844645


   This looks good, @tylerchr. The only thing you missed is that the `$buffer_size` argument
is optional on the two read_binary functions. So in your stub file, provide the default value
from the source which is 8192. Like this:
   ```php
   function thrift_protocol_read_binary(object $protocol, string $obj_typename, bool $strict_read,
int $buffer_size=8192): object {}
   function thrift_protocol_read_binary_after_message_begin(object $protocol, string $obj_typename,
bool $strict_read, int $buffer_size=8192): object {}
   ```
   And regenerate the arginfo.h file, of course.
   
   You can also add `/** @generate-function-entries */` to your stub file to get it to generate
the function entries and pull those out of the `.cpp` file, but it isn't necessary.
   
   And another optional thing. To address your initial issue of the arginfo failing with PHP
7.2 and earlier, you can macro your way around that issue using:
   ```
   #ifndef ZEND_ARG_INFO_WITH_DEFAULT_VALUE
   #define ZEND_ARG_INFO_WITH_DEFAULT_VALUE(pass_by_ref, name, default_value) \
   	ZEND_ARG_INFO(pass_by_ref, name)
   #endif
   
   #if PHP_VERSION_ID < 70200
   #undef ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX
   #define ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(name, return_reference, required_num_args,
class_name, allow_null) \
   	static const zend_internal_arg_info name[] = { \
   		{ (const char*)(zend_uintptr_t)(required_num_args), ( #class_name ), 0, return_reference,
allow_null, 0 },
   #endif
   
   #ifndef ZEND_BEGIN_ARG_WITH_RETURN_OBJ_INFO_EX
   # define ZEND_BEGIN_ARG_WITH_RETURN_OBJ_INFO_EX(name, return_reference, required_num_args,
class_name, allow_null) \
   	ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(name, return_reference, required_num_args, class_name,
allow_null)
   #endif
   
   #ifndef ZEND_BEGIN_ARG_WITH_RETURN_TYPE_MASK_EX
   # define ZEND_BEGIN_ARG_WITH_RETURN_TYPE_MASK_EX(name, return_reference, num_args, type)
\
   	ZEND_BEGIN_ARG_INFO_EX(name, 0, return_reference, num_args)
   #endif
   
   #ifndef ZEND_BEGIN_ARG_WITH_RETURN_OBJ_TYPE_MASK_EX
   # define ZEND_BEGIN_ARG_WITH_RETURN_OBJ_TYPE_MASK_EX(name, return_reference, required_num_args,
class_name, type) \
   	ZEND_BEGIN_ARG_INFO_EX(name, 0, return_reference, required_num_args)
   #endif
   
   #ifndef ZEND_ARG_TYPE_MASK
   # define ZEND_ARG_TYPE_MASK(pass_by_ref, name, type_mask, default_value) \
   	ZEND_ARG_TYPE_INFO(pass_by_ref, name, 0, 0)
   #endif
   
   #ifndef ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE
   # define ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(pass_by_ref, name, type_hint, allow_null,
default_value) \
   	ZEND_ARG_TYPE_INFO(pass_by_ref, name, type_hint, allow_null)
   #endif
   ```
   But again, not required.
   
   I reworked your patch slightly. This compiles cleanly with arginfo back to PHP 7.0 - https://github.com/rlerdorf/thrift/commit/a95df2539d9434dd5ac86a832e1c3cfcfa4c6097


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



Mime
View raw message