From notifications-return-991-archive-asf-public=cust-asf.ponee.io@thrift.apache.org Wed Feb 3 23:48:26 2021 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mxout1-ec2-va.apache.org (mxout1-ec2-va.apache.org [3.227.148.255]) by mx-eu-01.ponee.io (Postfix) with ESMTPS id 437AB18062B for ; Thu, 4 Feb 2021 00:48:26 +0100 (CET) Received: from mail.apache.org (mailroute1-lw-us.apache.org [207.244.88.153]) by mxout1-ec2-va.apache.org (ASF Mail Server at mxout1-ec2-va.apache.org) with SMTP id 836854183E for ; Wed, 3 Feb 2021 23:48:25 +0000 (UTC) Received: (qmail 81495 invoked by uid 500); 3 Feb 2021 23:48:25 -0000 Mailing-List: contact notifications-help@thrift.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@thrift.apache.org Delivered-To: mailing list notifications@thrift.apache.org Received: (qmail 81486 invoked by uid 99); 3 Feb 2021 23:48:25 -0000 Received: from ec2-52-202-80-70.compute-1.amazonaws.com (HELO gitbox.apache.org) (52.202.80.70) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 03 Feb 2021 23:48:25 +0000 From: =?utf-8?q?GitBox?= To: notifications@thrift.apache.org Subject: =?utf-8?q?=5BGitHub=5D_=5Bthrift=5D_rlerdorf_edited_a_comment_on_pull_reques?= =?utf-8?q?t_=232288=3A_THRIFT-5318=3A_Update_PHP_thrift=5Fprotocol_extensio?= =?utf-8?q?n_for_PHP_8?= Message-ID: <161239610519.15632.8973815761959695393.asfpy@gitbox.apache.org> Date: Wed, 03 Feb 2021 23:48:25 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit References: In-Reply-To: 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