Uploaded image for project: 'Thrift'
  1. Thrift
  2. THRIFT-1453

Don't change types of arguments when serializing with thrift php extension

Details

    • Improvement
    • Status: Closed
    • Minor
    • Resolution: Fixed
    • None
    • 0.9
    • C++ - Library
    • None

    Description

      From c630a1a3d7485678176c53e365d20b38db17f8bd Mon Sep 17 00:00:00 2001
      From: Dan 'Sabretooth' Weatherford <dweatherford@fb.com>
      Date: Fri, 25 Jun 2010 03:39:58 +0000
      Subject: [PATCH 4/5] Don't change types of arguments when serializing with thrift php extension

      Summary:
      If the PHP variable type is not compatible with the desired serialization type,
      we'll copy the variable before doing the conversion. If it is compatible, this
      just adds a little refcount fiddling, so it shouldn't slow down much.

      Test Plan:
      Passed a bool, int, and double to functions expecting strings, and verified
      that the types were unchanged afterwards. (Previously, they'd all end up as
      strings.)

      DiffCamp Revision: 126908
      Reviewed By: dreiss
      CC: dreiss, kholst, thrift-team@lists
      Revert Plan:
      OK

      Attachments

        Activity

          grayson grayson added a comment -

          this bug can easily fix,thank you for your reply.

          grayson grayson added a comment - this bug can easily fix,thank you for your reply.
          grayson grayson added a comment -

          sorry,the link is not correct,use this https://issues.apache.org/jira/i#browse/THRIFT-2321

          grayson grayson added a comment - sorry,the link is not correct,use this https://issues.apache.org/jira/i#browse/THRIFT-2321
          grayson grayson added a comment -

          Did you try array as a arguments? You will find out that the data in array arguments is missing. This function "SEPARATE_ZVAL(value)", can seperate pointer "value" with a newly allocated memory space. But the "value" point to a new place, and the "value" will be free later, so data in array arguments is lost. More detail in :
          https://issues.apache.org/jira/i#browse/THRIFT-2321?filter=-2

          grayson grayson added a comment - Did you try array as a arguments? You will find out that the data in array arguments is missing. This function "SEPARATE_ZVAL(value)", can seperate pointer "value" with a newly allocated memory space. But the "value" point to a new place, and the "value" will be free later, so data in array arguments is lost. More detail in : https://issues.apache.org/jira/i#browse/THRIFT-2321?filter=-2
          hudson Hudson added a comment -

          Integrated in Thrift #399 (See https://builds.apache.org/job/Thrift/399/)
          Thrift-1453:Don't change types of arguments when serializing with thrift php extension
          Client:php
          patch: Dave Watson

          Don't change types of arguments when serializing with thrift php extension

          hudson Hudson added a comment - Integrated in Thrift #399 (See https://builds.apache.org/job/Thrift/399/ ) Thrift-1453:Don't change types of arguments when serializing with thrift php extension Client:php patch: Dave Watson Don't change types of arguments when serializing with thrift php extension
          jfarrell Jake Farrell added a comment -

          Committed modified patch

          jfarrell Jake Farrell added a comment - Committed modified patch
          jfarrell Jake Farrell added a comment -

          PHP 5.3.x uses the macro Z_ADDREF_P instead of ZVAL_ADDREF. Adding the following to the patch to accommodate for both. Any objections?

          @@ -57,6 +57,10 @@
          #error Unknown __BYTE_ORDER
          #endif

          +#ifndef Z_ADDREF_P
          +#define Z_ADDREF_P ZVAL_ADDREF
          +#endif
          +

          jfarrell Jake Farrell added a comment - PHP 5.3.x uses the macro Z_ADDREF_P instead of ZVAL_ADDREF. Adding the following to the patch to accommodate for both. Any objections? @@ -57,6 +57,10 @@ #error Unknown __BYTE_ORDER #endif +#ifndef Z_ADDREF_P +#define Z_ADDREF_P ZVAL_ADDREF +#endif +

          People

            davejwatson@fb Dave Watson
            davejwatson@fb Dave Watson
            Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: