Commits on May 27, 2021

1. Fixed sub-message getters for well-known types when message is unset. …

   The well-known types generate C code into wkt.inc, and this C code was
   not testing isset($msg->submsg_field) like the generated code does:
   
   ```php
   // PHP generated getter: checks isset().
   public function getOptions()
   {
       return isset($this->options) ? $this->options : null;
   }
   ```
   
   ```c
   // C generated getter, does not check upb_msg_has()
   static PHP_METHOD(google_protobuf_Value, getListValue) {
     Message* intern = (Message*)Z_OBJ_P(getThis());
     const upb_fielddef *f = upb_msgdef_ntofz(intern->desc->msgdef,
                                              "list_value");
     zval ret;
     Message_get(intern, f, &ret);
     RETURN_COPY_VALUE(&ret);
   }
   ```
   
   This led to an error where we wnuld try to get a sub-message field from upb
   when it `upb_msg_has(msg, field) == false`, which is an error according to upb.
   
   There are two possible fixes for this bug. A guiding principle is that we want
   the generated C code in wkt.inc to have the same behavior as PHP generated
   code. Following this principle, the two possible fixes are:
   
   1. Change the code generator for wkt.inc to check upb_msg_has(f) before
      calling Message_get(). This would match the isset() check that the
      The PHP generated code does, and we would leave the PHP code unchanged.
   
   2. Change Message_get() to itself perform the upb_msg_has(f) check for
      sub-message fields. This means that generated code would no longer need
      to perform an isset() check, so we would want to remove this check from
      the PHP generated code also to avoid a redundant check.
   
   Both of these are reasonable fixes, and it is not immediately obvious which is
   better. (1) has the benefit of resolving this case when we are in more
   specialized code (a getter function that already knows this is a sub-message
   field), and therefore avoids performing the check later in more generic code
   that would have to test the type again. On the other hand, the isset() check is
   not needed for the pure PHP implementation, as an unset PHP variable will
   return `null` anyway. And for the C extension, we'd rather check upb_msg_has()
   at the C level instead of PHP.
   
   So this change implements (2). The generated code in wkt.inc remains unchanged,
   and the PHP generated code for sub-message fields is changed to remove the
   isset() check.

   

   haberman committed May 27, 2021
   Copy the full SHA

   75de6aa View commit details

   Browse the repository at this point in the history